[Video capture service] Improve delayed service quit logic

The video capture service shuts down when not in use. The current logic
triggers a 5 second timer when the last client disconnects. When the
timer expires, it either stays alive if there now is a client connect or
it shuts down if there still is no client connected.

This logic is not very suitable for usage patterns where clients may
connect frequently but only for very brief periods of time. If that
happens, the service may end up shutting down and starting back up again
frequently.

This CL changes the logic such that when a new client connects, any previous
timers are cancelled.

Also part of this CL:
* Move tests that are concerned with verifying when the service does or
  does not quit closer to the implementation. This allows eliminating the
  need for dynamically configuring a timeout for testing, and instead
  looking for ref counts.

Bug: 829581
Test: services_unittests --gtest_filter=DeviceFactoryProviderConnectorTest.*
Change-Id: I788775aaf6e77159a9142994d4772200ab42a14d
Reviewed-on: https://chromium-review.googlesource.com/1089921Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569666}

services/service_manager/public/cpp/service_keepalive.cc

@@ -11,9 +11,11 @@
 namespace service_manager {
 
 ServiceKeepalive::ServiceKeepalive(ServiceContext* context,
-                                   base::TimeDelta idle_timeout)
+                                   base::TimeDelta idle_timeout,
+                                   TimeoutObserver* timeout_observer)
     : context_(context),
       idle_timeout_(idle_timeout),
