Merge "Add binder to libbinder cache after addService" into main am: d1cd573

parthsane · android-build-merge-worker-robot · commit 6b7aab856a50 · 2024-12-02T19:46:39.000Z
Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/3350828 Change-Id: I39a6d032dc2f98888a5e7820af49aab5bddbda64 Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/cmds/dumpsys/tests/dumpsys_test.cpp b/cmds/dumpsys/tests/dumpsys_test.cpp
@@ -67,6 +67,8 @@ class ServiceManagerMock : public IServiceManager {
     MOCK_METHOD2(unregisterForNotifications, status_t(const String16&,
                                              const sp<LocalRegistrationCallback>&));
     MOCK_METHOD0(getServiceDebugInfo, std::vector<ServiceDebugInfo>());
+    MOCK_METHOD1(enableAddServiceCache, void(bool));
+
   protected:
     MOCK_METHOD0(onAsBinder, IBinder*());
 };
diff --git a/libs/binder/Android.bp b/libs/binder/Android.bp
@@ -477,9 +477,34 @@ libbinder_client_cache_config {
     },
 }
 
+soong_config_module_type {
+    name: "libbinder_addservice_cache_config",
+    module_type: "cc_defaults",
+    config_namespace: "libbinder",
+    bool_variables: ["release_libbinder_addservice_cache"],
+    properties: [
+        "cflags",
+    ],
+}
+
+libbinder_addservice_cache_config {
+    name: "libbinder_addservice_cache_flag",
+    soong_config_variables: {
+        release_libbinder_addservice_cache: {
+            cflags: ["-DLIBBINDER_ADDSERVICE_CACHE"],
+            conditions_default: {
+                cflags: ["-DNO_LIBBINDER_ADDSERVICE_CACHE"],
+            },
+        },
+    },
+}
+
 cc_defaults {
     name: "libbinder_kernel_defaults",
-    defaults: ["libbinder_client_cache_flag"],
+    defaults: [
+        "libbinder_client_cache_flag",
+        "libbinder_addservice_cache_flag",
+    ],
     srcs: [
         "BufferedTextOutput.cpp",
         "BackendUnifiedServiceManager.cpp",
diff --git a/libs/binder/BackendUnifiedServiceManager.cpp b/libs/binder/BackendUnifiedServiceManager.cpp
@@ -30,6 +30,12 @@ constexpr bool kUseCache = true;
 constexpr bool kUseCache = false;
 #endif
 
+#ifdef LIBBINDER_ADDSERVICE_CACHE
+constexpr bool kUseCacheInAddService = true;
+#else
+constexpr bool kUseCacheInAddService = false;
+#endif
+
 using AidlServiceManager = android::os::IServiceManager;
 using android::os::IAccessor;
 using binder::Status;
@@ -125,31 +131,36 @@ Status BackendUnifiedServiceManager::updateCache(const std::string& serviceName,
     if (!kUseCache) {
         return Status::ok();
     }
+
+    if (service.getTag() == os::Service::Tag::binder) {
+        return updateCache(serviceName, service.get<os::Service::Tag::binder>());
+    }
+    return Status::ok();
+}
+
+Status BackendUnifiedServiceManager::updateCache(const std::string& serviceName,
+                                                 const sp<IBinder>& binder) {
     std::string traceStr;
     if (atrace_is_tag_enabled(ATRACE_TAG_AIDL)) {
         traceStr = "BinderCacheWithInvalidation::updateCache : " + serviceName;
     }
     binder::ScopedTrace aidlTrace(ATRACE_TAG_AIDL, traceStr.c_str());
-
-    if (service.getTag() == os::Service::Tag::binder) {
-        sp<IBinder> binder = service.get<os::Service::Tag::binder>();
-        if (!binder) {
-            binder::ScopedTrace
-                    aidlTrace(ATRACE_TAG_AIDL,
-                              "BinderCacheWithInvalidation::updateCache failed: binder_null");
-        } else if (!binder->isBinderAlive()) {
-            binder::ScopedTrace aidlTrace(ATRACE_TAG_AIDL,
-                                          "BinderCacheWithInvalidation::updateCache failed: "
-                                          "isBinderAlive_false");
-        } else if (mCacheForGetService->isClientSideCachingEnabled(serviceName)) {
-            binder::ScopedTrace aidlTrace(ATRACE_TAG_AIDL,
-                                          "BinderCacheWithInvalidation::updateCache successful");
-            return mCacheForGetService->setItem(serviceName, binder);
-        } else {
-            binder::ScopedTrace aidlTrace(ATRACE_TAG_AIDL,
-                                          "BinderCacheWithInvalidation::updateCache failed: "
-                                          "caching_not_enabled");
-        }
+    if (!binder) {
+        binder::ScopedTrace
+                aidlTrace(ATRACE_TAG_AIDL,
+                          "BinderCacheWithInvalidation::updateCache failed: binder_null");
+    } else if (!binder->isBinderAlive()) {
+        binder::ScopedTrace aidlTrace(ATRACE_TAG_AIDL,
+                                      "BinderCacheWithInvalidation::updateCache failed: "
+                                      "isBinderAlive_false");
+    } else if (mCacheForGetService->isClientSideCachingEnabled(serviceName)) {
+        binder::ScopedTrace aidlTrace(ATRACE_TAG_AIDL,
+                                      "BinderCacheWithInvalidation::updateCache successful");
+        return mCacheForGetService->setItem(serviceName, binder);
+    } else {
+        binder::ScopedTrace aidlTrace(ATRACE_TAG_AIDL,
+                                      "BinderCacheWithInvalidation::updateCache failed: "
+                                      "caching_not_enabled");
     }
     return Status::ok();
 }
@@ -277,7 +288,12 @@ Status BackendUnifiedServiceManager::toBinderService(const ::std::string& name,
 Status BackendUnifiedServiceManager::addService(const ::std::string& name,
                                                 const sp<IBinder>& service, bool allowIsolated,
                                                 int32_t dumpPriority) {
-    return mTheRealServiceManager->addService(name, service, allowIsolated, dumpPriority);
+    Status status = mTheRealServiceManager->addService(name, service, allowIsolated, dumpPriority);
+    // mEnableAddServiceCache is true by default.
+    if (kUseCacheInAddService && mEnableAddServiceCache && status.isOk()) {
+        return updateCache(name, service);
+    }
+    return status;
 }
 Status BackendUnifiedServiceManager::listServices(int32_t dumpPriority,
                                                   ::std::vector<::std::string>* _aidl_return) {
diff --git a/libs/binder/BackendUnifiedServiceManager.h b/libs/binder/BackendUnifiedServiceManager.h
@@ -105,8 +105,7 @@ class BinderCacheWithInvalidation
         binder::ScopedTrace aidlTrace(ATRACE_TAG_AIDL,
                                       "BinderCacheWithInvalidation::setItem Successfully Cached");
         std::lock_guard<std::mutex> lock(mCacheMutex);
-        Entry entry = {.service = item, .deathRecipient = deathRecipient};
-        mCache[key] = entry;
+        mCache[key] = {.service = item, .deathRecipient = deathRecipient};
         return binder::Status::ok();
     }
 
@@ -147,17 +146,20 @@ class BackendUnifiedServiceManager : public android::os::BnServiceManager {
                                         const sp<IBinder>& service) override;
     binder::Status getServiceDebugInfo(::std::vector<os::ServiceDebugInfo>* _aidl_return) override;
 
+    void enableAddServiceCache(bool value) { mEnableAddServiceCache = value; }
     // for legacy ABI
     const String16& getInterfaceDescriptor() const override {
         return mTheRealServiceManager->getInterfaceDescriptor();
     }
 
 private:
+    bool mEnableAddServiceCache = true;
     std::shared_ptr<BinderCacheWithInvalidation> mCacheForGetService;
     sp<os::IServiceManager> mTheRealServiceManager;
     binder::Status toBinderService(const ::std::string& name, const os::Service& in,
                                    os::Service* _out);
     binder::Status updateCache(const std::string& serviceName, const os::Service& service);
+    binder::Status updateCache(const std::string& serviceName, const sp<IBinder>& binder);
     bool returnIfCached(const std::string& serviceName, os::Service* _out);
 };
 
diff --git a/libs/binder/IServiceManager.cpp b/libs/binder/IServiceManager.cpp
@@ -127,6 +127,8 @@ class CppBackendShim : public IServiceManager {
     }
     IBinder* onAsBinder() override { return IInterface::asBinder(mUnifiedServiceManager).get(); }
 
+    void enableAddServiceCache(bool value) { mUnifiedServiceManager->enableAddServiceCache(value); }
+
 protected:
     sp<BackendUnifiedServiceManager> mUnifiedServiceManager;
     // AidlRegistrationCallback -> services that its been registered for
diff --git a/libs/binder/include/binder/IServiceManager.h b/libs/binder/include/binder/IServiceManager.h
@@ -150,6 +150,12 @@ class LIBBINDER_EXPORTED IServiceManager : public IInterface {
         int pid;
     };
     virtual std::vector<ServiceDebugInfo> getServiceDebugInfo() = 0;
+
+    /**
+     * Directly enable or disable caching binder during addService calls.
+     * Only used for testing.
+     */
+    virtual void enableAddServiceCache(bool value) = 0;
 };
 
 LIBBINDER_EXPORTED sp<IServiceManager> defaultServiceManager();
diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp
@@ -70,7 +70,10 @@ cc_test {
     static_libs: [
         "libfakeservicemanager",
     ],
-    defaults: ["libbinder_client_cache_flag"],
+    defaults: [
+        "libbinder_client_cache_flag",
+        "libbinder_addservice_cache_flag",
+    ],
     test_suites: ["general-tests"],
     require_root: true,
 }
diff --git a/libs/binder/tests/binderCacheUnitTest.cpp b/libs/binder/tests/binderCacheUnitTest.cpp
@@ -34,6 +34,12 @@ constexpr bool kUseLibbinderCache = true;
 constexpr bool kUseLibbinderCache = false;
 #endif
 
+#ifdef LIBBINDER_ADDSERVICE_CACHE
+constexpr bool kUseCacheInAddService = true;
+#else
+constexpr bool kUseCacheInAddService = false;
+#endif
+
 // A service name which is in the static list of cachable services
 const String16 kCachedServiceName = String16("isub");
 
@@ -74,14 +80,58 @@ class MockAidlServiceManager : public os::IServiceManagerDefault {
                 innerSm.addService(String16(name.c_str()), service, allowIsolated, dumpPriority));
     }
 
+    void clearServices() { innerSm.clear(); }
+
     FakeServiceManager innerSm;
 };
 
