[Fuchsia] Add AddKeepAliveBinding() helper to ComponentStateBase.

Change-Id: Ia7e7399d266530754895ec14ec6e5a3876d5a157
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1496324Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637954}

base/fuchsia/service_provider_impl.h

@@ -37,6 +37,9 @@ class BASE_EXPORT ServiceProviderImpl : public ::fuchsia::sys::ServiceProvider {
   void SetOnLastClientDisconnectedClosure(
       base::OnceClosure on_last_client_disconnected);
 
+  // Returns true if one or more clients are connected.
+  bool has_clients() const { return bindings_.size() != 0; }
+
  private:
   // fuchsia::sys::ServiceProvider implementation.
   void ConnectToService(std::string service_name,

fuchsia/base/agent_impl.cc

@@ -20,11 +20,22 @@ AgentImpl::ComponentStateBase::ComponentStateBase(
 
   // Tear down this instance when the client disconnects from the directory.
   service_provider_->SetOnLastClientDisconnectedClosure(base::BindOnce(
-      &ComponentStateBase::OnComponentDisconnected, base::Unretained(this)));
+      &ComponentStateBase::TeardownIfUnused, base::Unretained(this)));
 }
 
-void AgentImpl::ComponentStateBase::OnComponentDisconnected() {
+void AgentImpl::ComponentStateBase::TeardownIfUnused() {
   DCHECK(agent_impl_);
+
+  // Don't teardown if the ServiceProvider has client(s).
+  if (service_provider_->has_clients())
+    return;
+
+  // Don't teardown if caller-specified bindings still have clients.
+  for (auto& keepalive_callback : keepalive_callbacks_) {
+    if (keepalive_callback.Run())
+      return;
+  }
+
   agent_impl_->DeleteComponentState(component_id_);
   // Do not touch |this|, since it is already gone.
 }

fuchsia/base/agent_impl.h

@@ -63,15 +63,31 @@ class AgentImpl : public ::fuchsia::modular::Agent {
       return &service_directory_;
     }
 
+    // Registers |service_binding| to prevent teardown of this
+    // ComponentStateBase while it has one or more clients. |service_binding|
+    // will typically be the ScopedServiceBinding<> of a critical service used
+    // by the component.
+    template <typename T>
+    void AddKeepAliveBinding(T* service_binding) {
+      keepalive_callbacks_.push_back(base::BindRepeating(
+          &T::has_clients, base::Unretained(service_binding)));
+      service_binding->SetOnLastClientCallback(base::BindRepeating(
+          &ComponentStateBase::TeardownIfUnused, base::Unretained(this)));
+    }
+
    private:
     friend class AgentImpl;
 
-    void OnComponentDisconnected();
+    // Tears down this instance if there are no ServiceProvider clients, and
+    // no |keepalive_callbacks_| indicate that there are no clients of
+    // bindings that were configured to keep us alive.
+    void TeardownIfUnused();
 
     const std::string component_id_;
     AgentImpl* agent_impl_ = nullptr;
     base::fuchsia::ServiceDirectory service_directory_;
     std::unique_ptr<base::fuchsia::ServiceProviderImpl> service_provider_;
+    std::vector<base::RepeatingCallback<bool()>> keepalive_callbacks_;
 
     DISALLOW_COPY_AND_ASSIGN(ComponentStateBase);
   };

fuchsia/base/agent_impl_unittests.cc

