Implement keepAliveBinder for setRpcClientDebug

setRpcClientDebug can now be called multiple times. Each will
set up a different RpcServer and opens up a different port.

When keepAliveBinder dies, the corresponding RpcServer also
dies.

Also fixes tests to meet new expectations.
- Positive tests now only run on remote binder, because the
  keepAliveBinder object cannot be a local binder to call
  linkToDeath.
- Negative tests keeps running on local and remote binders.
- Use real servicedispatcher.

Test: binderLibTest
Test: manual with servicedispatcher.

Bug: 182914638

Change-Id: Ic46ac54bb7988ee7d65546563f56f0a02c29ce7c
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 19a22a5..d811b17 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -17,6 +17,7 @@
 #include <binder/Binder.h>
 
 #include <atomic>
+#include <set>
 
 #include <android-base/unique_fd.h>
 #include <binder/BpBinder.h>
@@ -180,6 +181,38 @@
 
 // ---------------------------------------------------------------------------
 
+class BBinder::RpcServerLink : public IBinder::DeathRecipient {
+public:
+    // On binder died, calls RpcServer::shutdown on @a rpcServer, and removes itself from @a binder.
+    RpcServerLink(const sp<RpcServer>& rpcServer, const sp<IBinder>& keepAliveBinder,
+                  const wp<BBinder>& binder)
+          : mRpcServer(rpcServer), mKeepAliveBinder(keepAliveBinder), mBinder(binder) {}
+    void binderDied(const wp<IBinder>&) override {
+        LOG_RPC_DETAIL("RpcServerLink: binder died, shutting down RpcServer");
+        if (mRpcServer == nullptr) {
+            ALOGW("RpcServerLink: Unable to shut down RpcServer because it does not exist.");
+        } else {
+            ALOGW_IF(!mRpcServer->shutdown(),
+                     "RpcServerLink: RpcServer did not shut down properly. Not started?");
+        }
+        mRpcServer.clear();
+
+        auto promoted = mBinder.promote();
+        if (promoted == nullptr) {
+            ALOGW("RpcServerLink: Unable to remove link from parent binder object because parent "
+                  "binder object is gone.");
+        } else {
+            promoted->removeRpcServerLink(sp<RpcServerLink>::fromExisting(this));
+        }
+        mBinder.clear();
+    }
+
+private:
+    sp<RpcServer> mRpcServer;
+    sp<IBinder> mKeepAliveBinder; // hold to avoid automatically unlinking
+    wp<BBinder> mBinder;
+};
+
 class BBinder::Extras
 {
 public:
@@ -192,7 +225,7 @@
 
     // for below objects
     Mutex mLock;
-    sp<RpcServer> mRpcServer;
+    std::set<sp<RpcServerLink>> mRpcServerLinks;
     BpBinder::ObjectManager mObjects;
 };
 
@@ -489,24 +522,38 @@
         return INVALID_OPERATION;
     }
 
+    // Weak ref to avoid circular dependency:
+    // BBinder -> RpcServerLink ----> RpcServer -X-> BBinder
+    //                          `-X-> BBinder
+    auto weakThis = wp<BBinder>::fromExisting(this);
+
     Extras* e = getOrCreateExtras();
     AutoMutex _l(e->mLock);
-    if (e->mRpcServer != nullptr) {
-        ALOGE("%s: Already have RPC client", __PRETTY_FUNCTION__);
-        return ALREADY_EXISTS;
+    auto rpcServer = RpcServer::make();
+    LOG_ALWAYS_FATAL_IF(rpcServer == nullptr, "RpcServer::make returns null");
+    rpcServer->iUnderstandThisCodeIsExperimentalAndIWillNotUseItInProduction();
+    auto link = sp<RpcServerLink>::make(rpcServer, keepAliveBinder, weakThis);
+    if (auto status = keepAliveBinder->linkToDeath(link, nullptr, 0); status != OK) {
+        ALOGE("%s: keepAliveBinder->linkToDeath returns %s", __PRETTY_FUNCTION__,
+              statusToString(status).c_str());
+        return status;
     }
-    e->mRpcServer = RpcServer::make();
-    LOG_ALWAYS_FATAL_IF(e->mRpcServer == nullptr, "RpcServer::make returns null");
-    e->mRpcServer->iUnderstandThisCodeIsExperimentalAndIWillNotUseItInProduction();
-    // Weak ref to avoid circular dependency: BBinder -> RpcServer -X-> BBinder
-    e->mRpcServer->setRootObjectWeak(wp<BBinder>::fromExisting(this));
-    e->mRpcServer->setupExternalServer(std::move(socketFd));
-    e->mRpcServer->setMaxThreads(binderThreadPoolMaxCount);
-    e->mRpcServer->start();
+    rpcServer->setRootObjectWeak(weakThis);
+    rpcServer->setupExternalServer(std::move(socketFd));
+    rpcServer->setMaxThreads(binderThreadPoolMaxCount);
+    rpcServer->start();
+    e->mRpcServerLinks.emplace(link);
     LOG_RPC_DETAIL("%s(fd=%d) successful", __PRETTY_FUNCTION__, socketFdForPrint);
     return OK;
 }
 
