commit 9cd9e88f7e83518a1e4fcdf612d1f2b11aa29749
author Steven Moreland <smoreland@google.com> Fri Feb 01 18:55:42 2019 -0800
committer Steven Moreland <smoreland@google.com> Tue Feb 05 13:51:24 2019 -0800
tree bcd1442163ac15342ba251712d3a3b84b4299ab4
parent 501bc8eeaaebaf7ee6d237586455214a857e80a6

lazy hals: fix onClient(true) double-send.

Before, if guaranteeClient was called, and mHasClients was true
(meaning that we should have a client and there is a record of having
a client), then an onClient(true) notification would be called.
However, since mHasClients is true, we must have already sent a
notification out. A CHECK for this situation was also added.

Test: hwservicemanager_test
    HidlServiceLazyTest AcquisitionAfterGuarantee (now passes)
Test: internal 'lazy_hidl_test' fuzzing health storage HAL, health
    storage HAL no longer reports double-acquisition of clients.

Change-Id: I18489c1b1d085028ce39163035a06ca04fd9f10d

HidlService.cpp

diff --git a/HidlService.cpp b/HidlService.cpp
index 61f6621..ddc0f1a 100644
--- a/HidlService.cpp
+++ b/HidlService.cpp
@@ -52,7 +52,7 @@
     mClientCallbacks.clear();
     mHasClients = false;
     mGuaranteeClient = false;
-    mNoClientsCounter = false;
+    mNoClientsCounter = 0;
 
     sendRegistrationNotifications();
 }
@@ -131,21 +131,30 @@
 
     bool hasClients = count > 1; // this process holds a strong count
 
-    // a client has its first client OR a handle was handed out, but it was immediately dropped
-    if ((hasClients && !mHasClients) || (mGuaranteeClient && !hasClients)) {
-        sendClientCallbackNotifications(true); // for first ref, or for when we handed it out
+    if (mGuaranteeClient) {
+        // we have no record of this client
+        if (!mHasClients && !hasClients) {
+            sendClientCallbackNotifications(true);
+        }
+
+        // guarantee is temporary
+        mGuaranteeClient = false;
+    }
+
+    if (hasClients && !mHasClients) {
+        // client was retrieved in some other way
+        sendClientCallbackNotifications(true);
     }
 
     // there are no more clients, but the callback has not been called yet
-    if (isCalledOnInterval && !hasClients && mHasClients) {
+    if (!hasClients && mHasClients && isCalledOnInterval) {
         mNoClientsCounter++;
+
+        if (mNoClientsCounter >= kNoClientRepeatLimit) {
+            sendClientCallbackNotifications(false);
+        }
     }
 
-    if (mNoClientsCounter >= kNoClientRepeatLimit) {
-        sendClientCallbackNotifications(false);
-    }
-
-    mGuaranteeClient = false;
     return count;
 }
 
@@ -197,6 +206,9 @@
 }
 
 void HidlService::sendClientCallbackNotifications(bool hasClients) {
+    CHECK(hasClients != mHasClients) << "Record shows: " << mHasClients
+        << " so we can't tell clients again that we have client: " << hasClients;
+
     LOG(INFO) << "Notifying " << string() << " they have clients: " << hasClients;
 
     for (const auto& cb : mClientCallbacks) {

test_lazy.cpp

diff --git a/test_lazy.cpp b/test_lazy.cpp
index 0aeac47..542bf53 100644
--- a/test_lazy.cpp
+++ b/test_lazy.cpp
@@ -147,3 +147,26 @@
     service->handleClientCallbacks(true /*onInterval*/);
     ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
 }
+
+TEST_F(HidlServiceLazyTest, AcquisitionAfterGuarantee) {
+    sp<RecordingClientCallback> cb = new RecordingClientCallback;
+
+    std::unique_ptr<HidlService> service = makeService();
+    service->addClientCallback(cb);
+
+    setReportedClientCount(2);
+    service->handleClientCallbacks(false /*onInterval*/);
+    ASSERT_THAT(cb->stream, ElementsAre(true));
+
+    setReportedClientCount(1);
+    service->guaranteeClient();
+
+    service->handleClientCallbacks(false /*onInterval*/);
+    ASSERT_THAT(cb->stream, ElementsAre(true));
+
+    service->handleClientCallbacks(true /*onInterval*/);
+    ASSERT_THAT(cb->stream, ElementsAre(true));
+
+    service->handleClientCallbacks(true /*onInterval*/);
+    ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
+}