+class LibbinderCacheAddServiceTest : public ::testing::Test {
+protected:
+    void SetUp() override {
+        fakeServiceManager = sp<MockAidlServiceManager>::make();
+        mServiceManager = getServiceManagerShimFromAidlServiceManagerForTests(fakeServiceManager);
+        mServiceManager->enableAddServiceCache(true);
+    }
+
+    void TearDown() override {}
+
+public:
+    void cacheAddServiceAndConfirmCacheHit(const sp<IBinder>& binder1) {
+        // Add a service. This also caches it.
+        EXPECT_EQ(OK, mServiceManager->addService(kCachedServiceName, binder1));
+        // remove services from fakeservicemanager
+        fakeServiceManager->clearServices();
+
+        sp<IBinder> result = mServiceManager->checkService(kCachedServiceName);
+        if (kUseCacheInAddService && kUseLibbinderCache) {
+            // If cache is enabled, we should get the binder.
+            EXPECT_EQ(binder1, result);
+        } else {
+            // If cache is disabled, then we should get the null binder
+            EXPECT_EQ(nullptr, result);
+        }
+    }
+    sp<MockAidlServiceManager> fakeServiceManager;
+    sp<android::IServiceManager> mServiceManager;
+};
+
+TEST_F(LibbinderCacheAddServiceTest, AddLocalServiceAndConfirmCacheHit) {
+    sp<IBinder> binder1 = sp<BBinder>::make();
+    cacheAddServiceAndConfirmCacheHit(binder1);
+}
+
+TEST_F(LibbinderCacheAddServiceTest, AddRemoteServiceAndConfirmCacheHit) {
+    sp<IBinder> binder1 = defaultServiceManager()->checkService(kServerName);
+    ASSERT_NE(binder1, nullptr);
+    cacheAddServiceAndConfirmCacheHit(binder1);
+}
+
 class LibbinderCacheTest : public ::testing::Test {
 protected:
     void SetUp() override {
-        sp<MockAidlServiceManager> sm = sp<MockAidlServiceManager>::make();
-        mServiceManager = getServiceManagerShimFromAidlServiceManagerForTests(sm);
+        fakeServiceManager = sp<MockAidlServiceManager>::make();
+        mServiceManager = getServiceManagerShimFromAidlServiceManagerForTests(fakeServiceManager);
+        mServiceManager->enableAddServiceCache(false);
     }
 
     void TearDown() override {}
@@ -108,6 +158,7 @@ class LibbinderCacheTest : public ::testing::Test {
         }
     }
 
