media: Add DeferredDestroyStrongBindingSet

This CL adds DeferredDestroyStrongBindingSet such that mojom::Foo impl
will only be destroyed when:
- mojom::Foo connection error happens, AND FooImpl calls the
  DestroyCallback, or
- DeferredDestroyStrongBindingSet is destroyed.

This helps solve tricky lifetime issues where we do not want destroy the
implementation immediately.

For example, currently when connection error on mojom::CdmFactory
happens, CdmFactoryImpl will be destroyed, which will also destroy all
CDMs owned by CdmFactoryImpl. However, this could happen before MojoCdm
is stopped in the render process, causing unexpected behavior. See the
bug for more details.

This CL also fixes the CdmFactoryImpl lifetime issue by using the
DeferredDestroyStrongBindingSet, and let CdmFactoryImpl only call the
DestroyCallback when no outstanding CDMs exist.

For historical context, mojo::StrongBindingSet was originally added
also to solve lifetime issues around media::mojom::InterfaceFactory, the
predecessor of media::mojom::CdmFactory.
See https://codereview.chromium.org/2530613003/.

Bug: 821171
Test: Added new BindingSetTest cases.
Change-Id: I24a660e8b62720f7fd772db1bafb9499cb71428c
Reviewed-on: https://chromium-review.googlesource.com/969655
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548704}

media/mojo/services/BUILD.gn