+      timeout_observer_(timeout_observer),
       ref_factory_(base::BindRepeating(&ServiceKeepalive::OnRefCountZero,
                                        base::Unretained(this))),
       weak_ptr_factory_(this) {
@@ -27,12 +29,26 @@ std::unique_ptr<ServiceContextRef> ServiceKeepalive::CreateRef() {
   return ref_factory_.CreateRef();
 }
 
+bool ServiceKeepalive::HasNoRefs() {
+  return ref_factory_.HasNoRefs();
+}
+
 void ServiceKeepalive::OnRefAdded() {
+  if (idle_timer_.IsRunning() && timeout_observer_)
+    timeout_observer_->OnTimeoutCancelled();
   idle_timer_.Stop();
 }
 
 void ServiceKeepalive::OnRefCountZero() {
-  idle_timer_.Start(FROM_HERE, idle_timeout_, context_->CreateQuitClosure());
+  idle_timer_.Start(FROM_HERE, idle_timeout_,
+                    base::BindRepeating(&ServiceKeepalive::OnTimerExpired,
+                                        weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ServiceKeepalive::OnTimerExpired() {
+  if (timeout_observer_)
+    timeout_observer_->OnTimeoutExpired();
+  context_->CreateQuitClosure().Run();
 }
 
 }  // namespace service_manager

services/service_manager/public/cpp/service_keepalive.h

@@ -31,19 +31,32 @@ class ServiceContext;
 // service references to different endpoints in your service.
 class SERVICE_MANAGER_PUBLIC_CPP_EXPORT ServiceKeepalive {
  public:
+  class TimeoutObserver {
+   public:
+    virtual ~TimeoutObserver() {}
+    virtual void OnTimeoutExpired() = 0;
+    virtual void OnTimeoutCancelled() = 0;
+  };
+
   // Creates a keepalive which allows the service to be idle for |idle_timeout|
-  // before requesting termination. Must outlive |context|, which is not owned.
-  ServiceKeepalive(ServiceContext* context, base::TimeDelta idle_timeout);
+  // before requesting termination. Both |context| and |timeout_observer| are
+  // not owned and must outlive the ServiceKeepalive instance.
+  ServiceKeepalive(ServiceContext* context,
+                   base::TimeDelta idle_timeout,
+                   TimeoutObserver* timeout_observer = nullptr);
   ~ServiceKeepalive();
 
   std::unique_ptr<ServiceContextRef> CreateRef();
+  bool HasNoRefs();
 
  private:
   void OnRefAdded();
   void OnRefCountZero();
+  void OnTimerExpired();
 
   ServiceContext* const context_;
   const base::TimeDelta idle_timeout_;
+  TimeoutObserver* const timeout_observer_;
   base::OneShotTimer idle_timer_;
   ServiceContextRefFactory ref_factory_;
   base::WeakPtrFactory<ServiceKeepalive> weak_ptr_factory_;

services/video_capture/device_factory_provider_impl.cc

@@ -92,16 +92,16 @@ class DeviceFactoryProviderImpl::GpuDependenciesContext {
 };
 
 DeviceFactoryProviderImpl::DeviceFactoryProviderImpl(
-    std::unique_ptr<service_manager::ServiceContextRef> service_ref,
-    base::Callback<void(float)> set_shutdown_delay_cb)
-    : service_ref_(std::move(service_ref)),
-      set_shutdown_delay_cb_(std::move(set_shutdown_delay_cb)) {}
+    std::unique_ptr<service_manager::ServiceContextRef> service_ref)
+    : service_ref_(std::move(service_ref)) {}
 
 DeviceFactoryProviderImpl::~DeviceFactoryProviderImpl() {
   factory_bindings_.CloseAllBindings();
   device_factory_.reset();
-  gpu_dependencies_context_->GetTaskRunner()->DeleteSoon(
-      FROM_HERE, std::move(gpu_dependencies_context_));
+  if (gpu_dependencies_context_) {
+    gpu_dependencies_context_->GetTaskRunner()->DeleteSoon(
+        FROM_HERE, std::move(gpu_dependencies_context_));
+  }
 }
 
 void DeviceFactoryProviderImpl::InjectGpuDependencies(
@@ -121,10 +121,6 @@ void DeviceFactoryProviderImpl::ConnectToDeviceFactory(
   factory_bindings_.AddBinding(device_factory_.get(), std::move(request));
 }
 
-void DeviceFactoryProviderImpl::SetShutdownDelayInSeconds(float seconds) {
-  set_shutdown_delay_cb_.Run(seconds);
-}
-
 void DeviceFactoryProviderImpl::LazyInitializeGpuDependenciesContext() {
   if (!gpu_dependencies_context_)
     gpu_dependencies_context_ = std::make_unique<GpuDependenciesContext>();

services/video_capture/device_factory_provider_impl.h

@@ -20,8 +20,7 @@ namespace video_capture {
 class DeviceFactoryProviderImpl : public mojom::DeviceFactoryProvider {
  public:
   DeviceFactoryProviderImpl(
-      std::unique_ptr<service_manager::ServiceContextRef> service_ref,
-      base::Callback<void(float)> set_shutdown_delay_cb);
+      std::unique_ptr<service_manager::ServiceContextRef> service_ref);
   ~DeviceFactoryProviderImpl() override;
 
   // mojom::DeviceFactoryProvider implementation.
@@ -29,7 +28,6 @@ class DeviceFactoryProviderImpl : public mojom::DeviceFactoryProvider {
       ui::mojom::GpuMemoryBufferFactoryPtr memory_buffer_factory,
       mojom::AcceleratorFactoryPtr accelerator_factory) override;
   void ConnectToDeviceFactory(mojom::DeviceFactoryRequest request) override;
-  void SetShutdownDelayInSeconds(float seconds) override;
 
  private:
   class GpuDependenciesContext;
@@ -41,7 +39,6 @@ class DeviceFactoryProviderImpl : public mojom::DeviceFactoryProvider {
   std::unique_ptr<mojom::DeviceFactory> device_factory_;
   const std::unique_ptr<service_manager::ServiceContextRef> service_ref_;
   std::unique_ptr<GpuDependenciesContext> gpu_dependencies_context_;
-  base::Callback<void(float)> set_shutdown_delay_cb_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceFactoryProviderImpl);
 };

services/video_capture/public/mojom/constants.mojom

@@ -5,5 +5,4 @@
 module video_capture.mojom;
 
 const string kServiceName = "video_capture";
-const float kDefaultShutdownDelayInSeconds = 5;
 const int32 kInvalidBufferId = -1;

services/video_capture/public/mojom/device_factory_provider.mojom

@@ -27,11 +27,4 @@ interface DeviceFactoryProvider  {
                         AcceleratorFactory accelerator_factory);
 
   ConnectToDeviceFactory(DeviceFactory& request);
-
-  // The service shuts down after a certain delay when no client is connected.
-  // This method allows clients to customize this delay.
-  // An example use case for a custom delay is integration tests that want to
-  // minimize wait time.
-  // The default value is found in constants.mojom.
-  SetShutdownDelayInSeconds(float seconds);
 };

services/video_capture/service_impl.cc

@@ -13,12 +13,14 @@
 
 namespace video_capture {
 
-ServiceImpl::ServiceImpl()
-    : shutdown_delay_in_seconds_(mojom::kDefaultShutdownDelayInSeconds),
+ServiceImpl::ServiceImpl(float shutdown_delay_in_seconds)
+    : shutdown_delay_in_seconds_(shutdown_delay_in_seconds),
       weak_factory_(this) {}
 
 ServiceImpl::~ServiceImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (destruction_cb_)
+    std::move(destruction_cb_).Run();
 }
 
 // static
@@ -26,15 +28,48 @@ std::unique_ptr<service_manager::Service> ServiceImpl::Create() {
   return std::make_unique<ServiceImpl>();
 }
 
+void ServiceImpl::SetDestructionObserver(base::OnceClosure observer_cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  destruction_cb_ = std::move(observer_cb);
+}
+
+void ServiceImpl::SetFactoryProviderClientConnectedObserver(
+    base::RepeatingClosure observer_cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  factory_provider_client_connected_cb_ = std::move(observer_cb);
+}
+
+void ServiceImpl::SetFactoryProviderClientDisconnectedObserver(
+    base::RepeatingClosure observer_cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  factory_provider_client_disconnected_cb_ = std::move(observer_cb);
+}
+
+void ServiceImpl::SetShutdownTimeoutCancelledObserver(
+    base::RepeatingClosure observer_cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  shutdown_timeout_cancelled_cb_ = std::move(observer_cb);
+}
+
+bool ServiceImpl::HasNoContextRefs() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return ref_factory_->HasNoRefs();
+}
+
 void ServiceImpl::OnStart() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   video_capture::uma::LogVideoCaptureServiceEvent(
       video_capture::uma::SERVICE_STARTED);
 
-  ref_factory_ = std::make_unique<service_manager::ServiceContextRefFactory>(
-      base::BindRepeating(&ServiceImpl::MaybeRequestQuitDelayed,
-                          weak_factory_.GetWeakPtr()));
+  // Do not replace |ref_factory_| if one has already been set via
+  // SetServiceContextRefProviderForTesting().
+  if (!ref_factory_) {
+    ref_factory_ = std::make_unique<service_manager::ServiceKeepalive>(
+        context(), base::TimeDelta::FromSecondsD(shutdown_delay_in_seconds_),
+        this);
+  }
+
   registry_.AddInterface<mojom::DeviceFactoryProvider>(
       // Unretained |this| is safe because |registry_| is owned by |this|.
       base::Bind(&ServiceImpl::OnDeviceFactoryProviderRequest,
@@ -53,7 +88,6 @@ void ServiceImpl::OnBindInterface(
     const std::string& interface_name,
     mojo::ScopedMessagePipeHandle interface_pipe) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(thread_checker_.CalledOnValidThread());
   registry_.BindInterface(interface_name, std::move(interface_pipe));
 }
 
@@ -62,9 +96,16 @@ bool ServiceImpl::OnServiceManagerConnectionLost() {
   return true;
 }
 
-void ServiceImpl::SetFactoryProviderClientDisconnectedObserver(
-    const base::RepeatingClosure& observer_cb) {
-  factory_provider_client_disconnected_cb_ = observer_cb;
+void ServiceImpl::OnTimeoutExpired() {
+  video_capture::uma::LogVideoCaptureServiceEvent(
+      video_capture::uma::SERVICE_SHUTTING_DOWN_BECAUSE_NO_CLIENT);
+}
+
+void ServiceImpl::OnTimeoutCancelled() {
+  video_capture::uma::LogVideoCaptureServiceEvent(
+      video_capture::uma::SERVICE_SHUTDOWN_TIMEOUT_CANCELED);
+  if (shutdown_timeout_cancelled_cb_)
+    shutdown_timeout_cancelled_cb_.Run();
 }
 
 void ServiceImpl::OnDeviceFactoryProviderRequest(
@@ -73,6 +114,10 @@ void ServiceImpl::OnDeviceFactoryProviderRequest(
   LazyInitializeDeviceFactoryProvider();
   factory_provider_bindings_.AddBinding(device_factory_provider_.get(),
                                         std::move(request));
+
+  if (!factory_provider_client_connected_cb_.is_null()) {
+    factory_provider_client_connected_cb_.Run();
+  }
 }
 
 void ServiceImpl::OnTestingControlsRequest(
@@ -83,43 +128,12 @@ void ServiceImpl::OnTestingControlsRequest(
       std::move(request));
 }
 
-void ServiceImpl::SetShutdownDelayInSeconds(float seconds) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  shutdown_delay_in_seconds_ = seconds;
-}
-
-void ServiceImpl::MaybeRequestQuitDelayed() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&ServiceImpl::MaybeRequestQuit, weak_factory_.GetWeakPtr()),
-      base::TimeDelta::FromSeconds(shutdown_delay_in_seconds_));
-}
-
-void ServiceImpl::MaybeRequestQuit() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(ref_factory_);
-  if (ref_factory_->HasNoRefs()) {
-    video_capture::uma::LogVideoCaptureServiceEvent(
-        video_capture::uma::SERVICE_SHUTTING_DOWN_BECAUSE_NO_CLIENT);
-    context()->CreateQuitClosure().Run();
-  } else {
-    video_capture::uma::LogVideoCaptureServiceEvent(
-        video_capture::uma::SERVICE_SHUTDOWN_TIMEOUT_CANCELED);
-  }
-}
-
 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)));
