media: Add test to cover delayed CdmService release

Also refator CdmService so that we can override service release delay for
testing.

Bug: 826039
Change-Id: I595a26c019ee760c49a2bac638c4f928101f6423
Reviewed-on: https://chromium-review.googlesource.com/1011404Reviewed-by: John Rummell <jrummell@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550640}

media/mojo/services/cdm_service.cc

@@ -22,44 +22,49 @@ namespace media {
 
 namespace {
 
+using service_manager::ServiceContextRef;
+
 constexpr base::TimeDelta kServiceContextRefReleaseDelay =
     base::TimeDelta::FromSeconds(5);
 
-void DeleteServiceContextRef(service_manager::ServiceContextRef* ref) {
+void DeleteServiceContextRef(ServiceContextRef* ref) {
   delete ref;
 }
 
 // Starting a new process and loading the library CDM could be expensive. This
-// class helps delay the release of service_manager::ServiceContextRef by
+// class helps delay the release of ServiceContextRef by
 // |kServiceContextRefReleaseDelay|, which will ultimately delay CdmService
 // destruction by the same delay as well. This helps reduce the chance of
 // destroying the CdmService and immediately creates it (in another process) in
 // cases like navigation, which could cause long service connection delays.
-class DelayedReleaseServiceContextRef
-    : public service_manager::ServiceContextRef {
+class DelayedReleaseServiceContextRef : public ServiceContextRef {
  public:
-  explicit DelayedReleaseServiceContextRef(
-      std::unique_ptr<service_manager::ServiceContextRef> ref)
+  DelayedReleaseServiceContextRef(std::unique_ptr<ServiceContextRef> ref,
+                                  base::TimeDelta delay)
       : ref_(std::move(ref)),
-        task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
+        delay_(delay),
+        task_runner_(base::ThreadTaskRunnerHandle::Get()) {
+    DCHECK_GT(delay_, base::TimeDelta());
+  }
 
   ~DelayedReleaseServiceContextRef() override {
     service_manager::ServiceContextRef* ref_ptr = ref_.release();
     if (!task_runner_->PostNonNestableDelayedTask(
             FROM_HERE, base::BindOnce(&DeleteServiceContextRef, ref_ptr),
-            kServiceContextRefReleaseDelay)) {
+            delay_)) {
       DeleteServiceContextRef(ref_ptr);
     }
   }
 
-  // service_manager::ServiceContextRef implementation.
+  // ServiceContextRef implementation.
   std::unique_ptr<ServiceContextRef> Clone() override {
     NOTIMPLEMENTED();
     return nullptr;
   }
 
  private:
-  std::unique_ptr<service_manager::ServiceContextRef> ref_;
+  std::unique_ptr<ServiceContextRef> ref_;
+  base::TimeDelta delay_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(DelayedReleaseServiceContextRef);
@@ -85,10 +90,9 @@ class DelayedReleaseServiceContextRef
 //     details.
 class CdmFactoryImpl : public DeferredDestroy<mojom::CdmFactory> {
  public:
-  CdmFactoryImpl(
-      CdmService::Client* client,
-      service_manager::mojom::InterfaceProviderPtr interfaces,
-      std::unique_ptr<service_manager::ServiceContextRef> service_context_ref)
+  CdmFactoryImpl(CdmService::Client* client,
+                 service_manager::mojom::InterfaceProviderPtr interfaces,
+                 std::unique_ptr<ServiceContextRef> service_context_ref)
       : client_(client),
         interfaces_(std::move(interfaces)),
         service_context_ref_(std::move(service_context_ref)) {
@@ -147,7 +151,7 @@ class CdmFactoryImpl : public DeferredDestroy<mojom::CdmFactory> {
   CdmService::Client* client_;
   service_manager::mojom::InterfaceProviderPtr interfaces_;
   mojo::StrongBindingSet<mojom::ContentDecryptionModule> cdm_bindings_;
-  std::unique_ptr<service_manager::ServiceContextRef> service_context_ref_;
+  std::unique_ptr<ServiceContextRef> service_context_ref_;
   std::unique_ptr<media::CdmFactory> cdm_factory_;
   base::OnceClosure destroy_cb_;
 
@@ -157,7 +161,8 @@ class CdmFactoryImpl : public DeferredDestroy<mojom::CdmFactory> {
 }  // namespace
 
 CdmService::CdmService(std::unique_ptr<Client> client)
-    : client_(std::move(client)) {
+    : client_(std::move(client)),
+      service_release_delay_(kServiceContextRefReleaseDelay) {
   DVLOG(1) << __func__;
   DCHECK(client_);
   registry_.AddInterface<mojom::CdmService>(
@@ -260,10 +265,10 @@ void CdmService::CreateCdmFactory(
   if (!client_)
     return;
 
-  std::unique_ptr<service_manager::ServiceContextRef> service_context_ref =
-      is_delayed_service_release_enabled
+  std::unique_ptr<ServiceContextRef> service_context_ref =
+      service_release_delay_ > base::TimeDelta()
           ? std::make_unique<DelayedReleaseServiceContextRef>(
-                ref_factory_->CreateRef())
+                ref_factory_->CreateRef(), service_release_delay_)
           : ref_factory_->CreateRef();
 
   cdm_factory_bindings_.AddBinding(

media/mojo/services/cdm_service.h

@@ -53,8 +53,10 @@ class MEDIA_MOJO_EXPORT CdmService : public service_manager::Service,
   explicit CdmService(std::unique_ptr<Client> client);
   ~CdmService() final;
 
-  void DisableDelayedServiceReleaseForTesting() {
-    is_delayed_service_release_enabled = false;
+  // By default CdmService release is delayed. Overrides the delay with |delay|.
+  // If |delay| is 0, delayed service release will be disabled.
+  void SetServiceReleaseDelayForTesting(base::TimeDelta delay) {
+    service_release_delay_ = delay;
   }
 
   size_t BoundCdmFactorySizeForTesting() const {
@@ -92,7 +94,7 @@ class MEDIA_MOJO_EXPORT CdmService : public service_manager::Service,
   DeferredDestroyStrongBindingSet<mojom::CdmFactory> cdm_factory_bindings_;
   service_manager::BinderRegistry registry_;
   mojo::BindingSet<mojom::CdmService> bindings_;
-  bool is_delayed_service_release_enabled = true;
+  base::TimeDelta service_release_delay_;
 };
 
 }  // namespace media

media/mojo/services/cdm_service_unittest.cc

@@ -87,12 +87,16 @@ class ServiceTestClient : public service_manager::test::ServiceTestClient,
         std::make_unique<CdmService>(std::move(mock_cdm_service_client));
     cdm_service_ = cdm_service.get();
 
-    // Delayed service release involves a posted delayed task which will not
-    // block *.RunUntilIdle() and hence cause a memory leak in the test.
-    cdm_service_->DisableDelayedServiceReleaseForTesting();
+    cdm_service_->SetServiceReleaseDelayForTesting(service_release_delay_);
 
     service_context_ = std::make_unique<service_manager::ServiceContext>(
         std::move(cdm_service), std::move(request));
+    service_context_->SetQuitClosure(base::BindRepeating(
+        &ServiceTestClient::DestroyService, base::Unretained(this)));
+  }
+
+  void SetServiceReleaseDelay(base::TimeDelta delay) {
+    service_release_delay_ = delay;
   }
 
   void DestroyService() { service_context_.reset(); }
@@ -108,6 +112,11 @@ class ServiceTestClient : public service_manager::test::ServiceTestClient,
     service_factory_bindings_.AddBinding(this, std::move(request));
   }
 
+  // Delayed service release involves a posted delayed task which will not
+  // block *.RunUntilIdle() and hence cause a memory leak in the test. So by
+  // default use a zero value delay to disable the delay.
+  base::TimeDelta service_release_delay_;
+
   service_manager::BinderRegistry registry_;
   mojo::BindingSet<service_manager::mojom::ServiceFactory>
       service_factory_bindings_;
@@ -123,15 +132,15 @@ class CdmServiceTest : public service_manager::test::ServiceTest {
   CdmServiceTest() : ServiceTest("cdm_service_unittest") {}
   ~CdmServiceTest() override {}
 
+  MOCK_METHOD0(CdmServiceConnectionClosed, void());
   MOCK_METHOD0(CdmFactoryConnectionClosed, void());
   MOCK_METHOD0(CdmConnectionClosed, void());
 
-  // service_manager::test::ServiceTest:
-  void SetUp() override {
-    ServiceTest::SetUp();
-
+  void Initialize() {
     connector()->BindInterface(media::mojom::kCdmServiceName,
                                &cdm_service_ptr_);
+    cdm_service_ptr_.set_connection_error_handler(base::BindRepeating(
+        &CdmServiceTest::CdmServiceConnectionClosed, base::Unretained(this)));
 
     service_manager::mojom::InterfaceProviderPtr interfaces;
     auto provider = std::make_unique<MediaInterfaceProvider>(
@@ -146,6 +155,11 @@ class CdmServiceTest : public service_manager::test::ServiceTest {
         &CdmServiceTest::CdmFactoryConnectionClosed, base::Unretained(this)));
   }
 
+  void InitializeWithServiceReleaseDelay(base::TimeDelta delay) {
+    service_test_client_->SetServiceReleaseDelay(delay);
+    Initialize();
+  }
+
   // MOCK_METHOD* doesn't support move-only types. Work around this by having
   // an extra method.
   MOCK_METHOD1(OnCdmInitializedInternal, void(bool result));
@@ -169,6 +183,7 @@ class CdmServiceTest : public service_manager::test::ServiceTest {
     run_loop.Run();
   }
 
+  // service_manager::test::ServiceTest implementation.
   std::unique_ptr<service_manager::Service> CreateService() override {
     auto service_test_client = std::make_unique<ServiceTestClient>(this);
     service_test_client_ = service_test_client.get();
@@ -185,13 +200,14 @@ class CdmServiceTest : public service_manager::test::ServiceTest {
 };
 
 TEST_F(CdmServiceTest, LoadCdm) {
-  base::FilePath cdm_path(FILE_PATH_LITERAL("dummy path"));
+  Initialize();
 
   // Even with a dummy path where the CDM cannot be loaded, EnsureSandboxed()
   // should still be called to ensure the process is sandboxed.
   EXPECT_CALL(*service_test_client_->mock_cdm_service_client(),
               EnsureSandboxed());
 
+  base::FilePath cdm_path(FILE_PATH_LITERAL("dummy path"));
 #if defined(OS_MACOSX)
   // Token provider will not be used since the path is a dummy path.
   cdm_service_ptr_->LoadCdm(cdm_path, nullptr);
@@ -203,22 +219,26 @@ TEST_F(CdmServiceTest, LoadCdm) {
 }
 
 TEST_F(CdmServiceTest, InitializeCdm_Success) {
+  Initialize();
   InitializeCdm(kClearKeyKeySystem, true);
 }
 
 TEST_F(CdmServiceTest, InitializeCdm_InvalidKeySystem) {
+  Initialize();
   InitializeCdm(kInvalidKeySystem, false);
 }
 
 TEST_F(CdmServiceTest, DestroyAndRecreateCdm) {
+  Initialize();
   InitializeCdm(kClearKeyKeySystem, true);
   cdm_ptr_.reset();
   InitializeCdm(kClearKeyKeySystem, true);
 }
 
 // CdmFactory connection error will NOT destroy CDMs. Instead, it will only be
-// destroyed after |cdm_| is reset.
+// destroyed after |cdm_ptr_| is reset.
 TEST_F(CdmServiceTest, DestroyCdmFactory) {
+  Initialize();
   auto* service = service_test_client_->cdm_service();
 
   InitializeCdm(kClearKeyKeySystem, true);
@@ -236,13 +256,39 @@ TEST_F(CdmServiceTest, DestroyCdmFactory) {
   EXPECT_EQ(service->UnboundCdmFactorySizeForTesting(), 0u);
 }
 
+// Same as DestroyCdmFactory test, but do not disable delayed service release.
+// TODO(xhwang): Use ScopedTaskEnvironment::MainThreadType::MOCK_TIME and
+// ScopedTaskEnvironment::FastForwardBy() so we don't have to really wait for
+// the delay in the test. But currently FastForwardBy() doesn't support delayed
+// task yet.
+TEST_F(CdmServiceTest, DestroyCdmFactory_DelayedServiceRelease) {
+  constexpr base::TimeDelta kServiceContextRefReleaseDelay =
+      base::TimeDelta::FromSeconds(1);
+  InitializeWithServiceReleaseDelay(kServiceContextRefReleaseDelay);
+
+  InitializeCdm(kClearKeyKeySystem, true);
+  cdm_factory_ptr_.reset();
+  base::RunLoop().RunUntilIdle();
+
+  base::RunLoop run_loop;
+  auto start_time = base::Time::Now();
+  cdm_ptr_.reset();
+  EXPECT_CALL(*this, CdmServiceConnectionClosed())
+      .WillOnce(Invoke(&run_loop, &base::RunLoop::Quit));
+  run_loop.Run();
+  auto time_passed = base::Time::Now() - start_time;
+  EXPECT_GE(time_passed, kServiceContextRefReleaseDelay);
+}
+
 // Destroy service will destroy the CdmFactory and all CDMs.
 TEST_F(CdmServiceTest, DestroyCdmService) {
+  Initialize();
   InitializeCdm(kClearKeyKeySystem, true);
 
   base::RunLoop run_loop;
   // Ideally we should not care about order, and should only quit the loop when
   // both connections are closed.
+  EXPECT_CALL(*this, CdmServiceConnectionClosed());
   EXPECT_CALL(*this, CdmFactoryConnectionClosed());
   EXPECT_CALL(*this, CdmConnectionClosed())
       .WillOnce(Invoke(&run_loop, &base::RunLoop::Quit));