@@ -21,6 +21,7 @@ namespace {
 const char kNoServicesComponentId[] = "no-services";
 const char kAccumulatorComponentId1[] = "accumulator1";
 const char kAccumulatorComponentId2[] = "accumulator2";
+const char kKeepAliveComponentId[] = "keep-alive";
 
 class EmptyComponentState : public AgentImpl::ComponentStateBase {
  public:
@@ -51,12 +52,20 @@ class AccumulatorComponentState : public AgentImpl::ComponentStateBase {
       : ComponentStateBase(component),
         service_binding_(service_directory(), &service_) {}
 
- private:
+ protected:
   AccumulatingTestInterfaceImpl service_;
   base::fuchsia::ScopedServiceBinding<base::fuchsia::testfidl::TestInterface>
       service_binding_;
 };
 
+class KeepAliveComponentState : public AccumulatorComponentState {
+ public:
+  KeepAliveComponentState(base::StringPiece component)
+      : AccumulatorComponentState(component) {
+    AddKeepAliveBinding(&service_binding_);
+  }
+};
+
 class AgentImplTest : public ::testing::Test {
  public:
   AgentImplTest() {
@@ -85,6 +94,8 @@ class AgentImplTest : public ::testing::Test {
     } else if (component_id == kAccumulatorComponentId1 ||
                component_id == kAccumulatorComponentId2) {
       return std::make_unique<AccumulatorComponentState>(component_id);
+    } else if (component_id == kKeepAliveComponentId) {
+      return std::make_unique<KeepAliveComponentState>(component_id);
     }
     return nullptr;
   }
@@ -203,10 +214,14 @@ TEST_F(AgentImplTest, DifferentComponentIdSameService) {
       test_interface1.NewRequest().TakeChannel());
 
   // Both TestInterface pointers should remain valid until we are done.
-  test_interface1.set_error_handler(
-      [](zx_status_t status) { ZX_LOG(FATAL, status); });
-  test_interface2.set_error_handler(
-      [](zx_status_t status) { ZX_LOG(FATAL, status); });
+  test_interface1.set_error_handler([](zx_status_t status) {
+    ZX_LOG(ERROR, status);
+    ADD_FAILURE();
+  });
+  test_interface2.set_error_handler([](zx_status_t status) {
+    ZX_LOG(ERROR, status);
+    ADD_FAILURE();
+  });
 
   // Call Add() via one TestInterface and verify accumulator had been at zero.
   {
@@ -255,10 +270,14 @@ TEST_F(AgentImplTest, SameComponentIdSameService) {
       test_interface1.NewRequest().TakeChannel());
 
   // Both TestInterface pointers should remain valid until we are done.
-  test_interface1.set_error_handler(
-      [](zx_status_t status) { ZX_LOG(FATAL, status); });
-  test_interface2.set_error_handler(
-      [](zx_status_t status) { ZX_LOG(FATAL, status); });
+  test_interface1.set_error_handler([](zx_status_t status) {
+    ZX_LOG(ERROR, status);
+    ADD_FAILURE();
+  });
+  test_interface2.set_error_handler([](zx_status_t status) {
+    ZX_LOG(ERROR, status);
+    ADD_FAILURE();
+  });
 
   // Call Add() via one TestInterface and verify accumulator had been at zero.
   {
@@ -285,4 +304,54 @@ TEST_F(AgentImplTest, SameComponentIdSameService) {
   test_interface2.set_error_handler(nullptr);
 }
 
+// Verify that connections to a service registered to kee-alive the
+// ComponentStateBase keeps it alive after the ServiceProvider is dropped.
+TEST_F(AgentImplTest, KeepAliveBinding) {
+  fuchsia::modular::AgentPtr agent = CreateAgentAndConnect();
+
+  {
+    // Connect to the Agent and request the TestInterface.
+    fuchsia::sys::ServiceProviderPtr component_services;
+    agent->Connect(kAccumulatorComponentId1, component_services.NewRequest());
+    base::fuchsia::testfidl::TestInterfacePtr test_interface;
+    component_services->ConnectToService(
+        base::fuchsia::testfidl::TestInterface::Name_,
+        test_interface.NewRequest().TakeChannel());
+
+    // The TestInterface pointer should be closed as soon as we Unbind() the
+    // ServiceProvider.
+    test_interface.set_error_handler(
+        [](zx_status_t status) { EXPECT_EQ(status, ZX_ERR_PEER_CLOSED); });
+
+    // After disconnecting ServiceProvider, TestInterface should remain valid.
+    component_services.Unbind();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_FALSE(test_interface);
+  }
+
+  {
+    // Connect to the Agent and request the TestInterface.
+    fuchsia::sys::ServiceProviderPtr component_services;
+    agent->Connect(kKeepAliveComponentId, component_services.NewRequest());
+    base::fuchsia::testfidl::TestInterfacePtr test_interface;
+    component_services->ConnectToService(
+        base::fuchsia::testfidl::TestInterface::Name_,
+        test_interface.NewRequest().TakeChannel());
+
+    // The TestInterface pointer should remain valid until we are done.
+    test_interface.set_error_handler([](zx_status_t status) {
+      ZX_LOG(ERROR, status);
+      ADD_FAILURE();
+    });
+
+    // After disconnecting ServiceProvider, TestInterface should remain valid.
+    component_services.Unbind();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(test_interface);
+  }
+
+  // Spin the MessageLoop to let the AgentImpl see that TestInterface is gone.
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace cr_fuchsia