video capture: bind all connections to one DeviceFactoryProvider

Currently, the video capture service would create a separate
DeviceFactoryProvider and DeviceFactory for each client.
This could potentially causes undefined behaviors
when there are multiple clients, since each DeviceFactory
manage devices independent of each other, the DeviceFactory
might not have the up-to-date information on all devices.

So fix it by only creating only one instance of DeviceFactoryProvider
for the service, and all clients can bind to the same instance.

Bug: None
Test: manual
Test: services_unittests
Change-Id: I4629b3c80c11cab0a7274b07080ccb2b86ac0e86
Reviewed-on: https://chromium-review.googlesource.com/737955
Commit-Queue: Peter Qiu <zqiu@chromium.org>
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512983}

services/video_capture/BUILD.gn

@@ -60,6 +60,7 @@ source_set("tests") {
   testonly = true
 
   sources = [
+    "test/device_factory_provider_connectortest.cc",
     "test/device_factory_provider_test.cc",
     "test/device_factory_provider_test.h",
     "test/device_factory_provider_unittest.cc",
@@ -85,6 +86,7 @@ source_set("tests") {
     "//media/capture/mojo:capture_types",
     "//services/service_manager/public/cpp",
     "//services/service_manager/public/cpp:service_test_support",
+    "//services/service_manager/public/cpp/test:test_support",
     "//testing/gmock",
     "//testing/gtest",
     "//ui/gfx:test_support",

services/video_capture/service_impl.cc

@@ -38,6 +38,9 @@ void ServiceImpl::OnStart() {
       // Unretained |this| is safe because |registry_| is owned by |this|.
       base::Bind(&ServiceImpl::OnTestingControlsRequest,
                  base::Unretained(this)));
+
+  factory_provider_bindings_.set_connection_error_handler(base::BindRepeating(
+      &ServiceImpl::OnProviderClientDisconnected, base::Unretained(this)));
 }
 
 void ServiceImpl::OnBindInterface(
@@ -54,18 +57,17 @@ bool ServiceImpl::OnServiceManagerConnectionLost() {
   return true;
 }
 
+void ServiceImpl::SetFactoryProviderClientDisconnectedObserver(
+    const base::RepeatingClosure& observer_cb) {
+  factory_provider_client_disconnected_cb_ = observer_cb;
+}
+
 void ServiceImpl::OnDeviceFactoryProviderRequest(
     mojom::DeviceFactoryProviderRequest request) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  mojo::MakeStrongBinding(
-      base::MakeUnique<DeviceFactoryProviderImpl>(
-          ref_factory_->CreateRef(),
-          // Use of unretained |this| is safe, because the
-          // VideoCaptureServiceImpl has shared ownership of |this| via the
-          // reference created by ref_factory->CreateRef().
-          base::Bind(&ServiceImpl::SetShutdownDelayInSeconds,
-                     base::Unretained(this))),
-      std::move(request));
+  LazyInitializeDeviceFactoryProvider();
+  factory_provider_bindings_.AddBinding(device_factory_provider_.get(),
+                                        std::move(request));
 }
 
 void ServiceImpl::OnTestingControlsRequest(
@@ -102,4 +104,28 @@ void ServiceImpl::MaybeRequestQuit() {
   }
 }
 
+void ServiceImpl::LazyInitializeDeviceFactoryProvider() {
+  if (device_factory_provider_)
+    return;
+
+  device_factory_provider_ = std::make_unique<DeviceFactoryProviderImpl>(
+      ref_factory_->CreateRef(),
+      // Use of unretained |this| is safe, because the
+      // VideoCaptureServiceImpl has shared ownership of |this| via the
+      // reference created by ref_factory->CreateRef().
+      base::Bind(&ServiceImpl::SetShutdownDelayInSeconds,
+                 base::Unretained(this)));
+}
+
+void ServiceImpl::OnProviderClientDisconnected() {
+  // Reset factory provider if no client is connected.
+  if (factory_provider_bindings_.empty()) {
+    device_factory_provider_.reset();
+  }
+
+  if (!factory_provider_client_disconnected_cb_.is_null()) {
+    factory_provider_client_disconnected_cb_.Run();
+  }
+}
+
 }  // namespace video_capture

services/video_capture/service_impl.h

@@ -12,6 +12,7 @@
 #include "services/service_manager/public/cpp/binder_registry.h"
 #include "services/service_manager/public/cpp/service.h"
 #include "services/service_manager/public/cpp/service_context_ref.h"
+#include "services/video_capture/device_factory_provider_impl.h"
 #include "services/video_capture/public/interfaces/device_factory_provider.mojom.h"
 #include "services/video_capture/public/interfaces/testing_controls.mojom.h"
 
@@ -33,6 +34,9 @@ class ServiceImpl : public service_manager::Service {
                        mojo::ScopedMessagePipeHandle interface_pipe) override;
   bool OnServiceManagerConnectionLost() override;
 
+  void SetFactoryProviderClientDisconnectedObserver(
+      const base::RepeatingClosure& observer_cb);
+
  private:
   void OnDeviceFactoryProviderRequest(
       mojom::DeviceFactoryProviderRequest request);
@@ -40,6 +44,8 @@ class ServiceImpl : public service_manager::Service {
   void SetShutdownDelayInSeconds(float seconds);
   void MaybeRequestQuitDelayed();
   void MaybeRequestQuit();
+  void LazyInitializeDeviceFactoryProvider();
+  void OnProviderClientDisconnected();
 
 #if defined(OS_WIN)
   // COM must be initialized in order to access the video capture devices.
@@ -47,7 +53,12 @@ class ServiceImpl : public service_manager::Service {
 #endif
   float shutdown_delay_in_seconds_;
   service_manager::BinderRegistry registry_;
+  mojo::BindingSet<mojom::DeviceFactoryProvider> factory_provider_bindings_;
+  std::unique_ptr<DeviceFactoryProviderImpl> device_factory_provider_;
   std::unique_ptr<service_manager::ServiceContextRefFactory> ref_factory_;
+  // Callback to be invoked when a provider client is disconnected. Mainly used
+  // for testing.
+  base::RepeatingClosure factory_provider_client_disconnected_cb_;
   base::ThreadChecker thread_checker_;
   base::WeakPtrFactory<ServiceImpl> weak_factory_;
 

services/video_capture/test/device_factory_provider_connectortest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/run_loop.h"
+#include "base/test/mock_callback.h"
+#include "base/test/scoped_task_environment.h"
+#include "media/base/media_switches.h"
+#include "services/service_manager/public/cpp/test/test_connector_factory.h"
+#include "services/video_capture/public/interfaces/constants.mojom.h"
+#include "services/video_capture/public/interfaces/device_factory_provider.mojom.h"
+#include "services/video_capture/service_impl.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace video_capture {
+
+using testing::Exactly;
+using testing::_;
+using testing::Invoke;
+using testing::InvokeWithoutArgs;
+
+// Test fixture that creates a video_capture::ServiceImpl and sets up a
+// local service_manager::Connector through which client code can connect to
+// it.
+class DeviceFactoryProviderConnectorTest : public ::testing::Test {
+ public:
+  DeviceFactoryProviderConnectorTest() {}
+  ~DeviceFactoryProviderConnectorTest() override {}
+
+  void SetUp() override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kUseFakeDeviceForMediaStream);
+    std::unique_ptr<ServiceImpl> service_impl = std::make_unique<ServiceImpl>();
+    service_impl_ = service_impl.get();
+    connector_factory_ =
+        base::MakeUnique<service_manager::TestConnectorFactory>(
+            std::move(service_impl));
+    connector_ = connector_factory_->CreateConnector();
+    connector_->BindInterface(mojom::kServiceName, &factory_provider_);
+    factory_provider_->SetShutdownDelayInSeconds(0.0f);
+    factory_provider_->ConnectToDeviceFactory(mojo::MakeRequest(&factory_));
+  }
+
+ protected:
+  ServiceImpl* service_impl_;
+  mojom::DeviceFactoryProviderPtr factory_provider_;
+  mojom::DeviceFactoryPtr factory_;
+  base::MockCallback<mojom::DeviceFactory::GetDeviceInfosCallback>
+      device_info_receiver_;
+  std::unique_ptr<service_manager::Connector> connector_;
+
+ private:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  std::unique_ptr<service_manager::TestConnectorFactory> connector_factory_;
+};
+
+TEST_F(DeviceFactoryProviderConnectorTest, ServiceNotQuitWhenClientConnected) {
+  base::RunLoop wait_loop;
+
+  mojom::DeviceFactoryProviderPtr leftover_provider_;
+  connector_->BindInterface(mojom::kServiceName, &leftover_provider_);
+  service_impl_->SetFactoryProviderClientDisconnectedObserver(
+      wait_loop.QuitClosure());
+
+  factory_.reset();
+  factory_provider_.reset();
+
+  wait_loop.Run();
+
+  // Verify that the original provider is not bound while the
+  // |leftover_provider| is still bound.
+  EXPECT_FALSE(factory_provider_.is_bound());
+  EXPECT_TRUE(leftover_provider_.is_bound());
+}
+
+}  // namespace video_capture

