commit bd603f76a0008cb5943a649af8a59785be21fa25
author Steven Moreland <smoreland@google.com> Mon Apr 06 14:21:55 2020 -0700
committer Steven Moreland <smoreland@google.com> Mon Apr 06 16:25:24 2020 -0700
tree cf65a09089a140435fbd932b6ab9f3547b0edd12
parent bb842a65ed0be3e762b1f2214fa20a45cd5ade83

Harder failures for lazy service clients.

Detect when hwservicemanager state is inconsistent at runtime.

Bug: 140310064
Test: hidl_lazy_test
Change-Id: I891368d7c4ef346b521c81a77e02214df50a5511

transport/HidlLazyUtils.cpp

diff --git a/transport/HidlLazyUtils.cpp b/transport/HidlLazyUtils.cpp
index 08ed676..b943c99 100644
--- a/transport/HidlLazyUtils.cpp
+++ b/transport/HidlLazyUtils.cpp
@@ -29,15 +29,27 @@
 using ::android::hidl::base::V1_0::IBase;
 
 class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCallback {
-   public:
-    ClientCounterCallback() : mNumConnectedServices(0) {}
+  public:
+    ClientCounterCallback() {}
 
     bool addRegisteredService(const sp<IBase>& service, const std::string& name);
 
-   protected:
+  protected:
     Return<void> onClients(const sp<IBase>& service, bool clients) override;
 
-   private:
+  private:
+    struct Service {
+        sp<IBase> service;
+        std::string name;
+        bool clients = false;
+    };
+
+    /**
+     * Looks up service that is guaranteed to be registered (service from
+     * onClients).
+     */
+    Service& assertRegisteredService(const sp<IBase>& service);
+
     /**
      * Registers or re-registers services. Returns whether successful.
      */
@@ -50,28 +62,19 @@
     void tryShutdown();
 
     /**
-     * Counter of the number of services that currently have at least one client.
-     */
-    size_t mNumConnectedServices;
-
-    struct Service {
-        sp<IBase> service;
-        std::string name;
-    };
-    /**
      * Number of services that have been registered.
      */
     std::vector<Service> mRegisteredServices;
 };
 
 class LazyServiceRegistrarImpl {
-   public:
+  public:
     LazyServiceRegistrarImpl() : mClientCallback(new ClientCounterCallback) {}
 
     status_t registerService(const sp<::android::hidl::base::V1_0::IBase>& service,
                              const std::string& name);
 
-   private:
+  private:
     sp<ClientCounterCallback> mClientCallback;
 };
 
@@ -86,6 +89,17 @@
     return success;
 }
 
+ClientCounterCallback::Service& ClientCounterCallback::assertRegisteredService(
+        const sp<IBase>& service) {
+    for (Service& registered : mRegisteredServices) {
+        if (registered.service != service) continue;
+        return registered;
+    }
+    LOG(FATAL) << "Got callback on service " << getDescriptor(service.get())
+               << " which we did not register.";
+    __builtin_unreachable();
+}
+
 bool ClientCounterCallback::registerService(const sp<IBase>& service, const std::string& name) {
     auto manager = hardware::defaultServiceManager1_2();
 
@@ -114,17 +128,24 @@
  */
 Return<void> ClientCounterCallback::onClients(const sp<::android::hidl::base::V1_0::IBase>& service,
                                               bool clients) {
-    if (clients) {
-        mNumConnectedServices++;
-    } else {
-        mNumConnectedServices--;
+    Service& registered = assertRegisteredService(service);
+    if (registered.clients == clients) {
+        LOG(FATAL) << "Process already thought " << getDescriptor(service.get()) << "/"
+                   << registered.name << " had clients: " << registered.clients
+                   << " but hwservicemanager has notified has clients: " << clients;
+    }
+    registered.clients = clients;
+
+    size_t numWithClients = 0;
+    for (const Service& registered : mRegisteredServices) {
+        if (registered.clients) numWithClients++;
     }
 
-    LOG(INFO) << "Process has " << mNumConnectedServices << " (of " << mRegisteredServices.size()
+    LOG(INFO) << "Process has " << numWithClients << " (of " << mRegisteredServices.size()
               << " available) client(s) in use after notification " << getDescriptor(service.get())
-              << " has clients: " << clients;
+              << "/" << registered.name << " has clients: " << clients;
 
-    if (mNumConnectedServices == 0) {
+    if (numWithClients == 0) {
         tryShutdown();
     }