[mDns] Make DnsSdRegistry a leaky singleton

Make DnsSdRegistry a leaky singleton so that it can be used by both CastMediaSinkService and MDnsAPI.

BUG=687377

Review-Url: https://codereview.chromium.org/2874243003
Cr-Commit-Position: refs/heads/master@{#472201}

chrome/browser/extensions/api/mdns/mdns_api.cc

@@ -38,7 +38,8 @@ bool IsServiceTypeWhitelisted(const std::string& service_type) {
 
 using DnsSdRegistry = media_router::DnsSdRegistry;
 
-MDnsAPI::MDnsAPI(content::BrowserContext* context) : browser_context_(context) {
+MDnsAPI::MDnsAPI(content::BrowserContext* context)
+    : browser_context_(context), dns_sd_registry_(nullptr) {
   DCHECK(browser_context_);
   extensions::EventRouter* event_router = EventRouter::Get(context);
   DCHECK(event_router);
@@ -46,7 +47,7 @@ MDnsAPI::MDnsAPI(content::BrowserContext* context) : browser_context_(context) {
 }
 
 MDnsAPI::~MDnsAPI() {
-  if (dns_sd_registry_.get()) {
+  if (dns_sd_registry_) {
     dns_sd_registry_->RemoveObserver(this);
   }
 }
@@ -65,10 +66,9 @@ BrowserContextKeyedAPIFactory<MDnsAPI>* MDnsAPI::GetFactoryInstance() {
   return g_factory.Pointer();
 }
 
-void MDnsAPI::SetDnsSdRegistryForTesting(
-    std::unique_ptr<DnsSdRegistry> dns_sd_registry) {
-  dns_sd_registry_ = std::move(dns_sd_registry);
-  if (dns_sd_registry_.get())
+void MDnsAPI::SetDnsSdRegistryForTesting(DnsSdRegistry* dns_sd_registry) {
+  dns_sd_registry_ = dns_sd_registry;
+  if (dns_sd_registry_)
     dns_sd_registry_->AddObserver(this);
 }
 
@@ -80,11 +80,11 @@ void MDnsAPI::ForceDiscovery() {
 
 DnsSdRegistry* MDnsAPI::dns_sd_registry() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!dns_sd_registry_.get()) {
-    dns_sd_registry_.reset(new media_router::DnsSdRegistry());
+  if (!dns_sd_registry_) {
+    dns_sd_registry_ = media_router::DnsSdRegistry::GetInstance();
     dns_sd_registry_->AddObserver(this);
   }
-  return dns_sd_registry_.get();
+  return dns_sd_registry_;
 }
 
 void MDnsAPI::OnListenerAdded(const EventListenerInfo& details) {

chrome/browser/extensions/api/mdns/mdns_api.h

@@ -24,10 +24,6 @@ namespace content {
 class BrowserContext;
 }
 
-namespace media_router {
-class DnsSdRegistry;
-}
-
 namespace extensions {
 // MDnsAPI is instantiated with the profile and will listen for extensions that
 // register listeners for the chrome.mdns extension API. It will use a registry
@@ -45,9 +41,9 @@ class MDnsAPI : public BrowserContextKeyedAPI,
   // BrowserContextKeyedAPI implementation.
   static BrowserContextKeyedAPIFactory<MDnsAPI>* GetFactoryInstance();
 
-  // Used to mock out the DnsSdRegistry for testing.
-  void SetDnsSdRegistryForTesting(
-      std::unique_ptr<media_router::DnsSdRegistry> registry);
+  // Used to mock out the DnsSdRegistry for testing. Does not take ownership of
+  // |registry|.
+  void SetDnsSdRegistryForTesting(media_router::DnsSdRegistry* registry);
 
   // Immediately issues a multicast DNS query for all service types.
   // NOTE: Discovery queries are sent to all event handlers associated with
@@ -112,8 +108,9 @@ class MDnsAPI : public BrowserContextKeyedAPI,
   // Ensure methods are only called on UI thread.
   base::ThreadChecker thread_checker_;
   content::BrowserContext* const browser_context_;
-  // Lazily created on first access and destroyed with this API class.
-  std::unique_ptr<media_router::DnsSdRegistry> dns_sd_registry_;
+  // Raw pointer to a leaky singleton. Lazily created on first access. Must
+  // outlive this object.
+  media_router::DnsSdRegistry* dns_sd_registry_;
   // Count of active listeners per service type, saved from the previous
   // invocation of UpdateMDnsListeners().
   ServiceTypeCounts prev_service_counts_;

chrome/browser/extensions/api/mdns/mdns_api_unittest.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_service_test_base.h"
 #include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.h"
 #include "chrome/common/extensions/api/mdns.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/test/mock_render_process_host.h"
@@ -26,6 +27,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using media_router::MockDnsSdRegistry;
 using testing::_;
 using testing::Return;
 using testing::ReturnRef;
@@ -91,27 +93,6 @@ base::FilePath bogus_file_pathname(const std::string& name) {
       .AppendASCII(name);
 }
 
-class MockDnsSdRegistry : public media_router::DnsSdRegistry {
- public:
-  explicit MockDnsSdRegistry(extensions::MDnsAPI* api) : api_(api) {}
-  virtual ~MockDnsSdRegistry() {}
-
-  MOCK_METHOD1(AddObserver, void(DnsSdObserver* observer));
-  MOCK_METHOD1(RemoveObserver, void(DnsSdObserver* observer));
-  MOCK_METHOD1(RegisterDnsSdListener, void(const std::string& service_type));
-  MOCK_METHOD1(UnregisterDnsSdListener, void(const std::string& service_type));
-  MOCK_METHOD1(Publish, void(const std::string&));
-  MOCK_METHOD0(ForceDiscovery, void(void));
-
-  void DispatchMDnsEvent(const std::string& service_type,
-                         const DnsSdServiceList& services) {
-    api_->OnDnsSdEvent(service_type, services);
-  }
-
- private:
-  media_router::DnsSdRegistry::DnsSdObserver* api_;
-};
-
 class MockEventRouter : public EventRouter {
  public:
   explicit MockEventRouter(content::BrowserContext* browser_context,
@@ -204,13 +185,13 @@ class MDnsAPITest : public extensions::ExtensionServiceTestBase {
     ASSERT_TRUE(MDnsAPI::Get(browser_context()));      // constructs MDnsAPI
     ASSERT_TRUE(EventRouter::Get(browser_context()));  // constructs EventRouter
 
-    registry_ = new MockDnsSdRegistry(MDnsAPI::Get(browser_context()));
+    registry_ =
+        base::MakeUnique<MockDnsSdRegistry>(MDnsAPI::Get(browser_context()));
     EXPECT_CALL(*dns_sd_registry(),
                 AddObserver(MDnsAPI::Get(browser_context())))
         .Times(1);
     MDnsAPI::Get(browser_context())
-        ->SetDnsSdRegistryForTesting(
-            std::unique_ptr<media_router::DnsSdRegistry>(registry_));
+        ->SetDnsSdRegistryForTesting(registry_.get());
 
     render_process_host_.reset(
         new content::MockRenderProcessHost(browser_context()));
@@ -223,16 +204,12 @@ class MDnsAPITest : public extensions::ExtensionServiceTestBase {
   }
 
   void TearDown() override {
-    EXPECT_CALL(*dns_sd_registry(),
-                RemoveObserver(MDnsAPI::Get(browser_context())))
-        .Times(1);
+    MDnsAPI::Get(browser_context())->SetDnsSdRegistryForTesting(nullptr);
     render_process_host_.reset();
     extensions::ExtensionServiceTestBase::TearDown();
   }
 
-  virtual MockDnsSdRegistry* dns_sd_registry() {
-    return registry_;
-  }
+  virtual MockDnsSdRegistry* dns_sd_registry() { return registry_.get(); }
 
   // Constructs an extension according to the parameters that matter most to
   // MDnsAPI the local unit tests.
@@ -265,9 +242,7 @@ class MDnsAPITest : public extensions::ExtensionServiceTestBase {
   }
 
  private:
-  // The registry is owned by MDnsAPI, but MDnsAPI does not have an accessor
-  // for it, so use a private member.
-  MockDnsSdRegistry* registry_;
+  std::unique_ptr<MockDnsSdRegistry> registry_;
 
   std::unique_ptr<content::RenderProcessHost> render_process_host_;
 };

chrome/browser/extensions/api/mdns/mdns_apitest.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/extensions/api/mdns/mdns_api.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.h"
 #include "chrome/common/extensions/api/mdns.h"
 #include "extensions/common/switches.h"
 #include "extensions/test/result_catcher.h"
@@ -15,32 +16,12 @@
 
 using ::testing::_;
 using ::testing::A;
-using DnsSdRegistry = media_router::DnsSdRegistry;
+using media_router::DnsSdRegistry;
 
 namespace api = extensions::api;
 
 namespace {
 
-class MockDnsSdRegistry : public DnsSdRegistry {
- public:
-  explicit MockDnsSdRegistry(extensions::MDnsAPI* api) : api_(api) {}
-  virtual ~MockDnsSdRegistry() {}
-
-  MOCK_METHOD1(AddObserver, void(DnsSdObserver* observer));
-  MOCK_METHOD1(RemoveObserver, void(DnsSdObserver* observer));
-  MOCK_METHOD1(RegisterDnsSdListener, void(const std::string& service_type));
-  MOCK_METHOD1(UnregisterDnsSdListener, void(const std::string& service_type));
-  MOCK_METHOD0(ForceDiscovery, void(void));
-
-  void DispatchMDnsEvent(const std::string& service_type,
-                         const DnsSdServiceList& services) {
-    api_->OnDnsSdEvent(service_type, services);
-  }
-
- private:
-  DnsSdRegistry::DnsSdObserver* api_;
-};
-
 class MDnsAPITest : public ExtensionApiTest {
  public:
   MDnsAPITest() {}
@@ -54,16 +35,14 @@ class MDnsAPITest : public ExtensionApiTest {
 
   void SetUpTestDnsSdRegistry() {
     extensions::MDnsAPI* api = extensions::MDnsAPI::Get(profile());
-    dns_sd_registry_ = new MockDnsSdRegistry(api);
+    dns_sd_registry_ = base::MakeUnique<media_router::MockDnsSdRegistry>(api);
     EXPECT_CALL(*dns_sd_registry_, AddObserver(api))
         .Times(1);
-    // Transfers ownership of the registry instance.
-    api->SetDnsSdRegistryForTesting(
-        base::WrapUnique<DnsSdRegistry>(dns_sd_registry_));
+    api->SetDnsSdRegistryForTesting(dns_sd_registry_.get());
   }
 
  protected:
-  MockDnsSdRegistry* dns_sd_registry_;
+  std::unique_ptr<media_router::MockDnsSdRegistry> dns_sd_registry_;
 };
 
 }  // namespace

chrome/browser/media/router/BUILD.gn

@@ -109,6 +109,8 @@ static_library("test_support") {
       "//extensions/common",
     ]
     sources += [
+      "discovery/mdns/mock_dns_sd_registry.cc",
+      "discovery/mdns/mock_dns_sd_registry.h",
       "mojo/media_router_mojo_test.cc",
       "mojo/media_router_mojo_test.h",
     ]

chrome/browser/media/router/discovery/mdns/dns_sd_registry.cc

@@ -115,13 +115,23 @@ DnsSdRegistry::DnsSdRegistry(ServiceDiscoverySharedClient* client) {
   service_discovery_client_ = client;
 }
 
-DnsSdRegistry::~DnsSdRegistry() {}
+DnsSdRegistry::~DnsSdRegistry() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+// static
+DnsSdRegistry* DnsSdRegistry::GetInstance() {
+  return base::Singleton<DnsSdRegistry,
+                         base::LeakySingletonTraits<DnsSdRegistry>>::get();
+}
 
 void DnsSdRegistry::AddObserver(DnsSdObserver* observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   observers_.AddObserver(observer);
 }
 
 void DnsSdRegistry::RemoveObserver(DnsSdObserver* observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   observers_.RemoveObserver(observer);
 }
 
@@ -133,16 +143,19 @@ DnsSdDeviceLister* DnsSdRegistry::CreateDnsSdDeviceLister(
 }
 
 void DnsSdRegistry::Publish(const std::string& service_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DispatchApiEvent(service_type);
 }
 
 void DnsSdRegistry::ForceDiscovery() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   for (const auto& next_service : service_data_map_) {
     next_service.second->ForceDiscovery();
   }
 }
 
 void DnsSdRegistry::RegisterDnsSdListener(const std::string& service_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "RegisterDnsSdListener: " << service_type
           << ", registered: " << IsRegistered(service_type);
   if (service_type.empty())
@@ -164,6 +177,7 @@ void DnsSdRegistry::RegisterDnsSdListener(const std::string& service_type) {
 }
 
 void DnsSdRegistry::UnregisterDnsSdListener(const std::string& service_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "UnregisterDnsSdListener: " << service_type;
   auto it = service_data_map_.find(service_type);
   if (it == service_data_map_.end())
@@ -176,6 +190,7 @@ void DnsSdRegistry::UnregisterDnsSdListener(const std::string& service_type) {
 void DnsSdRegistry::ServiceChanged(const std::string& service_type,
                                    bool added,
                                    const DnsSdService& service) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "ServiceChanged: service_type: " << service_type
           << ", known: " << IsRegistered(service_type)
           << ", service: " << service.service_name << ", added: " << added;
@@ -194,6 +209,7 @@ void DnsSdRegistry::ServiceChanged(const std::string& service_type,
 
 void DnsSdRegistry::ServiceRemoved(const std::string& service_type,
                                    const std::string& service_name) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "ServiceRemoved: service_type: " << service_type
           << ", known: " << IsRegistered(service_type)
           << ", service: " << service_name;
@@ -210,6 +226,7 @@ void DnsSdRegistry::ServiceRemoved(const std::string& service_type,
 }
 
 void DnsSdRegistry::ServicesFlushed(const std::string& service_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "ServicesFlushed: service_type: " << service_type
           << ", known: " << IsRegistered(service_type);
   if (!IsRegistered(service_type)) {
@@ -224,6 +241,7 @@ void DnsSdRegistry::ServicesFlushed(const std::string& service_type) {
 }
 
 void DnsSdRegistry::DispatchApiEvent(const std::string& service_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "DispatchApiEvent: service_type: " << service_type;
   for (auto& observer : observers_) {
     observer.OnDnsSdEvent(service_type,
@@ -232,6 +250,7 @@ void DnsSdRegistry::DispatchApiEvent(const std::string& service_type) {
 }
 
 bool DnsSdRegistry::IsRegistered(const std::string& service_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   return service_data_map_.find(service_type) != service_data_map_.end();
 }
 

chrome/browser/media/router/discovery/mdns/dns_sd_registry.h

@@ -12,7 +12,9 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/singleton.h"
 #include "base/observer_list.h"
+#include "base/threading/thread_checker.h"
 #include "chrome/browser/media/router/discovery/mdns/dns_sd_delegate.h"
 
 namespace local_discovery {
@@ -38,9 +40,7 @@ class DnsSdRegistry : public DnsSdDelegate {
     virtual ~DnsSdObserver() {}
   };
 
-  DnsSdRegistry();
-  explicit DnsSdRegistry(local_discovery::ServiceDiscoverySharedClient* client);
-  virtual ~DnsSdRegistry();
+  static DnsSdRegistry* GetInstance();
 
   // Publishes the current device list for |service_type| to event listeners
   // whose event filter matches the service type.
@@ -108,12 +108,21 @@ class DnsSdRegistry : public DnsSdDelegate {
   std::map<std::string, std::unique_ptr<ServiceTypeData>> service_data_map_;
 
  private:
+  friend struct base::DefaultSingletonTraits<DnsSdRegistry>;
+  friend class MockDnsSdRegistry;
+  friend class TestDnsSdRegistry;
+
+  DnsSdRegistry();
+  explicit DnsSdRegistry(local_discovery::ServiceDiscoverySharedClient* client);
+  virtual ~DnsSdRegistry();
+
   void DispatchApiEvent(const std::string& service_type);
   bool IsRegistered(const std::string& service_type);
 
   scoped_refptr<local_discovery::ServiceDiscoverySharedClient>
       service_discovery_client_;
   base::ObserverList<DnsSdObserver> observers_;
+  base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(DnsSdRegistry);
 };

chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.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 "chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.h"
+
+namespace media_router {
+
+MockDnsSdRegistry::MockDnsSdRegistry(DnsSdRegistry::DnsSdObserver* observer)
+    : observer_(observer) {}
+
+MockDnsSdRegistry::~MockDnsSdRegistry() {}
+
+void MockDnsSdRegistry::DispatchMDnsEvent(const std::string& service_type,
+                                          const DnsSdServiceList& services) {
+  observer_->OnDnsSdEvent(service_type, services);
+}
+
+}  // namespace media_router

chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.h

+// 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.
+
+#ifndef CHROME_BROWSER_MEDIA_ROUTER_DISCOVERY_MDNS_MOCK_DNS_SD_REGISTRY_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_DISCOVERY_MDNS_MOCK_DNS_SD_REGISTRY_H_
+
+#include "chrome/browser/media/router/discovery/mdns/dns_sd_registry.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace media_router {
+
+class MockDnsSdRegistry : public DnsSdRegistry {
+ public:
+  explicit MockDnsSdRegistry(DnsSdObserver* observer);
+  ~MockDnsSdRegistry() override;
+
+  MOCK_METHOD1(AddObserver, void(DnsSdObserver* observer));
+  MOCK_METHOD1(RemoveObserver, void(DnsSdObserver* observer));
+  MOCK_METHOD1(RegisterDnsSdListener, void(const std::string& service_type));
+  MOCK_METHOD1(UnregisterDnsSdListener, void(const std::string& service_type));
+  MOCK_METHOD1(Publish, void(const std::string&));
+  MOCK_METHOD0(ForceDiscovery, void(void));
+
+  void DispatchMDnsEvent(const std::string& service_type,
+                         const DnsSdServiceList& services);
+
+ private:
+  DnsSdObserver* observer_;
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_DISCOVERY_MDNS_MOCK_DNS_SD_REGISTRY_H_