services/video_capture/test/device_factory_provider_unittest.cc

@@ -71,11 +71,11 @@ TEST_F(VideoCaptureServiceDeviceFactoryProviderTest,
   wait_loop.Run();
 }
 
-// Tests that the service requests to be closed when the last client disconnects
+// Tests that the service requests to be closed when the only client disconnects
 // after not having done anything other than obtaining a connection to the
 // fake device factory.
 TEST_F(VideoCaptureServiceDeviceFactoryProviderTest,
-       ServiceQuitsWhenNoClientConnected) {
+       ServiceQuitsWhenSingleClientDisconnected) {
   base::RunLoop wait_loop;
   EXPECT_CALL(*service_state_observer_, OnServiceStopped(_))
       .WillOnce(Invoke([&wait_loop](const service_manager::Identity& identity) {
@@ -90,4 +90,28 @@ TEST_F(VideoCaptureServiceDeviceFactoryProviderTest,
   wait_loop.Run();
 }
 
+// Tests that the service requests to be closed when the all clients disconnect
+// after not having done anything other than obtaining a connection to the
+// fake device factory.
+TEST_F(VideoCaptureServiceDeviceFactoryProviderTest,
+       ServiceQuitsWhenAllClientsDisconnected) {
+  // Bind another client to the DeviceFactoryProvider interface.
+  mojom::DeviceFactoryProviderPtr unused_provider;
+  connector()->BindInterface(mojom::kServiceName, &unused_provider);
+
+  base::RunLoop wait_loop;
+  EXPECT_CALL(*service_state_observer_, OnServiceStopped(_))
+      .WillOnce(Invoke([&wait_loop](const service_manager::Identity& identity) {
+        if (identity.name() == mojom::kServiceName)
+          wait_loop.Quit();
+      }));
+
+  // Exercise: Disconnect from service by discarding our references to it.
+  factory_.reset();
+  factory_provider_.reset();
+  unused_provider.reset();
+
+  wait_loop.Run();
+}
+
 }  // namespace video_capture