+void BBinder::removeRpcServerLink(const sp<RpcServerLink>& link) {
+    Extras* e = mExtras.load(std::memory_order_acquire);
+    if (!e) return;
+    AutoMutex _l(e->mLock);
+    (void)e->mRpcServerLinks.erase(link);
+}
+
 BBinder::~BBinder()
 {
     Extras* e = mExtras.load(std::memory_order_relaxed);
diff --git a/libs/binder/include/binder/Binder.h b/libs/binder/include/binder/Binder.h
index 0cc0ef1..472e546 100644
--- a/libs/binder/include/binder/Binder.h
+++ b/libs/binder/include/binder/Binder.h
@@ -117,11 +117,13 @@
                         BBinder(const BBinder& o);
             BBinder&    operator=(const BBinder& o);
 
+    class RpcServerLink;
     class Extras;
 
     Extras*             getOrCreateExtras();
 
     [[nodiscard]] status_t setRpcClientDebug(const Parcel& data);
+    void removeRpcServerLink(const sp<RpcServerLink>& link);
 
     std::atomic<Extras*> mExtras;
 
diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h
index 122c595..f9cdac7 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -162,8 +162,8 @@
      * 2. spawns 1 new thread that calls RpcServer::join()
      *    - join() spawns some number of threads that accept() connections; see RpcServer
      *
-     * setRpcClientDebug() may only be called once.
-     * TODO(b/182914638): If allow to shut down the client, addRpcClient can be called repeatedly.
+     * setRpcClientDebug() may be called multiple times. Each call will add a new RpcServer
+     * and opens up a TCP port.
      *
      * Note: A thread is spawned for each accept()'ed fd, which may call into functions of the
      * interface freely. See RpcServer::join(). To avoid such race conditions, implement the service
diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp
index c7c899f..326d9fd 100644
--- a/libs/binder/tests/Android.bp
+++ b/libs/binder/tests/Android.bp
@@ -62,6 +62,7 @@
     shared_libs: [
         "libbase",
         "libbinder",
+        "liblog",
         "libutils",
     ],
     static_libs: [
@@ -104,6 +105,7 @@
     shared_libs: [
         "libbase",
         "libbinder",
+        "liblog",
         "libutils",
     ],
     static_libs: [
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index abbb16e..1ecb0b7 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -15,14 +15,13 @@
  */
 
 #include <errno.h>
-#include <fcntl.h>
-#include <fstream>
 #include <poll.h>
 #include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
 
 #include <chrono>
+#include <fstream>
 #include <thread>
 
 #include <gmock/gmock.h>
@@ -31,6 +30,7 @@
 #include <android-base/properties.h>
 #include <android-base/result-gmock.h>
 #include <android-base/result.h>
+#include <android-base/strings.h>
 #include <android-base/unique_fd.h>
 #include <binder/Binder.h>
 #include <binder/BpBinder.h>
@@ -55,6 +55,7 @@
 using namespace std::string_literals;
 using namespace std::chrono_literals;
 using android::base::testing::HasValue;
+using android::base::testing::Ok;
 using testing::ExplainMatchResult;
 using testing::Not;
 using testing::WithParamInterface;
@@ -1200,41 +1201,19 @@
     }
 };
 
-class BinderLibRpcTest : public BinderLibRpcTestBase, public WithParamInterface<bool> {
-public:
-    sp<IBinder> GetService() {
-        return GetParam() ? sp<IBinder>(addServer()) : sp<IBinder>(sp<BBinder>::make());
-    }
-    static std::string ParamToString(const testing::TestParamInfo<ParamType> &info) {
-        return info.param ? "remote" : "local";
-    }
-};
+class BinderLibRpcTest : public BinderLibRpcTestBase {};
 
-TEST_P(BinderLibRpcTest, SetRpcClientDebug) {
-    auto binder = GetService();
+TEST_F(BinderLibRpcTest, SetRpcClientDebug) {
+    auto binder = addServer();
     ASSERT_TRUE(binder != nullptr);
     auto [socket, port] = CreateSocket();
     ASSERT_TRUE(socket.ok());
     EXPECT_THAT(binder->setRpcClientDebug(std::move(socket), sp<BBinder>::make()), StatusEq(OK));
 }
 
-TEST_P(BinderLibRpcTest, SetRpcClientDebugNoFd) {
-    auto binder = GetService();
-    ASSERT_TRUE(binder != nullptr);
-    EXPECT_THAT(binder->setRpcClientDebug(android::base::unique_fd(), sp<BBinder>::make()),
-                StatusEq(BAD_VALUE));
-}
-
-TEST_P(BinderLibRpcTest, SetRpcClientDebugNoKeepAliveBinder) {
-    auto binder = GetService();
-    ASSERT_TRUE(binder != nullptr);
-    auto [socket, port] = CreateSocket();
-    ASSERT_TRUE(socket.ok());
-    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket), nullptr), StatusEq(UNEXPECTED_NULL));
-}
-
-TEST_P(BinderLibRpcTest, SetRpcClientDebugTwice) {
-    auto binder = GetService();
+// Tests for multiple RpcServer's on the same binder object.
+TEST_F(BinderLibRpcTest, SetRpcClientDebugTwice) {
+    auto binder = addServer();
     ASSERT_TRUE(binder != nullptr);
 
     auto [socket1, port1] = CreateSocket();
@@ -1245,12 +1224,37 @@
     auto [socket2, port2] = CreateSocket();
     ASSERT_TRUE(socket2.ok());
     auto keepAliveBinder2 = sp<BBinder>::make();
-    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket2), keepAliveBinder2),
-                StatusEq(ALREADY_EXISTS));
+    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket2), keepAliveBinder2), StatusEq(OK));
 }
 
