media: Use DeferredDestroyStrongBindingSet in MediaService

This will prevent decoding error caused by destroyed MediaService during
page navigation. See Bug for more details.

Bug: 821171
Change-Id: I515fe3731ced2c38c6ccbb364d05a6122c9a4461
Reviewed-on: https://chromium-review.googlesource.com/1018821
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555685}

media/mojo/services/interface_factory_impl.cc

@@ -58,6 +58,8 @@ InterfaceFactoryImpl::InterfaceFactoryImpl(
       mojo_media_client_(mojo_media_client) {
   DVLOG(1) << __func__;
   DCHECK(mojo_media_client_);
+
+  SetBindingConnectionErrorHandler();
 }
 
 InterfaceFactoryImpl::~InterfaceFactoryImpl() {
@@ -187,6 +189,77 @@ void InterfaceFactoryImpl::CreateCdmProxy(const std::string& cdm_guid,
 #endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
 }
 
+void InterfaceFactoryImpl::OnDestroyPending(base::OnceClosure destroy_cb) {
+  DVLOG(1) << __func__;
+  destroy_cb_ = std::move(destroy_cb);
+  if (IsEmpty())
+    std::move(destroy_cb_).Run();
+  // else the callback will be called when IsEmpty() becomes true.
+}
+
+bool InterfaceFactoryImpl::IsEmpty() {
+#if BUILDFLAG(ENABLE_MOJO_AUDIO_DECODER)
+  if (!audio_decoder_bindings_.empty())
+    return false;
+#endif  // BUILDFLAG(ENABLE_MOJO_AUDIO_DECODER)
+
+#if BUILDFLAG(ENABLE_MOJO_VIDEO_DECODER)
+  if (!video_decoder_bindings_.empty())
+    return false;
+#endif  // BUILDFLAG(ENABLE_MOJO_VIDEO_DECODER)
+
+#if BUILDFLAG(ENABLE_MOJO_RENDERER)
+  if (!renderer_bindings_.empty())
+    return false;
+#endif  // BUILDFLAG(ENABLE_MOJO_RENDERER)
+
+#if BUILDFLAG(ENABLE_MOJO_CDM)
+  if (!cdm_bindings_.empty())
+    return false;
+#endif  // BUILDFLAG(ENABLE_MOJO_CDM)
+
+#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
+  if (!cdm_proxy_bindings_.empty())
+    return false;
+#endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
+
+  return true;
+}
+
+void InterfaceFactoryImpl::SetBindingConnectionErrorHandler() {
+  // base::Unretained is safe because all bindings are owned by |this|. If
+  // |this| is destructed, the bindings will be destructed as well and the
+  // connection error handler should never be called.
+  auto connection_error_cb = base::BindRepeating(
+      &InterfaceFactoryImpl::OnBindingConnectionError, base::Unretained(this));
+
+#if BUILDFLAG(ENABLE_MOJO_AUDIO_DECODER)
+  audio_decoder_bindings_.set_connection_error_handler(connection_error_cb);
+#endif  // BUILDFLAG(ENABLE_MOJO_AUDIO_DECODER)
+
+#if BUILDFLAG(ENABLE_MOJO_VIDEO_DECODER)
+  video_decoder_bindings_.set_connection_error_handler(connection_error_cb);
+#endif  // BUILDFLAG(ENABLE_MOJO_VIDEO_DECODER)
+
+#if BUILDFLAG(ENABLE_MOJO_RENDERER)
+  renderer_bindings_.set_connection_error_handler(connection_error_cb);
+#endif  // BUILDFLAG(ENABLE_MOJO_RENDERER)
+
+#if BUILDFLAG(ENABLE_MOJO_CDM)
+  cdm_bindings_.set_connection_error_handler(connection_error_cb);
+#endif  // BUILDFLAG(ENABLE_MOJO_CDM)
+
+#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
+  cdm_proxy_bindings_.set_connection_error_handler(connection_error_cb);
+#endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
+}
+
+void InterfaceFactoryImpl::OnBindingConnectionError() {
+  DVLOG(2) << __func__;
+  if (destroy_cb_ && IsEmpty())
+    std::move(destroy_cb_).Run();
+}
+
 #if BUILDFLAG(ENABLE_MOJO_RENDERER)
 RendererFactory* InterfaceFactoryImpl::GetRendererFactory() {
   if (!renderer_factory_) {

media/mojo/services/interface_factory_impl.h

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "media/mojo/buildflags.h"
 #include "media/mojo/interfaces/interface_factory.mojom.h"
+#include "media/mojo/services/deferred_destroy_strong_binding_set.h"
 #include "media/mojo/services/mojo_cdm_service_context.h"
 #include "mojo/public/cpp/bindings/strong_binding_set.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -22,7 +23,7 @@ class MediaLog;
 class MojoMediaClient;
 class RendererFactory;
 
-class InterfaceFactoryImpl : public mojom::InterfaceFactory {
+class InterfaceFactoryImpl : public DeferredDestroy<mojom::InterfaceFactory> {
  public:
   InterfaceFactoryImpl(
       service_manager::mojom::InterfaceProviderPtr interfaces,
@@ -42,7 +43,17 @@ class InterfaceFactoryImpl : public mojom::InterfaceFactory {
   void CreateCdmProxy(const std::string& cdm_guid,
                       mojom::CdmProxyRequest request) final;
 
+  // DeferredDestroy<mojom::InterfaceFactory> implemenation.
+  void OnDestroyPending(base::OnceClosure destroy_cb) final;
+
  private:
+  // Returns true when there is no media component (audio/video decoder,
+  // renderer, cdm and cdm proxy) bindings exist.
+  bool IsEmpty();
+
+  void SetBindingConnectionErrorHandler();
+  void OnBindingConnectionError();
+
 #if BUILDFLAG(ENABLE_MOJO_RENDERER)
   RendererFactory* GetRendererFactory();
 #endif  // BUILDFLAG(ENABLE_MOJO_RENDERER)
@@ -82,6 +93,7 @@ class InterfaceFactoryImpl : public mojom::InterfaceFactory {
 
   std::unique_ptr<service_manager::ServiceContextRef> connection_ref_;
   MojoMediaClient* mojo_media_client_;
+  base::OnceClosure destroy_cb_;
 
   DISALLOW_COPY_AND_ASSIGN(InterfaceFactoryImpl);
 };

media/mojo/services/media_service.h

@@ -11,9 +11,9 @@
 #include "media/base/media_log.h"
 #include "media/mojo/interfaces/interface_factory.mojom.h"
 #include "media/mojo/interfaces/media_service.mojom.h"
+#include "media/mojo/services/deferred_destroy_strong_binding_set.h"
 #include "media/mojo/services/media_mojo_export.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
-#include "mojo/public/cpp/bindings/strong_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.h"
@@ -56,7 +56,8 @@ class MEDIA_MOJO_EXPORT MediaService : public service_manager::Service,
 
   // Note: Since |&media_log_| is passed to bindings, the bindings must be
   // destructed first.
-  mojo::StrongBindingSet<mojom::InterfaceFactory> interface_factory_bindings_;
+  DeferredDestroyStrongBindingSet<mojom::InterfaceFactory>
+      interface_factory_bindings_;
 
   service_manager::BinderRegistry registry_;
   mojo::BindingSet<mojom::MediaService> bindings_;

media/mojo/services/media_service_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/task_scheduler/post_task.h"
 #include "build/build_config.h"
 #include "media/base/cdm_config.h"
 #include "media/base/mock_filters.h"
@@ -46,6 +47,7 @@ using testing::Invoke;
 using testing::InvokeWithoutArgs;
 using testing::NiceMock;
 using testing::StrictMock;
+using testing::WithoutArgs;
 
 MATCHER_P(MatchesResult, success, "") {
   return arg->success == success;
@@ -96,6 +98,15 @@ class MockRendererClient : public mojom::RendererClient {
   DISALLOW_COPY_AND_ASSIGN(MockRendererClient);
 };
 
+ACTION_P(QuitLoop, run_loop) {
+  base::PostTask(FROM_HERE, run_loop->QuitClosure());
+}
+
+// Tests MediaService built into a standalone mojo service binary (see
+// ServiceMain() in main.cc) where MediaService uses TestMojoMediaClient.
+// TestMojoMediaClient supports CDM creation using DefaultCdmFactory (only
+// supports Clear Key key system), and Renderer creation using
+// DefaultRendererFactory that always create media::RendererImpl.
 class MediaServiceTest : public service_manager::test::ServiceTest {
  public:
   MediaServiceTest()
@@ -116,27 +127,31 @@ class MediaServiceTest : public service_manager::test::ServiceTest {
         mojo::MakeRequest(&interfaces));
     media_service->CreateInterfaceFactory(
         mojo::MakeRequest(&interface_factory_), std::move(interfaces));
-
-    run_loop_.reset(new base::RunLoop());
   }
 
   MOCK_METHOD3(OnCdmInitialized,
                void(mojom::CdmPromiseResultPtr result,
                     int cdm_id,
                     mojom::DecryptorPtr decryptor));
+  MOCK_METHOD0(OnCdmConnectionError, void());
 
   void InitializeCdm(const std::string& key_system,
                      bool expected_result,
                      int cdm_id) {
+    base::RunLoop run_loop;
     interface_factory_->CreateCdm(key_system, mojo::MakeRequest(&cdm_));
+    cdm_.set_connection_error_handler(base::BindRepeating(
+        &MediaServiceTest::OnCdmConnectionError, base::Unretained(this)));
 
+    // Have to use WithoutArgs since move-only types do not work with actions.
     EXPECT_CALL(*this,
                 OnCdmInitialized(MatchesResult(expected_result), cdm_id, _))
-        .WillOnce(InvokeWithoutArgs(run_loop_.get(), &base::RunLoop::Quit));
+        .WillOnce(WithoutArgs(QuitLoop(&run_loop)));
     cdm_->Initialize(key_system, url::Origin::Create(GURL(kSecurityOrigin)),
                      CdmConfig(),
                      base::BindOnce(&MediaServiceTest::OnCdmInitialized,
                                     base::Unretained(this)));
+    run_loop.Run();
   }
 
   MOCK_METHOD4(OnCdmProxyInitialized,
@@ -146,23 +161,26 @@ class MediaServiceTest : public service_manager::test::ServiceTest {
                     int cdm_id));
 
   void InitializeCdmProxy(const std::string& cdm_guid) {
+    base::RunLoop run_loop;
     interface_factory_->CreateCdmProxy(cdm_guid,
                                        mojo::MakeRequest(&cdm_proxy_));
 
     EXPECT_CALL(*this, OnCdmProxyInitialized(CdmProxy::Status::kOk, _, _, _))
-        .WillOnce(InvokeWithoutArgs(run_loop_.get(), &base::RunLoop::Quit));
+        .WillOnce(QuitLoop(&run_loop));
     mojom::CdmProxyClientAssociatedPtrInfo client_ptr_info;
     cdm_proxy_client_binding_.Bind(mojo::MakeRequest(&client_ptr_info));
     cdm_proxy_->Initialize(
         std::move(client_ptr_info),
         base::BindOnce(&MediaServiceTest::OnCdmProxyInitialized,
                        base::Unretained(this)));
+    run_loop.Run();
   }
 
   MOCK_METHOD1(OnRendererInitialized, void(bool));
 
   void InitializeRenderer(const VideoDecoderConfig& video_config,
                           bool expected_result) {
+    base::RunLoop run_loop;
     interface_factory_->CreateRenderer(
         media::mojom::HostedRendererType::kDefault, std::string(),
         mojo::MakeRequest(&renderer_));
@@ -177,7 +195,7 @@ class MediaServiceTest : public service_manager::test::ServiceTest {
     renderer_client_binding_.Bind(mojo::MakeRequest(&client_ptr_info));
 
     EXPECT_CALL(*this, OnRendererInitialized(expected_result))
-        .WillOnce(InvokeWithoutArgs(run_loop_.get(), &base::RunLoop::Quit));
+        .WillOnce(QuitLoop(&run_loop));
     std::vector<mojom::DemuxerStreamPtrInfo> streams;
     streams.push_back(std::move(video_stream_proxy_info));
     renderer_->Initialize(
@@ -185,13 +203,12 @@ class MediaServiceTest : public service_manager::test::ServiceTest {
         base::nullopt,
         base::BindOnce(&MediaServiceTest::OnRendererInitialized,
                        base::Unretained(this)));
+    run_loop.Run();
   }
 
-  MOCK_METHOD0(ConnectionClosed, void());
+  MOCK_METHOD0(MediaServiceConnectionClosed, void());
 
  protected:
-  std::unique_ptr<base::RunLoop> run_loop_;
-
   mojom::InterfaceFactoryPtr interface_factory_;
   mojom::ContentDecryptionModulePtr cdm_;
   mojom::CdmProxyPtr cdm_proxy_;
@@ -214,54 +231,105 @@ class MediaServiceTest : public service_manager::test::ServiceTest {
 
 // Note: base::RunLoop::RunUntilIdle() does not work well in these tests because
 // even when the loop is idle, we may still have pending events in the pipe.
+// - If you have an InterfacePtr hosted by the service in the service process,
+//   you can use InterfacePtr::FlushForTesting().
+// - If you expect a callback on an InterfacePtr call or connection error, use
+//   base::RunLoop::Run() and QuitLoop().
 
 // TODO(crbug.com/829233): Enable these tests on Android.
 #if BUILDFLAG(ENABLE_MOJO_CDM) && !defined(OS_ANDROID)
 TEST_F(MediaServiceTest, InitializeCdm_Success) {
   InitializeCdm(kClearKeyKeySystem, true, 1);
-  run_loop_->Run();
 }
 
 TEST_F(MediaServiceTest, InitializeCdm_InvalidKeySystem) {
   InitializeCdm(kInvalidKeySystem, false, 0);
-  run_loop_->Run();
 }
 #endif  // BUILDFLAG(ENABLE_MOJO_CDM) && !defined(OS_ANDROID)
 
 #if BUILDFLAG(ENABLE_MOJO_RENDERER)
 TEST_F(MediaServiceTest, InitializeRenderer) {
   InitializeRenderer(TestVideoConfig::Normal(), true);
-  run_loop_->Run();
 }
 #endif  // BUILDFLAG(ENABLE_MOJO_RENDERER)
 
 #if BUILDFLAG(ENABLE_LIBRARY_CDMS)
 TEST_F(MediaServiceTest, CdmProxy) {
   InitializeCdmProxy(kClearKeyCdmGuid);
-  run_loop_->Run();
 }
 #endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
 
 TEST_F(MediaServiceTest, Lifetime) {
   // The lifetime of the media service is controlled by the number of
-  // live InterfaceFactory impls, not MediaService impls, so this pipe should
-  // be closed when the last InterfaceFactory is destroyed.
+  // live InterfaceFactory impls, which are then deferred destroyed until no
+  // media components (CDM or Renderer) are hosted.
   media::mojom::MediaServicePtr media_service;
   connector()->BindInterface(media::mojom::kMediaServiceName, &media_service);
-  media_service.set_connection_error_handler(
-      base::Bind(&MediaServiceTest::ConnectionClosed, base::Unretained(this)));
+  media_service.set_connection_error_handler(base::BindRepeating(
+      &MediaServiceTest::MediaServiceConnectionClosed, base::Unretained(this)));
+
+#if BUILDFLAG(ENABLE_MOJO_CDM) && !defined(OS_ANDROID)
+  InitializeCdm(kClearKeyKeySystem, true, 1);
+#endif
 
-  // Disconnecting CDM and Renderer services doesn't terminate the app.
+#if BUILDFLAG(ENABLE_MOJO_RENDERER)
+  InitializeRenderer(TestVideoConfig::Normal(), true);
+#endif
+
+  // Disconnecting CDM and Renderer services doesn't terminate MediaService
+  // since |interface_factory_| is still alive.
   cdm_.reset();
   renderer_.reset();
+  interface_factory_.FlushForTesting();
 
-  // Disconnecting InterfaceFactory service should terminate the app, which will
-  // close the connection.
-  EXPECT_CALL(*this, ConnectionClosed())
-      .WillOnce(Invoke(run_loop_.get(), &base::RunLoop::Quit));
+  // Disconnecting InterfaceFactory will now terminate the MediaService.
+  base::RunLoop run_loop;
+  EXPECT_CALL(*this, MediaServiceConnectionClosed())
+      .WillOnce(QuitLoop(&run_loop));
   interface_factory_.reset();
+  run_loop.Run();
+}
 
-  run_loop_->Run();
+#if (BUILDFLAG(ENABLE_MOJO_CDM) && !defined(OS_ANDROID)) || \
+    BUILDFLAG(ENABLE_MOJO_RENDERER)
+TEST_F(MediaServiceTest, DeferredDestruction) {
+  // The lifetime of the media service is controlled by the number of
+  // live InterfaceFactory impls, which are then deferred destroyed until no
+  // media components (CDM or Renderer) are hosted.
+  media::mojom::MediaServicePtr media_service;
+  connector()->BindInterface(media::mojom::kMediaServiceName, &media_service);
+  media_service.set_connection_error_handler(base::BindRepeating(
+      &MediaServiceTest::MediaServiceConnectionClosed, base::Unretained(this)));
+
+#if BUILDFLAG(ENABLE_MOJO_CDM) && !defined(OS_ANDROID)
+  InitializeCdm(kClearKeyKeySystem, true, 1);
+#endif
+
+#if BUILDFLAG(ENABLE_MOJO_RENDERER)
+  InitializeRenderer(TestVideoConfig::Normal(), true);
+#endif
+
+  ASSERT_TRUE(cdm_ || renderer_);
+
+  // Disconnecting InterfaceFactory should not terminate the MediaService since
+  // there are still media components (CDM or Renderer) hosted.
+  interface_factory_.reset();
+  if (cdm_)
+    cdm_.FlushForTesting();
+  else if (renderer_)
+    renderer_.FlushForTesting();
+  else
+    NOTREACHED();
+
+  // Disconnecting CDM and Renderer will now terminate the MediaService.
+  base::RunLoop run_loop;
+  cdm_.reset();
+  renderer_.reset();
+  EXPECT_CALL(*this, MediaServiceConnectionClosed())
+      .WillOnce(QuitLoop(&run_loop));
+  run_loop.Run();
 }
+#endif  // (BUILDFLAG(ENABLE_MOJO_CDM) && !defined(OS_ANDROID)) ||
+        //  BUILDFLAG(ENABLE_MOJO_RENDERER)
 
 }  // namespace media