+    sp<MockAidlServiceManager> fakeServiceManager;
     sp<android::IServiceManager> mServiceManager;
 };
 
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
@@ -220,6 +220,8 @@ class BinderLibTestEnv : public ::testing::Environment {
             ASSERT_GT(m_serverpid, 0);
 
             sp<IServiceManager> sm = defaultServiceManager();
+            // disable caching during addService.
+            sm->enableAddServiceCache(false);
             //printf("%s: pid %d, get service\n", __func__, m_pid);
             LIBBINDER_IGNORE("-Wdeprecated-declarations")
             m_server = sm->getService(binderLibTestServiceName);
@@ -295,6 +297,9 @@ class BinderLibTest : public ::testing::Test {
         virtual void SetUp() {
             m_server = static_cast<BinderLibTestEnv *>(binder_env)->getServer();
             IPCThreadState::self()->restoreCallingWorkSource(0);
+            sp<IServiceManager> sm = defaultServiceManager();
+            // disable caching during addService.
+            sm->enableAddServiceCache(false);
         }
         virtual void TearDown() {
         }
@@ -1644,6 +1649,7 @@ TEST_F(BinderLibTest, TooManyFdsFlattenable) {
 
 TEST(ServiceNotifications, Unregister) {
     auto sm = defaultServiceManager();
+    sm->enableAddServiceCache(false);
     using LocalRegistrationCallback = IServiceManager::LocalRegistrationCallback;
     class LocalRegistrationCallbackImpl : public virtual LocalRegistrationCallback {
         void onServiceRegistration(const String16 &, const sp<IBinder> &) override {}
@@ -2374,6 +2380,8 @@ int run_server(int index, int readypipefd, bool usePoll)
 
     status_t ret;
     sp<IServiceManager> sm = defaultServiceManager();
+    sm->enableAddServiceCache(false);
+
     BinderLibTestService* testServicePtr;
     {
         sp<BinderLibTestService> testService = new BinderLibTestService(index);
diff --git a/libs/fakeservicemanager/include/fakeservicemanager/FakeServiceManager.h b/libs/fakeservicemanager/include/fakeservicemanager/FakeServiceManager.h
@@ -65,6 +65,7 @@ class FakeServiceManager : public IServiceManager {
 
     std::vector<IServiceManager::ServiceDebugInfo> getServiceDebugInfo() override;
 
+    void enableAddServiceCache(bool /*value*/) override {}
     // Clear all of the registered services
     void clear();
 

Original file line number	Diff line number	Diff line change
`@@ -127,6 +127,8 @@ class CppBackendShim : public IServiceManager {`
`127`	`127`	`}`
`128`	`128`	`IBinder* onAsBinder() override { return IInterface::asBinder(mUnifiedServiceManager).get(); }`
`129`	`129`
	`130`	`+ void enableAddServiceCache(bool value) { mUnifiedServiceManager->enableAddServiceCache(value); }`
	`131`	`+`
`130`	`132`	`protected:`
`131`	`133`	`sp<BackendUnifiedServiceManager> mUnifiedServiceManager;`
`132`	`134`	`// AidlRegistrationCallback -> services that its been registered for`