@@ -12,6 +12,7 @@ import("//testing/test.gni")
 component("services") {
   output_name = "media_mojo_services"
   sources = [
+    "deferred_destroy_strong_binding_set.h",
     "gpu_mojo_media_client.cc",
     "gpu_mojo_media_client.h",
     "interface_factory_impl.cc",
@@ -156,6 +157,7 @@ source_set("unit_tests") {
   testonly = true
 
   sources = [
+    "deferred_destroy_strong_binding_set_unittest.cc",
     "media_metrics_provider_unittest.cc",
     "mojo_audio_input_stream_unittest.cc",
     "mojo_audio_output_stream_provider_unittest.cc",
@@ -173,6 +175,7 @@ source_set("unit_tests") {
     "//media:test_support",
     "//media/mojo:test_support",
     "//mojo/edk",
+    "//mojo/public/interfaces/bindings/tests:test_interfaces",
     "//services/metrics/public/cpp:ukm_builders",
     "//testing/gmock",
     "//testing/gtest",

media/mojo/services/cdm_service.cc

@@ -58,7 +58,25 @@ class DelayedReleaseServiceContextRef {
   DISALLOW_COPY_AND_ASSIGN(DelayedReleaseServiceContextRef);
 };
 
-class CdmFactoryImpl : public mojom::CdmFactory {
+// Implementation of mojom::CdmFactory that creates and hosts MojoCdmServices
+// which then host CDMs created by the media::CdmFactory provided by the
+// CdmService::Client.
+//
+// Lifetime Note:
+// 1. CdmFactoryImpl instances are owned by a DeferredDestroyStrongBindingSet
+//    directly, which is owned by CdmService.
+// 2. Note that CdmFactoryImpl also holds a ServiceContextRef to the CdmService.
+// 3. CdmFactoryImpl is destroyed in any of the following two cases:
+//   - CdmService is destroyed. Because of (2) this should not happen except for
+//     during browser shutdown, when the ServiceContext could be destroyed
+//     directly which will then destroy CdmService, ignoring any outstanding
+//     ServiceContextRefs.
+//   - mojo::CdmFactory connection error happens, AND CdmFactoryImpl doesn't own
+//     any CDMs (|cdm_bindings_| is empty). This is to prevent destroying the
+//     CDMs too early (e.g. during page navigation) which could cause errors
+//     (session closed) on the client side. See https://crbug.com/821171 for
+//     details.
+class CdmFactoryImpl : public DeferredDestroy<mojom::CdmFactory> {
  public:
   CdmFactoryImpl(
       CdmService::Client* client,
@@ -69,6 +87,12 @@ class CdmFactoryImpl : public mojom::CdmFactory {
         connection_ref_(std::make_unique<DelayedReleaseServiceContextRef>(
             std::move(connection_ref))) {
     DVLOG(1) << __func__;
+
+    // base::Unretained is safe because |cdm_bindings_| is owned by |this|. If
+    // |this| is destructed, |cdm_bindings_| will be destructed as well and the
+    // error handler should never be called.
+    cdm_bindings_.set_connection_error_handler(base::BindRepeating(
+        &CdmFactoryImpl::OnBindingConnectionError, base::Unretained(this)));
   }
 
   ~CdmFactoryImpl() final { DVLOG(1) << __func__; }
@@ -87,6 +111,14 @@ class CdmFactoryImpl : public mojom::CdmFactory {
         std::move(request));
   }
 
+  // DeferredDestroy<mojom::CdmFactory> implemenation.
+  void OnDestroyPending(base::OnceClosure destroy_cb) final {
+    destroy_cb_ = std::move(destroy_cb);
+    if (cdm_bindings_.empty())
+      std::move(destroy_cb_).Run();
+    // else the callback will be called when |cdm_bindings_| become empty.
+  }
+
  private:
   media::CdmFactory* GetCdmFactory() {
     if (!cdm_factory_) {
@@ -96,6 +128,11 @@ class CdmFactoryImpl : public mojom::CdmFactory {
     return cdm_factory_.get();
   }
 
+  void OnBindingConnectionError() {
+    if (destroy_cb_ && cdm_bindings_.empty())
+      std::move(destroy_cb_).Run();
+  }
+
   // Must be declared before the bindings below because the bound objects might
   // take a raw pointer of |cdm_service_context_| and assume it's always
   // available.
@@ -106,6 +143,7 @@ class CdmFactoryImpl : public mojom::CdmFactory {
   mojo::StrongBindingSet<mojom::ContentDecryptionModule> cdm_bindings_;
   std::unique_ptr<DelayedReleaseServiceContextRef> connection_ref_;
   std::unique_ptr<media::CdmFactory> cdm_factory_;
+  base::OnceClosure destroy_cb_;
 
   DISALLOW_COPY_AND_ASSIGN(CdmFactoryImpl);
 };

media/mojo/services/cdm_service.h

@@ -11,9 +11,9 @@
 #include "media/media_buildflags.h"
 #include "media/mojo/interfaces/cdm_service.mojom.h"
 #include "media/mojo/interfaces/content_decryption_module.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_ref.h"
@@ -53,6 +53,14 @@ class MEDIA_MOJO_EXPORT CdmService : public service_manager::Service,
   explicit CdmService(std::unique_ptr<Client> client);
   ~CdmService() final;
 
+  size_t BoundCdmFactorySizeForTesting() const {
+    return cdm_factory_bindings_.size();
+  }
+
+  size_t UnboundCdmFactorySizeForTesting() const {
+    return cdm_factory_bindings_.unbound_size();
+  }
+
  private:
   // service_manager::Service implementation.
   void OnStart() final;
@@ -70,7 +78,6 @@ class MEDIA_MOJO_EXPORT CdmService : public service_manager::Service,
 #else
   void LoadCdm(const base::FilePath& cdm_path) final;
 #endif  // defined(OS_MACOSX)
-
   void CreateCdmFactory(
       mojom::CdmFactoryRequest request,
       service_manager::mojom::InterfaceProviderPtr host_interfaces) final;
@@ -78,7 +85,7 @@ class MEDIA_MOJO_EXPORT CdmService : public service_manager::Service,
   std::unique_ptr<service_manager::ServiceContextRefFactory> ref_factory_;
   std::unique_ptr<Client> client_;
   std::unique_ptr<CdmFactory> cdm_factory_;
-  mojo::StrongBindingSet<mojom::CdmFactory> cdm_factory_bindings_;
+  DeferredDestroyStrongBindingSet<mojom::CdmFactory> cdm_factory_bindings_;
   service_manager::BinderRegistry registry_;
   mojo::BindingSet<mojom::CdmService> bindings_;
 };

media/mojo/services/cdm_service_unittest.cc

@@ -82,9 +82,13 @@ class ServiceTestClient : public service_manager::test::ServiceTestClient,
 
     auto mock_cdm_service_client = std::make_unique<MockCdmServiceClient>();
     mock_cdm_service_client_ = mock_cdm_service_client.get();
+
+    auto cdm_service =
+        std::make_unique<CdmService>(std::move(mock_cdm_service_client));
+    cdm_service_ = cdm_service.get();
+
     service_context_ = std::make_unique<service_manager::ServiceContext>(
-        std::make_unique<CdmService>(std::move(mock_cdm_service_client)),
-        std::move(request));
+        std::move(cdm_service), std::move(request));
   }
 
   void DestroyService() { service_context_.reset(); }
@@ -93,6 +97,8 @@ class ServiceTestClient : public service_manager::test::ServiceTestClient,
     return mock_cdm_service_client_;
   }
 
+  CdmService* cdm_service() { return cdm_service_; }
+
  private:
   void Create(service_manager::mojom::ServiceFactoryRequest request) {
     service_factory_bindings_.AddBinding(this, std::move(request));
@@ -102,7 +108,8 @@ class ServiceTestClient : public service_manager::test::ServiceTestClient,
   mojo::BindingSet<service_manager::mojom::ServiceFactory>
       service_factory_bindings_;
   std::unique_ptr<service_manager::ServiceContext> service_context_;
-  MockCdmServiceClient* mock_cdm_service_client_;
+  CdmService* cdm_service_ = nullptr;
+  MockCdmServiceClient* mock_cdm_service_client_ = nullptr;
 };
 
 }  // namespace
@@ -119,18 +126,19 @@ class CdmServiceTest : public service_manager::test::ServiceTest {
   void SetUp() override {
     ServiceTest::SetUp();
 
-    connector()->BindInterface(media::mojom::kCdmServiceName, &cdm_service_);
+    connector()->BindInterface(media::mojom::kCdmServiceName,
+                               &cdm_service_ptr_);
 
     service_manager::mojom::InterfaceProviderPtr interfaces;
     auto provider = std::make_unique<MediaInterfaceProvider>(
         mojo::MakeRequest(&interfaces));
 
-    ASSERT_FALSE(cdm_factory_);
-    cdm_service_->CreateCdmFactory(mojo::MakeRequest(&cdm_factory_),
+    ASSERT_FALSE(cdm_factory_ptr_);
+    cdm_service_ptr_->CreateCdmFactory(mojo::MakeRequest(&cdm_factory_ptr_),
                                        std::move(interfaces));
-    cdm_service_.FlushForTesting();
-    ASSERT_TRUE(cdm_factory_);
-    cdm_factory_.set_connection_error_handler(base::BindRepeating(
+    cdm_service_ptr_.FlushForTesting();
+    ASSERT_TRUE(cdm_factory_ptr_);
+    cdm_factory_ptr_.set_connection_error_handler(base::BindRepeating(
         &CdmServiceTest::CdmFactoryConnectionClosed, base::Unretained(this)));
   }
 
@@ -145,12 +153,12 @@ class CdmServiceTest : public service_manager::test::ServiceTest {
 
   void InitializeCdm(const std::string& key_system, bool expected_result) {
     base::RunLoop run_loop;
-    cdm_factory_->CreateCdm(key_system, mojo::MakeRequest(&cdm_));
-    cdm_.set_connection_error_handler(base::BindRepeating(
+    cdm_factory_ptr_->CreateCdm(key_system, mojo::MakeRequest(&cdm_ptr_));
+    cdm_ptr_.set_connection_error_handler(base::BindRepeating(
         &CdmServiceTest::CdmConnectionClosed, base::Unretained(this)));
     EXPECT_CALL(*this, OnCdmInitializedInternal(expected_result))
         .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
-    cdm_->Initialize(key_system, url::Origin::Create(GURL(kSecurityOrigin)),
+    cdm_ptr_->Initialize(key_system, url::Origin::Create(GURL(kSecurityOrigin)),
                          CdmConfig(),
                          base::BindRepeating(&CdmServiceTest::OnCdmInitialized,
                                              base::Unretained(this)));
@@ -163,9 +171,9 @@ class CdmServiceTest : public service_manager::test::ServiceTest {
     return service_test_client;
   }
 
-  mojom::CdmServicePtr cdm_service_;
-  mojom::CdmFactoryPtr cdm_factory_;
-  mojom::ContentDecryptionModulePtr cdm_;
+  mojom::CdmServicePtr cdm_service_ptr_;
+  mojom::CdmFactoryPtr cdm_factory_ptr_;
+  mojom::ContentDecryptionModulePtr cdm_ptr_;
   ServiceTestClient* service_test_client_;
 
  private:
@@ -182,12 +190,12 @@ TEST_F(CdmServiceTest, LoadCdm) {
 
 #if defined(OS_MACOSX)
   // Token provider will not be used since the path is a dummy path.
-  cdm_service_->LoadCdm(cdm_path, nullptr);
+  cdm_service_ptr_->LoadCdm(cdm_path, nullptr);
 #else
-  cdm_service_->LoadCdm(cdm_path);
+  cdm_service_ptr_->LoadCdm(cdm_path);
 #endif
 
-  cdm_service_.FlushForTesting();
+  cdm_service_ptr_.FlushForTesting();
 }
 
 TEST_F(CdmServiceTest, InitializeCdm_Success) {
@@ -200,19 +208,28 @@ TEST_F(CdmServiceTest, InitializeCdm_InvalidKeySystem) {
 
 TEST_F(CdmServiceTest, DestroyAndRecreateCdm) {
   InitializeCdm(kClearKeyKeySystem, true);
-  cdm_.reset();
+  cdm_ptr_.reset();
   InitializeCdm(kClearKeyKeySystem, true);
 }
 
-// CdmFactory connection error will destroy all CDMs.
+// CdmFactory connection error will NOT destroy CDMs. Instead, it will only be
+// destroyed after |cdm_| is reset.
 TEST_F(CdmServiceTest, DestroyCdmFactory) {
-  InitializeCdm(kClearKeyKeySystem, true);
+  auto* service = service_test_client_->cdm_service();
 
-  base::RunLoop run_loop;
-  EXPECT_CALL(*this, CdmConnectionClosed())
-      .WillOnce(Invoke(&run_loop, &base::RunLoop::Quit));
-  cdm_factory_.reset();
-  run_loop.Run();
+  InitializeCdm(kClearKeyKeySystem, true);
+  EXPECT_EQ(service->BoundCdmFactorySizeForTesting(), 1u);
+  EXPECT_EQ(service->UnboundCdmFactorySizeForTesting(), 0u);
+
+  cdm_factory_ptr_.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(service->BoundCdmFactorySizeForTesting(), 0u);
+  EXPECT_EQ(service->UnboundCdmFactorySizeForTesting(), 1u);
+
+  cdm_ptr_.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(service->BoundCdmFactorySizeForTesting(), 0u);
+  EXPECT_EQ(service->UnboundCdmFactorySizeForTesting(), 0u);
 }
 
 // Destroy service will destroy the CdmFactory and all CDMs.

media/mojo/services/deferred_destroy_strong_binding_set.h

+// Copyright 2018 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.
+
+#ifndef MEDIA_MOJO_SERVICES_DEFERRED_DESTROY_STRONG_BINDING_SET_H_
+#define MEDIA_MOJO_SERVICES_DEFERRED_DESTROY_STRONG_BINDING_SET_H_
+
+#include <stdint.h>
+
+#include <map>
+#include <memory>
+
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"
+#include "media/base/bind_to_current_loop.h"
+#include "mojo/public/cpp/bindings/strong_binding_set.h"
+
+namespace media {
+
+// A class that can be deferred destroyed by its owner. For example, when used
+// in DeferredDestroyStrongBindingSet.
+template <typename Interface>
+class DeferredDestroy : public Interface {
+ public:
+  // Runs the |destroy_cb| to notify that it's okay to destroy |this|. The
+  // callback can be called synchronously. |this| will always be destroyed
+  // asynchronously after running |destroy_cb| to avoid reentrance issues.
+  virtual void OnDestroyPending(base::OnceClosure destroy_cb) = 0;
+};
+
+// Similar to mojo::StrongBindingSet, but provide a way to defer the destruction
+// of the interface implementation:
+// - When connection error happended on a binding, the binding is immediately
+// destroyed and removed from the set. The interface implementation will be
+// destroyed when the DestroyCallback is called.
+// - When the DeferredDestroyStrongBindingSet is destructed, all outstanding
+// bindings and interface implementations in the set are destroyed immediately
+// without any deferral.
+template <typename Interface>
+class DeferredDestroyStrongBindingSet {
+ public:
+  // Converts a delete callback to a deleter. If the callback is null or has
+  // been cancelled, callback bound with invalidated weak pointer, the pointer
+  // will be deleted with "delete" immediately.
+  class Deleter {
+   public:
+    using DeleteCallback =
+        base::RepeatingCallback<void(std::unique_ptr<Interface>)>;
+
+    Deleter() = default;
+    explicit Deleter(DeleteCallback delete_cb)
+        : delete_cb_(std::move(delete_cb)) {}
+
+    void operator()(Interface* p) {
+      // Immediately wrap |p| into a unique_ptr to avoid any potential leak.
+      auto ptr = base::WrapUnique<Interface>(p);
+
+      // Can be cancelled during DeferredDestroyStrongBindingSet destruction.
+      if (delete_cb_ && !delete_cb_.IsCancelled())
+        delete_cb_.Run(std::move(ptr));
+      else
+        ptr.reset();
+    }
+
+   private:
+    DeleteCallback delete_cb_;
+  };
+
+  DeferredDestroyStrongBindingSet() : weak_factory_(this) {}
+
+  void AddBinding(std::unique_ptr<DeferredDestroy<Interface>> impl,
+                  mojo::InterfaceRequest<Interface> request) {
+    // Wrap the pointer into a unique_ptr with a deleter.
+    Deleter deleter(
+        base::BindRepeating(&DeferredDestroyStrongBindingSet::OnBindingRemoved,
+                            weak_factory_.GetWeakPtr()));
+    std::unique_ptr<Interface, Deleter> impl_with_deleter(impl.release(),
+                                                          deleter);
+
+    bindings_.AddBinding(std::move(impl_with_deleter), std::move(request));
+  }
+
+  // TODO(xhwang): Add RemoveBinding() if needed.
+
+  void CloseAllBindings() {
+    weak_factory_.InvalidateWeakPtrs();
+    bindings_.CloseAllBindings();
+    unbound_impls_.clear();
+  }
+
+  bool empty() const { return bindings_.empty(); }
+
+  size_t size() const { return bindings_.size(); }
+
+  size_t unbound_size() const { return unbound_impls_.size(); }
+
+ private:
+  void OnBindingRemoved(std::unique_ptr<Interface> ptr) {
+    DVLOG(1) << __func__;
+
+    id_++;
+
+    // The cast is safe since AddBinding() takes DeferredDestroy<Interface>.
+    auto* impl_ptr = static_cast<DeferredDestroy<Interface>*>(ptr.get());
+
+    // Put the |ptr| in the map before calling OnDestroyPending() because the
+    // callback could be called synchronously.
+    unbound_impls_[id_] = std::move(ptr);
+
+    // Use BindToCurrentLoop() to force post the destroy callback. This is
+    // needed because the callback may be called directly in the same stack
+    // where the implemenation is being destroyed.
+    impl_ptr->OnDestroyPending(BindToCurrentLoop(
+        base::BindOnce(&DeferredDestroyStrongBindingSet::OnDestroyable,
+                       weak_factory_.GetWeakPtr(), id_)));
+  }
+
+  void OnDestroyable(int id) {
+    DVLOG(1) << __func__;
+    unbound_impls_.erase(id);
+  }
+
+  uint32_t id_ = 0;
+  std::map<uint32_t, std::unique_ptr<Interface>> unbound_impls_;
+  mojo::StrongBindingSet<Interface, void, Deleter> bindings_;
+
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<DeferredDestroyStrongBindingSet> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeferredDestroyStrongBindingSet);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_MOJO_SERVICES_DEFERRED_DESTROY_STRONG_BINDING_SET_H_

media/mojo/services/deferred_destroy_strong_binding_set_unittest.cc

+// Copyright 2018 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 <memory>
+#include <utility>
+
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "media/mojo/services/deferred_destroy_strong_binding_set.h"
+#include "mojo/public/interfaces/bindings/tests/ping_service.mojom.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+namespace {
+
+using PingService = mojo::test::PingService;
+using PingServicePtr = mojo::test::PingServicePtr;
+
+class DeferredDestroyPingImpl : public DeferredDestroy<PingService> {
+ public:
+  DeferredDestroyPingImpl() { ++instance_count; }
+  ~DeferredDestroyPingImpl() override { --instance_count; }
+
+  // DeferredDestroy<PingService> implementation.
+  void Ping(const PingCallback& callback) override {}
+  void OnDestroyPending(base::OnceClosure destroy_cb) override {
+    destroy_cb_ = std::move(destroy_cb);
+    if (can_destroy_)
+      std::move(destroy_cb_).Run();
+  }
+
+  void set_can_destroy() {
+    can_destroy_ = true;
+    if (destroy_cb_)
+      std::move(destroy_cb_).Run();
+  }
+
+  static int instance_count;
+
+ private:
+  bool can_destroy_ = false;
+  base::OnceClosure destroy_cb_;
+};
+int DeferredDestroyPingImpl::instance_count = 0;
+
+DeferredDestroyPingImpl* AddDeferredDestroyBinding(
+    DeferredDestroyStrongBindingSet<PingService>* bindings,
+    PingServicePtr* ptr) {
+  auto impl = std::make_unique<DeferredDestroyPingImpl>();
+  DeferredDestroyPingImpl* impl_ptr = impl.get();
+  bindings->AddBinding(std::move(impl), mojo::MakeRequest(ptr));
+  return impl_ptr;
+}
+
+class DeferredDestroyStrongBindingSetTest : public testing::Test {
+ public:
+  DeferredDestroyStrongBindingSetTest() = default;
+  ~DeferredDestroyStrongBindingSetTest() override = default;
+
+ protected:
+  base::test::ScopedTaskEnvironment task_environment_;
+};
+
+TEST_F(DeferredDestroyStrongBindingSetTest, Destructor) {
+  PingServicePtr ping[2];
+  auto bindings =
+      std::make_unique<DeferredDestroyStrongBindingSet<PingService>>();
+
+  for (int i = 0; i < 2; ++i)
+    AddDeferredDestroyBinding(bindings.get(), ping + i);
+  EXPECT_EQ(2, DeferredDestroyPingImpl::instance_count);
+
+  bindings.reset();
+  EXPECT_EQ(0, DeferredDestroyPingImpl::instance_count);
+}
+
+TEST_F(DeferredDestroyStrongBindingSetTest, ConnectionError) {
+  PingServicePtr ping[4];
+  DeferredDestroyPingImpl* impl[4];
+  auto bindings =
+      std::make_unique<DeferredDestroyStrongBindingSet<PingService>>();
+
+  for (int i = 0; i < 4; ++i)
+    impl[i] = AddDeferredDestroyBinding(bindings.get(), ping + i);
+  EXPECT_EQ(4, DeferredDestroyPingImpl::instance_count);
+
+  // Destroy deferred after connection error until set_can_destroy()..
+  ping[0].reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(4, DeferredDestroyPingImpl::instance_count);
+  impl[0]->set_can_destroy();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(3, DeferredDestroyPingImpl::instance_count);
+
+  // Destroyed immediately after connection error if set_can_destroy() in
+  // advance.
+  impl[1]->set_can_destroy();
+  ping[1].reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, DeferredDestroyPingImpl::instance_count);
+
+  // Deferred after connection until binding set destruction.
+  ping[2].reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, DeferredDestroyPingImpl::instance_count);
+
+  // Destructing the binding set will destruct all impls, including deferred
+  // destroy impls.
+  bindings.reset();
+  EXPECT_EQ(0, DeferredDestroyPingImpl::instance_count);
+}
+
+TEST_F(DeferredDestroyStrongBindingSetTest, CloseAllBindings) {
+  PingServicePtr ping[3];
+  DeferredDestroyPingImpl* impl[3];
+  DeferredDestroyStrongBindingSet<PingService> bindings;
+
+  for (int i = 0; i < 2; ++i)
+    impl[i] = AddDeferredDestroyBinding(&bindings, ping + i);
+  EXPECT_EQ(2, DeferredDestroyPingImpl::instance_count);
+  EXPECT_FALSE(bindings.empty());
+
+  bindings.CloseAllBindings();
+  EXPECT_EQ(0, DeferredDestroyPingImpl::instance_count);
+  EXPECT_TRUE(bindings.empty());
+
+  // After CloseAllBindings, new added bindings can still be deferred destroyed.
+  impl[2] = AddDeferredDestroyBinding(&bindings, ping + 2);
+  EXPECT_EQ(1, DeferredDestroyPingImpl::instance_count);
+
+  ping[2].reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, DeferredDestroyPingImpl::instance_count);
+
+  impl[2]->set_can_destroy();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, DeferredDestroyPingImpl::instance_count);
+}
+
+}  // namespace
+}  // namespace media

mojo/public/cpp/bindings/strong_binding_set.h

@@ -15,10 +15,12 @@ namespace mojo {
 // set, and the interface implementation is deleted. When the StrongBindingSet
 // is destructed, all outstanding bindings in the set are destroyed and all the
 // bound interface implementations are automatically deleted.
-template <typename Interface, typename ContextType = void>
-using StrongBindingSet =
-    BindingSetBase<Interface,
-                   Binding<Interface, UniquePtrImplRefTraits<Interface>>,
+template <typename Interface,
+          typename ContextType = void,
+          typename Deleter = std::default_delete<Interface>>
+using StrongBindingSet = BindingSetBase<
+    Interface,
+    Binding<Interface, UniquePtrImplRefTraits<Interface, Deleter>>,
     ContextType>;
 
 }  // namespace mojo

mojo/public/cpp/bindings/unique_ptr_impl_ref_traits.h

@@ -9,9 +9,9 @@ namespace mojo {
 
 // Traits for a binding's implementation reference type.
 // This corresponds to a unique_ptr reference type.
-template <typename Interface>
+template <typename Interface, typename Deleter = std::default_delete<Interface>>
 struct UniquePtrImplRefTraits {
-  using PointerType = std::unique_ptr<Interface>;
+  using PointerType = std::unique_ptr<Interface, Deleter>;
 
   static bool IsNull(const PointerType& ptr) { return !ptr; }
   static Interface* GetRawPointer(PointerType* ptr) { return ptr->get(); }