[Video Capture Service] Invoke callback even if service connection is lost

This CL is part of the Mojo Video Capture work.
Design doc for dev work [1].

Before this CL, the following situation could happen:
1. VideoCaptureManager calls ServiceVideoCaptureProvider::GetDeviceInfosAsync(
     callback).
2. ServiceVideoCaptureProvider makes a request to the service and waits for an
   answer
3. The service process crashes (or otherwise loses connection)
=> |callback| is never run

This CL also adds a test for the scenario described above and a fix in the
production code. The approach for the fix is to wrap the callback in a media::ScopedCallbackRunner to makes sure that  it always gets run. This change required moving some of the Callback types to OnceCallback, which is work we
want to do anyway.

Bug: 584797

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing

Change-Id: Id60b6ffd04de4f7118218099cc1ccd437744d5fb
Reviewed-on: https://chromium-review.googlesource.com/575786
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488433}

content/browser/renderer_host/media/in_process_video_capture_provider.cc

@@ -42,20 +42,20 @@ void InProcessVideoCaptureProvider::Uninitialize() {
 }
 
 void InProcessVideoCaptureProvider::GetDeviceInfosAsync(
-    const GetDeviceInfosCallback& result_callback) {
+    GetDeviceInfosCallback result_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!video_capture_system_) {
     std::vector<media::VideoCaptureDeviceInfo> empty_result;
-    result_callback.Run(empty_result);
+    base::ResetAndReturn(&result_callback).Run(empty_result);
     return;
   }
   // Using Unretained() is safe because |this| owns
   // |video_capture_system_| and |result_callback| has ownership of
   // |this|.
   device_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&media::VideoCaptureSystem::GetDeviceInfosAsync,
-                            base::Unretained(video_capture_system_.get()),
-                            result_callback));
+      FROM_HERE, base::BindOnce(&media::VideoCaptureSystem::GetDeviceInfosAsync,
+                                base::Unretained(video_capture_system_.get()),
+                                std::move(result_callback)));
 }
 
 std::unique_ptr<VideoCaptureDeviceLauncher>

content/browser/renderer_host/media/in_process_video_capture_provider.h

@@ -32,8 +32,7 @@ class CONTENT_EXPORT InProcessVideoCaptureProvider
 
   void Uninitialize() override;
 
-  void GetDeviceInfosAsync(
-      const GetDeviceInfosCallback& result_callback) override;
+  void GetDeviceInfosAsync(GetDeviceInfosCallback result_callback) override;
 
   std::unique_ptr<VideoCaptureDeviceLauncher> CreateDeviceLauncher() override;
 

content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc

