[Fuchsia] Update ScopedServiceBinding to use BindingSet.

Previously ScopedServiceBinding was using fidl::Binding instead of
BindingSet, which didn't allow multiple bindings. Also added some new
unittests for ServiceDirectory to validate cases that were not handled
previously. That required an update in ComponentContext class: it now
can handle the case when the target directory is gone.

Change-Id: I7975275bebfba716d97c3f41d13046172385aff7
Reviewed-on: https://chromium-review.googlesource.com/1125308Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572426}

base/fuchsia/component_context.cc

@@ -42,12 +42,10 @@ ComponentContext* ComponentContext::GetDefault() {
   return component_context.get();
 }
 
-void ComponentContext::ConnectToService(FidlInterfaceRequest request) {
+zx_status_t ComponentContext::ConnectToService(FidlInterfaceRequest request) {
   DCHECK(request.is_valid());
-  zx_status_t result =
-      fdio_service_connect_at(service_root_.get(), request.interface_name(),
+  return fdio_service_connect_at(service_root_.get(), request.interface_name(),
                                  request.TakeChannel().release());
-  ZX_CHECK(result == ZX_OK, result) << "fdio_service_connect_at()";
 }
 
 }  // namespace fuchsia

base/fuchsia/component_context.h

@@ -36,7 +36,7 @@ class BASE_EXPORT ComponentContext {
   static ComponentContext* GetDefault();
 
   // Satisfies the interface |request| by binding the channel to a service.
-  void ConnectToService(FidlInterfaceRequest request);
+  zx_status_t ConnectToService(FidlInterfaceRequest request);
 
   // Same as above, but returns interface pointer instead of taking a request.
   template <typename Interface>

base/fuchsia/scoped_service_binding.h

@@ -5,7 +5,7 @@
 #ifndef BASE_FUCHSIA_SCOPED_SERVICE_BINDING_H_
 #define BASE_FUCHSIA_SCOPED_SERVICE_BINDING_H_
 
-#include <lib/fidl/cpp/binding.h>
+#include <lib/fidl/cpp/binding_set.h>
 
 #include "base/bind.h"
 #include "base/fuchsia/service_directory.h"
@@ -18,7 +18,7 @@ class ScopedServiceBinding {
  public:
   // |service_directory| and |impl| must outlive the binding.
   ScopedServiceBinding(ServiceDirectory* service_directory, Interface* impl)
-      : directory_(service_directory), binding_(impl) {
+      : directory_(service_directory), impl_(impl) {
     directory_->AddService(
         Interface::Name_,
         BindRepeating(&ScopedServiceBinding::BindClient, Unretained(this)));
@@ -28,12 +28,13 @@ class ScopedServiceBinding {
 
  private:
   void BindClient(zx::channel channel) {
-    binding_.Bind(
-        typename fidl::InterfaceRequest<Interface>(std::move(channel)));
+    bindings_.AddBinding(impl_,
+                         fidl::InterfaceRequest<Interface>(std::move(channel)));
   }
 
-  ServiceDirectory* directory_;
-  fidl::Binding<Interface> binding_;
+  ServiceDirectory* const directory_;
+  Interface* const impl_;
+  fidl::BindingSet<Interface> bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedServiceBinding);
 };

base/fuchsia/service_directory_unittest.cc

@@ -25,101 +25,116 @@ class TestInterfaceImpl : public test_fidl::TestInterface {
   }
 };
 
-// Verifies that a service connected by ServiceDirectory can be imported from
-// another ServiceDirectory.
-TEST(ServiceDirectoryTest, Connect) {
-  MessageLoopForIO message_loop_;
-
-  zx::channel dir_service_channel;
-  zx::channel dir_client_channel;
-  ASSERT_EQ(zx::channel::create(0, &dir_service_channel, &dir_client_channel),
+class ServiceDirectoryTest : public testing::Test {
+ public:
+  ServiceDirectoryTest() {
+    zx::channel service_directory_channel;
+    EXPECT_EQ(zx::channel::create(0, &service_directory_channel,
+                                  &service_directory_client_channel_),
               ZX_OK);
 
     // Mount service dir and publish the service.
-  ServiceDirectory service_dir(std::move(dir_service_channel));
-  TestInterfaceImpl test_service;
-  ScopedServiceBinding<test_fidl::TestInterface> service_binding(&service_dir,
-                                                                 &test_service);
-
-  // Open public directory from the service directory.
-  zx::channel public_dir_service_channel;
-  zx::channel public_dir_client_channel;
-  ASSERT_EQ(zx::channel::create(0, &public_dir_service_channel,
-                                &public_dir_client_channel),
+    service_directory_ = std::make_unique<ServiceDirectory>(
+        std::move(service_directory_channel));
+    service_binding_ =
+        std::make_unique<ScopedServiceBinding<test_fidl::TestInterface>>(
+            service_directory_.get(), &test_service_);
+
+    ConnectClientContextToDirectory("public");
+  }
+
+  void ConnectClientContextToDirectory(const char* path) {
+    // Open directory |path| from the service directory.
+    zx::channel public_directory_channel;
+    zx::channel public_directory_client_channel;
+    EXPECT_EQ(zx::channel::create(0, &public_directory_channel,
+                                  &public_directory_client_channel),
               ZX_OK);
-  ASSERT_EQ(fdio_open_at(dir_client_channel.get(), "public", 0,
-                         public_dir_service_channel.release()),
+    EXPECT_EQ(fdio_open_at(service_directory_client_channel_.get(), path, 0,
+                           public_directory_channel.release()),
               ZX_OK);
 
     // Create ComponentContext and connect to the test service.
-  ComponentContext client_context(std::move(public_dir_client_channel));
-  auto stub = client_context.ConnectToService<test_fidl::TestInterface>();
+    client_context_ = std::make_unique<ComponentContext>(
+        std::move(public_directory_client_channel));
+  }
 
+  void VerifyTestInterface(fidl::InterfacePtr<test_fidl::TestInterface>* stub,
+                           bool expect_error) {
     // Call the service and wait for response.
     base::RunLoop run_loop;
     bool error = false;
 
-  stub.set_error_handler([&run_loop, &error]() {
+    stub->set_error_handler([&run_loop, &error]() {
       error = true;
       run_loop.Quit();
     });
 
-  stub->Add(2, 2, [&run_loop](int32_t result) {
+    (*stub)->Add(2, 2, [&run_loop](int32_t result) {
       EXPECT_EQ(result, 4);
       run_loop.Quit();
     });
 
     run_loop.Run();
 
-  EXPECT_FALSE(error);
-}
+    EXPECT_EQ(error, expect_error);
 
-// Verify that services are also exported to the legacy flat service namespace.
-TEST(ServiceDirectoryTest, ConnectLegacy) {
-  MessageLoopForIO message_loop_;
+    // Reset error handler because the current one captures |run_loop| and
+    // |error| references which are about to be destroyed.
+    stub->set_error_handler([]() {});
+  }
 
-  zx::channel dir_service_channel;
-  zx::channel dir_client_channel;
-  ASSERT_EQ(zx::channel::create(0, &dir_service_channel, &dir_client_channel),
-            ZX_OK);
+ protected:
+  MessageLoopForIO message_loop_;
+  std::unique_ptr<ServiceDirectory> service_directory_;
+  zx::channel service_directory_client_channel_;
+  TestInterfaceImpl test_service_;
+  std::unique_ptr<ScopedServiceBinding<test_fidl::TestInterface>>
+      service_binding_;
+  std::unique_ptr<ComponentContext> client_context_;
+};
 
-  // Mount service dir and publish the service.
-  ServiceDirectory service_dir(std::move(dir_service_channel));
-  TestInterfaceImpl test_service;
-  ScopedServiceBinding<test_fidl::TestInterface> service_binding(&service_dir,
-                                                                 &test_service);
-
-  // Open public directory from the service directory.
-  zx::channel public_dir_service_channel;
-  zx::channel public_dir_client_channel;
-  ASSERT_EQ(zx::channel::create(0, &public_dir_service_channel,
-                                &public_dir_client_channel),
-            ZX_OK);
-  ASSERT_EQ(fdio_open_at(dir_client_channel.get(), ".", 0,
-                         public_dir_service_channel.release()),
-            ZX_OK);
+// Verifies that ComponentContext can consume a public service in
+// ServiceDirectory.
+TEST_F(ServiceDirectoryTest, Connect) {
+  auto stub = client_context_->ConnectToService<test_fidl::TestInterface>();
+  VerifyTestInterface(&stub, false);
+}
 
-  // Create ComponentContext and connect to the test service.
-  ComponentContext client_context(std::move(public_dir_client_channel));
-  auto stub = client_context.ConnectToService<test_fidl::TestInterface>();
+// Verifies that we can connect to the service service more than once.
+TEST_F(ServiceDirectoryTest, ConnectMulti) {
+  auto stub = client_context_->ConnectToService<test_fidl::TestInterface>();
+  auto stub2 = client_context_->ConnectToService<test_fidl::TestInterface>();
+  VerifyTestInterface(&stub, false);
+  VerifyTestInterface(&stub2, false);
+}
 
-  // Call the service and wait for response.
-  base::RunLoop run_loop;
-  bool error = false;
+// Verify that services are also exported to the legacy flat service namespace.
+TEST_F(ServiceDirectoryTest, ConnectLegacy) {
+  ConnectClientContextToDirectory(".");
+  auto stub = client_context_->ConnectToService<test_fidl::TestInterface>();
+  VerifyTestInterface(&stub, false);
+}
 
-  stub.set_error_handler([&run_loop, &error]() {
-    error = true;
-    run_loop.Quit();
-  });
+// Verify that ComponentContext can handle the case when the service directory
+// connection is disconnected.
+TEST_F(ServiceDirectoryTest, DirectoryGone) {
+  service_binding_.reset();
+  service_directory_.reset();
 
-  stub->Add(2, 2, [&run_loop](int32_t result) {
-    EXPECT_EQ(result, 4);
-    run_loop.Quit();
-  });
+  fidl::InterfacePtr<test_fidl::TestInterface> stub;
+  zx_status_t status =
+      client_context_->ConnectToService(FidlInterfaceRequest(&stub));
+  EXPECT_EQ(status, ZX_ERR_PEER_CLOSED);
 
-  run_loop.Run();
+  VerifyTestInterface(&stub, true);
+}
 
-  EXPECT_FALSE(error);
+// Verify that the case when the service doesn't exist is handled properly.
+TEST_F(ServiceDirectoryTest, NoService) {
+  service_binding_.reset();
+  auto stub = client_context_->ConnectToService<test_fidl::TestInterface>();
+  VerifyTestInterface(&stub, true);
 }
 
 }  // namespace fuchsia