-INSTANTIATE_TEST_CASE_P(BinderLibTest, BinderLibRpcTest, testing::Bool(),
-                        BinderLibRpcTest::ParamToString);
+// Negative tests for RPC APIs on IBinder. Call should fail in the same way on both remote and
+// local binders.
+class BinderLibRpcTestP : public BinderLibRpcTestBase, public WithParamInterface<bool> {
+public:
+    sp<IBinder> GetService() {
+        return GetParam() ? sp<IBinder>(addServer()) : sp<IBinder>(sp<BBinder>::make());
+    }
+    static std::string ParamToString(const testing::TestParamInfo<ParamType> &info) {
+        return info.param ? "remote" : "local";
+    }
+};
+
+TEST_P(BinderLibRpcTestP, SetRpcClientDebugNoFd) {
+    auto binder = GetService();
+    ASSERT_TRUE(binder != nullptr);
+    EXPECT_THAT(binder->setRpcClientDebug(android::base::unique_fd(), sp<BBinder>::make()),
+                StatusEq(BAD_VALUE));
+}
+
+TEST_P(BinderLibRpcTestP, SetRpcClientDebugNoKeepAliveBinder) {
+    auto binder = GetService();
+    ASSERT_TRUE(binder != nullptr);
+    auto [socket, port] = CreateSocket();
+    ASSERT_TRUE(socket.ok());
+    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket), nullptr), StatusEq(UNEXPECTED_NULL));
+}
+INSTANTIATE_TEST_CASE_P(BinderLibTest, BinderLibRpcTestP, testing::Bool(),
+                        BinderLibRpcTestP::ParamToString);
 
 class BinderLibTestService;
 class BinderLibRpcClientTest : public BinderLibRpcTestBase,