@@ -286,11 +286,10 @@ class MediaStreamDispatcherHostTest : public testing::Test {
   void SetUp() override {
     stub_video_device_ids_.emplace_back(kRegularVideoDeviceId);
     stub_video_device_ids_.emplace_back(kDepthVideoDeviceId);
-    ON_CALL(*mock_video_capture_provider_, GetDeviceInfosAsync(_))
+    ON_CALL(*mock_video_capture_provider_, DoGetDeviceInfosAsync(_))
         .WillByDefault(Invoke(
-            [this](const base::Callback<void(
-                       const std::vector<media::VideoCaptureDeviceInfo>&)>&
-                       result_callback) {
+            [this](
+                VideoCaptureProvider::GetDeviceInfosCallback& result_callback) {
               std::vector<media::VideoCaptureDeviceInfo> result;
               for (const auto& device_id : stub_video_device_ids_) {
                 media::VideoCaptureDeviceInfo info;
@@ -308,7 +307,7 @@ class MediaStreamDispatcherHostTest : public testing::Test {
                 }
                 result.push_back(info);
               }
-              result_callback.Run(result);
+              base::ResetAndReturn(&result_callback).Run(result);
             }));
 
     base::RunLoop run_loop;

content/browser/renderer_host/media/mock_video_capture_provider.h

@@ -15,11 +15,13 @@ class MockVideoCaptureProvider : public VideoCaptureProvider {
   MockVideoCaptureProvider();
   ~MockVideoCaptureProvider() override;
 
+  void GetDeviceInfosAsync(GetDeviceInfosCallback result_callback) override {
+    DoGetDeviceInfosAsync(result_callback);
+  }
+
   MOCK_METHOD0(Uninitialize, void());
-  MOCK_METHOD1(GetDeviceInfosAsync,
-               void(const base::Callback<
-                    void(const std::vector<media::VideoCaptureDeviceInfo>&)>&
-                        result_callback));
+  MOCK_METHOD1(DoGetDeviceInfosAsync,
+               void(GetDeviceInfosCallback& result_callback));
 
   MOCK_METHOD0(CreateDeviceLauncher,
                std::unique_ptr<VideoCaptureDeviceLauncher>());

content/browser/renderer_host/media/service_video_capture_provider.cc

@@ -7,41 +7,72 @@
 #include "content/browser/renderer_host/media/service_video_capture_device_launcher.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/service_manager_connection.h"
+#include "media/base/scoped_callback_runner.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "services/video_capture/public/interfaces/constants.mojom.h"
 #include "services/video_capture/public/uma/video_capture_service_event.h"
 
+namespace {
+
+class ServiceConnectorImpl
+    : public content::ServiceVideoCaptureProvider::ServiceConnector {
+ public:
+  ServiceConnectorImpl() {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+    connector_ = content::ServiceManagerConnection::GetForProcess()
+                     ->GetConnector()
+                     ->Clone();
+    DETACH_FROM_SEQUENCE(sequence_checker_);
+  }
+
+  void BindFactoryProvider(
+      video_capture::mojom::DeviceFactoryProviderPtr* provider) override {
+    connector_->BindInterface(video_capture::mojom::kServiceName, provider);
+  }
+
+ private:
+  std::unique_ptr<service_manager::Connector> connector_;
+  SEQUENCE_CHECKER(sequence_checker_);
+};
+
+}  // anonymous namespace
+
 namespace content {
 
 ServiceVideoCaptureProvider::ServiceVideoCaptureProvider()
-    : has_created_device_launcher_(false) {
-  sequence_checker_.DetachFromSequence();
-  DCHECK(BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  connector_ =
-      ServiceManagerConnection::GetForProcess()->GetConnector()->Clone();
+    : ServiceVideoCaptureProvider(base::MakeUnique<ServiceConnectorImpl>()) {}
+
+ServiceVideoCaptureProvider::ServiceVideoCaptureProvider(
+    std::unique_ptr<ServiceConnector> service_connector)
+    : service_connector_(std::move(service_connector)),
+      has_created_device_launcher_(false) {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
 ServiceVideoCaptureProvider::~ServiceVideoCaptureProvider() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   UninitializeInternal(ReasonForUninitialize::kShutdown);
 }
 
 void ServiceVideoCaptureProvider::Uninitialize() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   UninitializeInternal(ReasonForUninitialize::kClientRequest);
 }
 
 void ServiceVideoCaptureProvider::GetDeviceInfosAsync(
-    const base::Callback<void(
-        const std::vector<media::VideoCaptureDeviceInfo>&)>& result_callback) {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
+    GetDeviceInfosCallback result_callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   LazyConnectToService();
-  device_factory_->GetDeviceInfos(result_callback);
+  // Use a ScopedCallbackRunner to make sure that |result_callback| gets
+  // invoked with an empty result in case that the service drops the request.
+  device_factory_->GetDeviceInfos(media::ScopedCallbackRunner(
+      std::move(result_callback),
+      std::vector<media::VideoCaptureDeviceInfo>()));
 }
 
 std::unique_ptr<VideoCaptureDeviceLauncher>
 ServiceVideoCaptureProvider::CreateDeviceLauncher() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   LazyConnectToService();
   has_created_device_launcher_ = true;
   return base::MakeUnique<ServiceVideoCaptureDeviceLauncher>(&device_factory_);
@@ -66,8 +97,7 @@ void ServiceVideoCaptureProvider::LazyConnectToService() {
   has_created_device_launcher_ = false;
   time_of_last_connect_ = base::TimeTicks::Now();
 
-  connector_->BindInterface(video_capture::mojom::kServiceName,
-                            &device_factory_provider_);
+  service_connector_->BindFactoryProvider(&device_factory_provider_);
   device_factory_provider_->ConnectToDeviceFactory(
       mojo::MakeRequest(&device_factory_));
   // Unretained |this| is safe, because |this| owns |device_factory_|.
@@ -77,7 +107,7 @@ void ServiceVideoCaptureProvider::LazyConnectToService() {
 }
 
 void ServiceVideoCaptureProvider::OnLostConnectionToDeviceFactory() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // This may indicate that the video capture service has crashed. Uninitialize
   // here, so that a new connection will be established when clients try to
   // reconnect.

content/browser/renderer_host/media/service_video_capture_provider.h

@@ -10,25 +10,29 @@
 #include "services/video_capture/public/interfaces/device_factory.mojom.h"
 #include "services/video_capture/public/interfaces/device_factory_provider.mojom.h"
 
-namespace service_manager {
-class Connector;
-}
-
 namespace content {
 
 // Implementation of VideoCaptureProvider that uses the "video_capture" service.
 class CONTENT_EXPORT ServiceVideoCaptureProvider : public VideoCaptureProvider {
  public:
+  class ServiceConnector {
+   public:
+    virtual ~ServiceConnector() {}
+    virtual void BindFactoryProvider(
+        video_capture::mojom::DeviceFactoryProviderPtr* provider) = 0;
+  };
+
+  // The parameterless constructor creates a default ServiceConnector which
+  // uses the ServiceManager associated with the current process to connect
+  // to the video capture service.
   ServiceVideoCaptureProvider();
+  // Lets clients provide a custom ServiceConnector.
+  explicit ServiceVideoCaptureProvider(
+      std::unique_ptr<ServiceConnector> service_connector);
   ~ServiceVideoCaptureProvider() override;
 
   void Uninitialize() override;
-
-  void GetDeviceInfosAsync(
-      const base::Callback<void(
-          const std::vector<media::VideoCaptureDeviceInfo>&)>& result_callback)
-      override;
-
+  void GetDeviceInfosAsync(GetDeviceInfosCallback result_callback) override;
   std::unique_ptr<VideoCaptureDeviceLauncher> CreateDeviceLauncher() override;
 
  private:
@@ -42,12 +46,12 @@ class CONTENT_EXPORT ServiceVideoCaptureProvider : public VideoCaptureProvider {
   void OnLostConnectionToDeviceFactory();
   void UninitializeInternal(ReasonForUninitialize reason);
 
-  std::unique_ptr<service_manager::Connector> connector_;
+  std::unique_ptr<ServiceConnector> service_connector_;
   // We must hold on to |device_factory_provider_| because it holds the
   // service-side binding for |device_factory_|.
   video_capture::mojom::DeviceFactoryProviderPtr device_factory_provider_;
   video_capture::mojom::DeviceFactoryPtr device_factory_;
-  base::SequenceChecker sequence_checker_;
+  SEQUENCE_CHECKER(sequence_checker_);
 
   bool has_created_device_launcher_;
   base::TimeTicks time_of_last_connect_;

content/browser/renderer_host/media/service_video_capture_provider_unittest.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 "content/browser/renderer_host/media/service_video_capture_provider.h"
+
+#include "base/run_loop.h"
+#include "base/test/mock_callback.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/threading/thread.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "services/video_capture/public/interfaces/device_factory.mojom.h"
+#include "services/video_capture/public/interfaces/device_factory_provider.mojom.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::Mock;
+using testing::Invoke;
+using testing::_;
+
+namespace content {
+
+class MockServiceConnector
+    : public ServiceVideoCaptureProvider::ServiceConnector {
+ public:
+  MockServiceConnector() {}
+
+  MOCK_METHOD1(BindFactoryProvider,
+               void(video_capture::mojom::DeviceFactoryProviderPtr* provider));
+};
+
+class MockDeviceFactoryProvider
+    : public video_capture::mojom::DeviceFactoryProvider {
+ public:
+  void ConnectToDeviceFactory(
+      video_capture::mojom::DeviceFactoryRequest request) override {
+    DoConnectToDeviceFactory(request);
+  }
+
+  MOCK_METHOD1(SetShutdownDelayInSeconds, void(float seconds));
+  MOCK_METHOD1(DoConnectToDeviceFactory,
+               void(video_capture::mojom::DeviceFactoryRequest& request));
+};
+
+class MockDeviceFactory : public video_capture::mojom::DeviceFactory {
+ public:
+  void GetDeviceInfos(GetDeviceInfosCallback callback) override {
+    DoGetDeviceInfos(callback);
+  }
+  void CreateDevice(const std::string& device_id,
+                    video_capture::mojom::DeviceRequest device_request,
+                    CreateDeviceCallback callback) override {
+    DoCreateDevice(device_id, &device_request, callback);
+  }
+
+  MOCK_METHOD1(DoGetDeviceInfos, void(GetDeviceInfosCallback& callback));
+  MOCK_METHOD3(DoCreateDevice,
+               void(const std::string& device_id,
+                    video_capture::mojom::DeviceRequest* device_request,
+                    CreateDeviceCallback& callback));
+};
+
+class ServiceVideoCaptureProviderTest : public testing::Test {
+ public:
+  ServiceVideoCaptureProviderTest()
+      : factory_provider_binding_(&mock_device_factory_provider_),
+        device_factory_binding_(&mock_device_factory_) {}
+  ~ServiceVideoCaptureProviderTest() override {}
+
+ protected:
+  void SetUp() override {
+    auto mock_service_connector = base::MakeUnique<MockServiceConnector>();
+    mock_service_connector_ = mock_service_connector.get();
+    provider_ = base::MakeUnique<ServiceVideoCaptureProvider>(
+        std::move(mock_service_connector));
+  }
+
+  void TearDown() override {}
+
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  MockServiceConnector* mock_service_connector_;
+  MockDeviceFactoryProvider mock_device_factory_provider_;
+  mojo::Binding<video_capture::mojom::DeviceFactoryProvider>
+      factory_provider_binding_;
+  MockDeviceFactory mock_device_factory_;
+  mojo::Binding<video_capture::mojom::DeviceFactory> device_factory_binding_;
+  std::unique_ptr<ServiceVideoCaptureProvider> provider_;
+  base::MockCallback<VideoCaptureProvider::GetDeviceInfosCallback> results_cb_;
+  base::MockCallback<
+      video_capture::mojom::DeviceFactory::GetDeviceInfosCallback>
+      service_cb_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ServiceVideoCaptureProviderTest);
+};
+
+// Tests that if connection to the service is lost during an outstanding call
+// to GetDeviceInfos(), the callback passed into GetDeviceInfos() still gets
+// invoked.
+TEST_F(ServiceVideoCaptureProviderTest,
+       GetDeviceInfosAsyncInvokesCallbackWhenLosingConnection) {
+  base::RunLoop run_loop;
+
+  EXPECT_CALL(*mock_service_connector_, BindFactoryProvider(_))
+      .WillOnce(Invoke([this](video_capture::mojom::DeviceFactoryProviderPtr*
+                                  device_factory_provider) {
+        factory_provider_binding_.Bind(
+            mojo::MakeRequest(device_factory_provider));
+      }));
+  EXPECT_CALL(mock_device_factory_provider_, DoConnectToDeviceFactory(_))
+      .WillOnce(
+          Invoke([this](video_capture::mojom::DeviceFactoryRequest& request) {
+            device_factory_binding_.Bind(std::move(request));
+          }));
+  video_capture::mojom::DeviceFactory::GetDeviceInfosCallback
+      callback_to_be_called_by_service;
+  base::RunLoop wait_for_call_to_arrive_at_service;
+  EXPECT_CALL(mock_device_factory_, DoGetDeviceInfos(_))
+      .WillOnce(Invoke(
+          [&callback_to_be_called_by_service,
+           &wait_for_call_to_arrive_at_service](
+              video_capture::mojom::DeviceFactory::GetDeviceInfosCallback&
+                  callback) {
+            // Hold on to the callback so we can drop it later.
+            callback_to_be_called_by_service = std::move(callback);
+            wait_for_call_to_arrive_at_service.Quit();
+          }));
+  base::RunLoop wait_for_callback_from_service;
+  EXPECT_CALL(results_cb_, Run(_))
+      .WillOnce(Invoke(
+          [&wait_for_callback_from_service](
+              const std::vector<media::VideoCaptureDeviceInfo>& results) {
+            EXPECT_EQ(0u, results.size());
+            wait_for_callback_from_service.Quit();
+          }));
+
+  // Exercise
+  provider_->GetDeviceInfosAsync(results_cb_.Get());
+  wait_for_call_to_arrive_at_service.Run();
+
+  // Simulate that the service goes down by cutting the connections.
+  device_factory_binding_.Close();
+  factory_provider_binding_.Close();
+
+  wait_for_callback_from_service.Run();
+}
+
+}  // namespace content

content/browser/renderer_host/media/video_capture_provider.h

@@ -72,8 +72,8 @@ class LaunchedVideoCaptureDevice
 // MediaStreamType == MEDIA_DEVICE_VIDEO_CAPTURE, i.e. camera devices.
 class CONTENT_EXPORT VideoCaptureProvider {
  public:
-  using GetDeviceInfosCallback =
-      base::Callback<void(const std::vector<media::VideoCaptureDeviceInfo>&)>;
+  using GetDeviceInfosCallback = base::OnceCallback<void(
+      const std::vector<media::VideoCaptureDeviceInfo>&)>;
 
   virtual ~VideoCaptureProvider() {}
 
@@ -81,8 +81,7 @@ class CONTENT_EXPORT VideoCaptureProvider {
 
   // The passed-in |result_callback| must guarantee that the called
   // instance stays alive until |result_callback| is invoked.
-  virtual void GetDeviceInfosAsync(
-      const GetDeviceInfosCallback& result_callback) = 0;
+  virtual void GetDeviceInfosAsync(GetDeviceInfosCallback result_callback) = 0;
 
   virtual std::unique_ptr<VideoCaptureDeviceLauncher>
   CreateDeviceLauncher() = 0;

content/browser/renderer_host/media/video_capture_provider_switcher.cc

@@ -75,8 +75,9 @@ void VideoCaptureProviderSwitcher::Uninitialize() {
 }
 
 void VideoCaptureProviderSwitcher::GetDeviceInfosAsync(
-    const GetDeviceInfosCallback& result_callback) {
-  media_device_capture_provider_->GetDeviceInfosAsync(result_callback);
+    GetDeviceInfosCallback result_callback) {
+  media_device_capture_provider_->GetDeviceInfosAsync(
+      std::move(result_callback));
 }
 
 std::unique_ptr<VideoCaptureDeviceLauncher>

content/browser/renderer_host/media/video_capture_provider_switcher.h

@@ -23,8 +23,7 @@ class CONTENT_EXPORT VideoCaptureProviderSwitcher
 
   void Uninitialize() override;
 
-  void GetDeviceInfosAsync(
-      const GetDeviceInfosCallback& result_callback) override;
+  void GetDeviceInfosAsync(GetDeviceInfosCallback result_callback) override;
 
   std::unique_ptr<VideoCaptureDeviceLauncher> CreateDeviceLauncher() override;
 

content/test/BUILD.gn

@@ -1286,6 +1286,7 @@ test("content_unittests") {
     "../browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc",
     "../browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc",
     "../browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc",
+    "../browser/renderer_host/media/service_video_capture_provider_unittest.cc",
     "../browser/renderer_host/media/video_capture_buffer_pool_unittest.cc",
     "../browser/renderer_host/media/video_capture_controller_unittest.cc",
     "../browser/renderer_host/media/video_capture_manager_unittest.cc",

media/capture/video/video_capture_system.h

@@ -15,7 +15,7 @@ namespace media {
 class CAPTURE_EXPORT VideoCaptureSystem {
  public:
   using DeviceInfoCallback =
-      base::Callback<void(const std::vector<VideoCaptureDeviceInfo>&)>;
+      base::OnceCallback<void(const std::vector<VideoCaptureDeviceInfo>&)>;
 
   virtual ~VideoCaptureSystem() {}
 
@@ -23,8 +23,7 @@ class CAPTURE_EXPORT VideoCaptureSystem {
   // VideoCaptureSystem instance to guarantee that it stays alive during the
   // asynchronous operation. |result_callback| is invoked on the same thread
   // that calls GetDeviceInfosAsync()
-  virtual void GetDeviceInfosAsync(
-      const DeviceInfoCallback& result_callback) = 0;
+  virtual void GetDeviceInfosAsync(DeviceInfoCallback result_callback) = 0;
 
   // Creates a VideoCaptureDevice object. Returns nullptr if something goes
   // wrong.

media/capture/video/video_capture_system_impl.cc

@@ -66,7 +66,7 @@ VideoCaptureSystemImpl::VideoCaptureSystemImpl(
 VideoCaptureSystemImpl::~VideoCaptureSystemImpl() = default;
 
 void VideoCaptureSystemImpl::GetDeviceInfosAsync(
-    const DeviceInfoCallback& result_callback) {
+    DeviceInfoCallback result_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   VideoCaptureDeviceDescriptors descriptors;
   factory_->GetDeviceDescriptors(&descriptors);
@@ -88,7 +88,7 @@ void VideoCaptureSystemImpl::GetDeviceInfosAsync(
   }
 
   devices_info_cache_.swap(new_devices_info_cache);
-  result_callback.Run(devices_info_cache_);
+  base::ResetAndReturn(&result_callback).Run(devices_info_cache_);
 }
 
 // Creates a VideoCaptureDevice object. Returns NULL if something goes wrong.

media/capture/video/video_capture_system_impl.h

@@ -18,7 +18,7 @@ class CAPTURE_EXPORT VideoCaptureSystemImpl : public VideoCaptureSystem {
       std::unique_ptr<VideoCaptureDeviceFactory> factory);
   ~VideoCaptureSystemImpl() override;
 
-  void GetDeviceInfosAsync(const DeviceInfoCallback& result_callback) override;
+  void GetDeviceInfosAsync(DeviceInfoCallback result_callback) override;
   std::unique_ptr<VideoCaptureDevice> CreateDevice(
       const std::string& device_id) override;