+  device_factory_provider_ =
+      std::make_unique<DeviceFactoryProviderImpl>(ref_factory_->CreateRef());
 }
 
 void ServiceImpl::OnProviderClientDisconnected() {

services/video_capture/service_impl.h

@@ -11,7 +11,7 @@
 #include "mojo/public/cpp/bindings/binding_set.h"
 #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/service_manager/public/cpp/service_keepalive.h"
 #include "services/video_capture/device_factory_provider_impl.h"
 #include "services/video_capture/public/mojom/device_factory_provider.mojom.h"
 #include "services/video_capture/public/mojom/testing_controls.mojom.h"
@@ -22,13 +22,22 @@
 
 namespace video_capture {
 
-class ServiceImpl : public service_manager::Service {
+class ServiceImpl : public service_manager::Service,
+                    public service_manager::ServiceKeepalive::TimeoutObserver {
  public:
-  ServiceImpl();
+  ServiceImpl(float shutdown_delay_in_seconds = 5.0f);
   ~ServiceImpl() override;
 
   static std::unique_ptr<service_manager::Service> Create();
 
+  void SetDestructionObserver(base::OnceClosure observer_cb);
+  void SetFactoryProviderClientConnectedObserver(
+      base::RepeatingClosure observer_cb);
+  void SetFactoryProviderClientDisconnectedObserver(
+      base::RepeatingClosure observer_cb);
+  void SetShutdownTimeoutCancelledObserver(base::RepeatingClosure observer_cb);
+  bool HasNoContextRefs();
+
   // service_manager::Service implementation.
   void OnStart() override;
   void OnBindInterface(const service_manager::BindSourceInfo& source_info,
@@ -36,31 +45,35 @@ class ServiceImpl : public service_manager::Service {
                        mojo::ScopedMessagePipeHandle interface_pipe) override;
   bool OnServiceManagerConnectionLost() override;
 
-  void SetFactoryProviderClientDisconnectedObserver(
-      const base::RepeatingClosure& observer_cb);
+  // service_manager::ServiceKeepalive::TimeoutObserver implementation.
+  void OnTimeoutExpired() override;
+  void OnTimeoutCancelled() override;
 
  private:
   void OnDeviceFactoryProviderRequest(
       mojom::DeviceFactoryProviderRequest request);
   void OnTestingControlsRequest(mojom::TestingControlsRequest request);
-  void SetShutdownDelayInSeconds(float seconds);
   void MaybeRequestQuitDelayed();
   void MaybeRequestQuit();
   void LazyInitializeDeviceFactoryProvider();
   void OnProviderClientDisconnected();
 
+  const float shutdown_delay_in_seconds_;
 #if defined(OS_WIN)
   // COM must be initialized in order to access the video capture devices.
   base::win::ScopedCOMInitializer com_initializer_;
 #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.
+  std::unique_ptr<service_manager::ServiceKeepalive> ref_factory_;
+
+  // Callbacks that can optionally be set by clients.
+  base::OnceClosure destruction_cb_;
+  base::RepeatingClosure factory_provider_client_connected_cb_;
   base::RepeatingClosure factory_provider_client_disconnected_cb_;
+  base::RepeatingClosure shutdown_timeout_cancelled_cb_;
+
   base::ThreadChecker thread_checker_;
   base::WeakPtrFactory<ServiceImpl> weak_factory_;
 

services/video_capture/test/device_factory_provider_connectortest.cc

@@ -11,6 +11,7 @@
 #include "services/video_capture/public/mojom/constants.mojom.h"
 #include "services/video_capture/public/mojom/device_factory_provider.mojom.h"
 #include "services/video_capture/service_impl.h"
+#include "services/video_capture/test/mock_receiver.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace video_capture {
@@ -31,47 +32,173 @@ class DeviceFactoryProviderConnectorTest : public ::testing::Test {
   void SetUp() override {
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
         switches::kUseFakeDeviceForMediaStream);
-    std::unique_ptr<ServiceImpl> service_impl = std::make_unique<ServiceImpl>();
+    // We need to set the shutdown delay to at least some epsilon > 0 in order
+    // to avoid the service shutting down synchronously which would prevent
+    // test case ServiceIncreasesRefCountOnNewConnectionAfterDisconnect from
+    // being able to reconnect before the timer expires.
+    static const float kShutdownDelayInSeconds = 0.0001f;
+    std::unique_ptr<ServiceImpl> service_impl =
+        std::make_unique<ServiceImpl>(kShutdownDelayInSeconds);
+    service_impl->SetDestructionObserver(base::BindOnce(
+        [](base::RunLoop* service_destroyed_wait_loop) {
+          service_destroyed_wait_loop->Quit();
+        },
+        &service_destroyed_wait_loop_));
     service_impl_ = service_impl.get();
     connector_factory_ =
         service_manager::TestConnectorFactory::CreateForUniqueService(
-            std::move(service_impl));
+            std::move(service_impl), true /*release_service_on_quit_request*/);
     connector_ = connector_factory_->CreateConnector();
-    connector_->BindInterface(mojom::kServiceName, &factory_provider_);
-    factory_provider_->SetShutdownDelayInSeconds(0.0f);
-    factory_provider_->ConnectToDeviceFactory(mojo::MakeRequest(&factory_));
+    {
+      base::RunLoop wait_loop;
+      service_impl_->SetFactoryProviderClientConnectedObserver(
+          wait_loop.QuitClosure());
+      connector_->BindInterface(mojom::kServiceName, &factory_provider_);
+      wait_loop.Run();
+    }
+  }
+
+  void TearDown() override {
+    if (factory_provider_.is_bound()) {
+      factory_provider_.reset();
+      service_destroyed_wait_loop_.Run();
+    }
   }
 
  protected:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   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_;
+  base::RunLoop service_destroyed_wait_loop_;
 
  private:
-  base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<service_manager::TestConnectorFactory> connector_factory_;
 };
 
-TEST_F(DeviceFactoryProviderConnectorTest, ServiceNotQuitWhenClientConnected) {
-  base::RunLoop wait_loop;
+// Tests that the service does not quit when a client connects
+// while a second client stays connected.
+TEST_F(DeviceFactoryProviderConnectorTest,
+       ServiceDoesNotQuitWhenOneOfTwoClientsDisconnects) {
+  // Establish second connection
+  mojom::DeviceFactoryProviderPtr second_connection;
+  {
+    base::RunLoop wait_loop;
+    service_impl_->SetFactoryProviderClientConnectedObserver(
+        wait_loop.QuitClosure());
+    connector_->BindInterface(mojom::kServiceName, &second_connection);
+    wait_loop.Run();
+  }
+
+  // Release first connection
+  {
+    base::RunLoop wait_loop;
+    service_impl_->SetFactoryProviderClientDisconnectedObserver(
+        wait_loop.QuitClosure());
+    factory_provider_.reset();
+    wait_loop.Run();
+  }
+
+  EXPECT_FALSE(service_impl_->HasNoContextRefs());
+  EXPECT_FALSE(factory_provider_.is_bound());
+  EXPECT_TRUE(second_connection.is_bound());
+}
+
+// Tests that the service quits when the only client disconnects after not
+// having done anything other than obtaining a connection to the fake device
+// factory.
+TEST_F(DeviceFactoryProviderConnectorTest,
+       ServiceQuitsWhenSingleClientDisconnected) {
+  mojom::DeviceFactoryPtr factory;
+  factory_provider_->ConnectToDeviceFactory(mojo::MakeRequest(&factory));
+  factory.reset();
+  factory_provider_.reset();
+
+  service_destroyed_wait_loop_.Run();
+}
 
-  mojom::DeviceFactoryProviderPtr leftover_provider_;
-  connector_->BindInterface(mojom::kServiceName, &leftover_provider_);
-  service_impl_->SetFactoryProviderClientDisconnectedObserver(
-      wait_loop.QuitClosure());
+// Tests that the service quits when both of two clients disconnect.
+TEST_F(DeviceFactoryProviderConnectorTest,
+       ServiceQuitsWhenAllClientsDisconnected) {
+  // Bind another client to the DeviceFactoryProvider interface.
+  mojom::DeviceFactoryProviderPtr second_connection;
+  connector_->BindInterface(mojom::kServiceName, &second_connection);
 
-  factory_.reset();
+  mojom::DeviceFactoryPtr factory;
+  factory_provider_->ConnectToDeviceFactory(mojo::MakeRequest(&factory));
+  factory.reset();
   factory_provider_.reset();
+  second_connection.reset();
 
+  service_destroyed_wait_loop_.Run();
+}
+
+// Tests that the service increase the context ref count when a new connection
+// comes in after all previous connections have been released.
+TEST_F(DeviceFactoryProviderConnectorTest,
+       ServiceIncreasesRefCountOnNewConnectionAfterDisconnect) {
+  base::RunLoop wait_loop;
+  service_impl_->SetShutdownTimeoutCancelledObserver(base::BindRepeating(
+      [](base::RunLoop* wait_loop) { wait_loop->Quit(); }, &wait_loop));
+  // Disconnect and reconnect
+  factory_provider_.reset();
+  connector_->BindInterface(mojom::kServiceName, &factory_provider_);
   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());
+  EXPECT_FALSE(service_impl_->HasNoContextRefs());
+}
+
+// Tests that the service quits when the last client disconnects while using a
+// device.
+TEST_F(DeviceFactoryProviderConnectorTest,
+       ServiceQuitsWhenClientDisconnectsWhileUsingDevice) {
+  mojom::DeviceFactoryPtr factory;
+  factory_provider_->ConnectToDeviceFactory(mojo::MakeRequest(&factory));
+
+  // Connect to and start first device (in this case a fake camera).
+  base::MockCallback<mojom::DeviceFactory::GetDeviceInfosCallback>
+      device_info_receiver_;
+  media::VideoCaptureDeviceInfo fake_device_info;
+  {
+    base::RunLoop wait_loop;
+    EXPECT_CALL(device_info_receiver_, Run(_))
+        .WillOnce(Invoke(
+            [&fake_device_info, &wait_loop](
+                const std::vector<media::VideoCaptureDeviceInfo>& infos) {
+              fake_device_info = infos[0];
+              wait_loop.Quit();
+            }));
+    factory->GetDeviceInfos(device_info_receiver_.Get());
+    wait_loop.Run();
+  }
+  mojom::DevicePtr fake_device;
+  factory->CreateDevice(
+      std::move(fake_device_info.descriptor.device_id),
+      mojo::MakeRequest(&fake_device),
+      base::BindOnce([](mojom::DeviceAccessResultCode result_code) {
+        ASSERT_EQ(mojom::DeviceAccessResultCode::SUCCESS, result_code);
+      }));
+  media::VideoCaptureParams requestable_settings;
+  requestable_settings.requested_format = fake_device_info.supported_formats[0];
+  mojom::ReceiverPtr receiver_proxy;
+  MockReceiver mock_receiver(mojo::MakeRequest(&receiver_proxy));
+  fake_device->Start(requestable_settings, std::move(receiver_proxy));
+  {
+    base::RunLoop wait_loop;
+    EXPECT_CALL(mock_receiver, OnStarted()).WillOnce([&wait_loop]() {
+      wait_loop.Quit();
+    });
+    wait_loop.Run();
+  }
+
+  // Disconnect
+  fake_device.reset();
+  factory.reset();
+  factory_provider_.reset();
+
+  service_destroyed_wait_loop_.Run();
 }
 
 }  // namespace video_capture

services/video_capture/test/device_factory_provider_test.cc

@@ -12,13 +12,6 @@
 
 namespace video_capture {
 
-ServiceManagerListenerImpl::ServiceManagerListenerImpl(
-    service_manager::mojom::ServiceManagerListenerRequest request,
-    base::RunLoop* loop)
-    : binding_(this, std::move(request)), loop_(loop) {}
-
-ServiceManagerListenerImpl::~ServiceManagerListenerImpl() = default;
-
 DeviceFactoryProviderTest::DeviceFactoryProviderTest()
     : service_manager::test::ServiceTest("video_capture_unittests") {}
 
@@ -30,18 +23,7 @@ void DeviceFactoryProviderTest::SetUp() {
 
   service_manager::test::ServiceTest::SetUp();
 
-  service_manager::mojom::ServiceManagerPtr service_manager;
-  connector()->BindInterface(service_manager::mojom::kServiceName,
-                             &service_manager);
-  service_manager::mojom::ServiceManagerListenerPtr listener;
-  base::RunLoop loop;
-  service_state_observer_ = std::make_unique<ServiceManagerListenerImpl>(
-      mojo::MakeRequest(&listener), &loop);
-  service_manager->AddListener(std::move(listener));
-  loop.Run();
-
   connector()->BindInterface(mojom::kServiceName, &factory_provider_);
-  factory_provider_->SetShutdownDelayInSeconds(0.0f);
   factory_provider_->ConnectToDeviceFactory(mojo::MakeRequest(&factory_));
 }
 

services/video_capture/test/device_factory_provider_test.h

@@ -5,46 +5,12 @@
 #ifndef SERVICES_VIDEO_CAPTURE_VIDEO_CAPTURE_TEST_DEVICE_FACTORY_PROVIDER_TEST_H_
 #define SERVICES_VIDEO_CAPTURE_VIDEO_CAPTURE_TEST_DEVICE_FACTORY_PROVIDER_TEST_H_
 
-#include "base/run_loop.h"
 #include "base/test/mock_callback.h"
-#include "mojo/public/cpp/bindings/binding.h"
 #include "services/service_manager/public/cpp/service_test.h"
-#include "services/service_manager/public/mojom/service_manager.mojom.h"
 #include "services/video_capture/public/mojom/device_factory_provider.mojom.h"
-#include "testing/gmock/include/gmock/gmock.h"
 
 namespace video_capture {
 
-class ServiceManagerListenerImpl
-    : public service_manager::mojom::ServiceManagerListener {
- public:
-  ServiceManagerListenerImpl(
-      service_manager::mojom::ServiceManagerListenerRequest request,
-      base::RunLoop* loop);
-  ~ServiceManagerListenerImpl() override;
-
-  // mojom::ServiceManagerListener implementation.
-  void OnInit(std::vector<service_manager::mojom::RunningServiceInfoPtr>
-                  instances) override {
-    loop_->Quit();
-  }
-  void OnServiceCreated(
-      service_manager::mojom::RunningServiceInfoPtr instance) override {}
-  void OnServiceStarted(const service_manager::Identity& identity,
-                        uint32_t pid) override {}
-  void OnServiceFailedToStart(
-      const service_manager::Identity& identity) override {}
-  void OnServicePIDReceived(const service_manager::Identity& identity,
-                            uint32_t pid) override {}
-
-  MOCK_METHOD1(OnServiceStopped,
-               void(const service_manager::Identity& identity));
-
- private:
-  mojo::Binding<service_manager::mojom::ServiceManagerListener> binding_;
-  base::RunLoop* loop_;
-};
-
 // Basic test fixture that sets up a connection to the fake device factory.
 class DeviceFactoryProviderTest : public service_manager::test::ServiceTest {
  public:
@@ -58,7 +24,6 @@ class DeviceFactoryProviderTest : public service_manager::test::ServiceTest {
   mojom::DeviceFactoryPtr factory_;
   base::MockCallback<mojom::DeviceFactory::GetDeviceInfosCallback>
       device_info_receiver_;
-  std::unique_ptr<ServiceManagerListenerImpl> service_state_observer_;
 };
 
 }  // namespace video_capture

services/video_capture/test/device_factory_provider_unittest.cc

@@ -131,47 +131,4 @@ TEST_F(VideoCaptureServiceDeviceFactoryProviderTest,
   wait_loop.Run();
 }
 
-// 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,
-       ServiceQuitsWhenSingleClientDisconnected) {
-  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();
-
-  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

services/video_capture/test/fake_device_unittest.cc

@@ -76,26 +76,4 @@ TEST_F(FakeVideoCaptureDeviceTest, BuffersGetReused) {
   ASSERT_LE(num_buffers_created, kMaxBufferPoolBuffers);
 }
 
-// Tests that the service requests to be closed when the last client disconnects
-// while using a device.
-TEST_F(FakeVideoCaptureDeviceTest,
-       ServiceQuitsWhenClientDisconnectsWhileUsingDevice) {
-  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();
-      }));
-
-  mojom::ReceiverPtr receiver_proxy;
-  MockReceiver receiver(mojo::MakeRequest(&receiver_proxy));
-  fake_device_proxy_->Start(requestable_settings_, std::move(receiver_proxy));
-
-  fake_device_proxy_.reset();
-  factory_.reset();
-  factory_provider_.reset();
-
-  wait_loop.Run();
-}
-
 }  // namespace video_capture