Revert "mac: Simplify the local_discovery ServiceWatcher and ServiceResolver."

This reverts commit f4240b29.

Reason for revert: Unfortunately, this seems to have caused a UAF:
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8877395395161584064/+/steps/unit_tests/0/logs/Deterministic_failure:_ServiceDiscoveryClientMacTest.ServiceWatcher__status_CRASH_/0

Original change's description:
> mac: Simplify the local_discovery ServiceWatcher and ServiceResolver.
> 
> This removes the inner Container classes by moving the logic into the
> ObjC classes that already exist.
> 
> Bug: 1072841
> Change-Id: If22d2d90ce3235ce160a0b740337fd71353a7ef7
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2243995
> Commit-Queue: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#778341}

TBR=ellyjones@chromium.org,rsesek@chromium.org

Change-Id: I3b2a26c44c52d2fb4ef3b3323c0c82c85db9239c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1072841
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246125Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
Commit-Queue: Alice Boxhall <aboxhall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#778588}

chrome/browser/local_discovery/service_discovery_client_mac.h

@@ -15,16 +15,20 @@
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/local_discovery/service_discovery_shared_client.h"
+#include "content/public/browser/browser_thread.h"
 
 namespace base {
 class Thread;
 }
 
-@class NetServiceBrowser;
-@class NetServiceResolver;
-
 namespace local_discovery {
 
+template <class T>
+class ServiceDiscoveryThreadDeleter {
+ public:
+  inline void operator()(T* t) { t->DeleteSoon(); }
+};
+
 // Implementation of ServiceDiscoveryClient that uses the Bonjour SDK.
 // https://developer.apple.com/library/mac/documentation/Networking/Conceptual/
 // NSNetServiceProgGuide/Articles/BrowsingForServices.html
@@ -56,6 +60,42 @@ class ServiceDiscoveryClientMac : public ServiceDiscoverySharedClient {
 
 class ServiceWatcherImplMac : public ServiceWatcher {
  public:
+  class NetServiceBrowserContainer {
+   public:
+    NetServiceBrowserContainer(
+        const std::string& service_type,
+        ServiceWatcher::UpdatedCallback callback,
+        scoped_refptr<base::SingleThreadTaskRunner> service_discovery_runner);
+    ~NetServiceBrowserContainer();
+
+    void Start();
+    void DiscoverNewServices();
+
+    void OnServicesUpdate(ServiceWatcher::UpdateType update,
+                          const std::string& service);
+
+    void DeleteSoon();
+
+   private:
+    void StartOnDiscoveryThread();
+    void DiscoverOnDiscoveryThread();
+
+    bool IsOnServiceDiscoveryThread() {
+      return base::ThreadTaskRunnerHandle::Get() ==
+             service_discovery_runner_.get();
+    }
+
+    std::string service_type_;
+    ServiceWatcher::UpdatedCallback callback_;
+
+    scoped_refptr<base::SingleThreadTaskRunner> callback_runner_;
+    scoped_refptr<base::SingleThreadTaskRunner> service_discovery_runner_;
+
+    base::scoped_nsobject<id> delegate_;
+    base::scoped_nsobject<NSNetServiceBrowser> browser_;
+    base::WeakPtrFactory<NetServiceBrowserContainer> weak_factory_;
+  };
+
   ServiceWatcherImplMac(
       const std::string& service_type,
       ServiceWatcher::UpdatedCallback callback,
@@ -72,28 +112,57 @@ class ServiceWatcherImplMac : public ServiceWatcher {
   void SetActivelyRefreshServices(bool actively_refresh_services) override;
   std::string GetServiceType() const override;
 
-  void StartOnDiscoveryThread(
-      ServiceWatcher::UpdatedCallback callback,
-      scoped_refptr<base::SingleThreadTaskRunner> callback_runner);
-  void DiscoverOnDiscoveryThread();
-
-  // These members should only be accessed on the object creator's sequence.
-  const std::string service_type_;
+  std::string service_type_;
   ServiceWatcher::UpdatedCallback callback_;
-  bool started_ = false;
+  bool started_;
 
-  scoped_refptr<base::SingleThreadTaskRunner> service_discovery_runner_;
-  // |browser_| lives on the |service_discovery_runner_|. It is released
-  // by move()ing it to StopServiceBrowser().
-  base::scoped_nsobject<NetServiceBrowser> browser_;
-
-  base::WeakPtrFactory<ServiceWatcherImplMac> weak_factory_{this};
+  std::unique_ptr<NetServiceBrowserContainer,
+                  ServiceDiscoveryThreadDeleter<NetServiceBrowserContainer>>
+      container_;
+  base::WeakPtrFactory<ServiceWatcherImplMac> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWatcherImplMac);
 };
 
 class ServiceResolverImplMac : public ServiceResolver {
  public:
+  class NetServiceContainer {
+   public:
+    NetServiceContainer(
+        const std::string& service_name,
+        ServiceResolver::ResolveCompleteCallback callback,
+        scoped_refptr<base::SingleThreadTaskRunner> service_discovery_runner);
+
+    virtual ~NetServiceContainer();
+
+    void StartResolving();
+
+    void OnResolveUpdate(RequestStatus);
+
+    void SetServiceForTesting(base::scoped_nsobject<NSNetService> service);
+
+    void DeleteSoon();
+
+   private:
+    void StartResolvingOnDiscoveryThread();
+
+    bool IsOnServiceDiscoveryThread() {
+      return base::ThreadTaskRunnerHandle::Get() ==
+             service_discovery_runner_.get();
+    }
+
+    const std::string service_name_;
+    ServiceResolver::ResolveCompleteCallback callback_;
+
+    scoped_refptr<base::SingleThreadTaskRunner> callback_runner_;
+    scoped_refptr<base::SingleThreadTaskRunner> service_discovery_runner_;
+
+    base::scoped_nsobject<id> delegate_;
+    base::scoped_nsobject<NSNetService> service_;
+    ServiceDescription service_description_;
+    base::WeakPtrFactory<NetServiceContainer> weak_factory_;
+  };
+
   ServiceResolverImplMac(
       const std::string& service_name,
       ServiceResolver::ResolveCompleteCallback callback,
@@ -101,37 +170,29 @@ class ServiceResolverImplMac : public ServiceResolver {
 
   ~ServiceResolverImplMac() override;
 
+  // Testing methods.
+  NetServiceContainer* GetContainerForTesting();
+
  private:
+
   void StartResolving() override;
   std::string GetName() const override;
 
   void OnResolveComplete(RequestStatus status,
                          const ServiceDescription& description);
 
-  void StartResolvingOnDiscoveryThread(
-      ServiceResolver::ResolveCompleteCallback callback,
-      scoped_refptr<base::SingleThreadTaskRunner> callback_runner);
-  void StopResolving();
-
-  // These members should only be accessed on the object creator's sequence.
   const std::string service_name_;
   ServiceResolver::ResolveCompleteCallback callback_;
-  bool has_resolved_ = false;
+  bool has_resolved_;
 
-  scoped_refptr<base::SingleThreadTaskRunner> service_discovery_runner_;
-  // |resolver_| lives on the |service_discovery_runner_|. It is released
-  // by move()ing it to StopServiceResolver().
-  base::scoped_nsobject<NetServiceResolver> resolver_;
-
-  base::WeakPtrFactory<ServiceResolverImplMac> weak_factory_{this};
+  std::unique_ptr<NetServiceContainer,
+                  ServiceDiscoveryThreadDeleter<NetServiceContainer>>
+      container_;
+  base::WeakPtrFactory<ServiceResolverImplMac> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceResolverImplMac);
 };
 
-// Parses the data out of the |service|, updating the |description| with the
-// results.
-void ParseNetService(NSNetService* service, ServiceDescription& description);
-
 }  // namespace local_discovery
 
 #endif  // CHROME_BROWSER_LOCAL_DISCOVERY_SERVICE_DISCOVERY_CLIENT_MAC_H_

chrome/browser/local_discovery/service_discovery_client_mac_unittest.mm

@@ -115,7 +115,13 @@ TEST_F(ServiceDiscoveryClientMacTest, ServiceWatcher) {
   EXPECT_EQ(num_updates_, 3);
 }
 
-TEST_F(ServiceDiscoveryClientMacTest, ParseServiceRecord) {
+TEST_F(ServiceDiscoveryClientMacTest, ServiceResolver) {
+  const std::string test_service_name = "Test.123._testing._tcp.local";
+  std::unique_ptr<ServiceResolver> resolver = client()->CreateServiceResolver(
+      test_service_name,
+      base::BindOnce(&ServiceDiscoveryClientMacTest::OnResolveComplete,
+                     base::Unretained(this)));
+
   const uint8_t record_bytes[] = {2, 'a', 'b', 3, 'd', '=', 'e'};
   base::scoped_nsobject<TestNSNetService> test_service([[TestNSNetService alloc]
       initWithData:[NSData dataWithBytes:record_bytes
@@ -133,21 +139,38 @@ TEST_F(ServiceDiscoveryClientMacTest, ParseServiceRecord) {
   NSArray* addresses = @[ discoveryHost ];
   [test_service setAddresses:addresses];
 
-  ServiceDescription description;
-  ParseNetService(test_service.get(), description);
+  ServiceResolverImplMac* resolver_impl =
+      static_cast<ServiceResolverImplMac*>(resolver.get());
+  resolver_impl->GetContainerForTesting()->SetServiceForTesting(
+      base::scoped_nsobject<NSNetService>(test_service));
+  resolver->StartResolving();
+
+  resolver_impl->GetContainerForTesting()->OnResolveUpdate(
+      ServiceResolver::STATUS_SUCCESS);
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, num_resolves_);
 
-  const std::vector<std::string>& metadata = description.metadata;
+  const std::vector<std::string>& metadata =
+      last_service_description_.metadata;
   EXPECT_EQ(2u, metadata.size());
   EXPECT_TRUE(base::Contains(metadata, "ab"));
   EXPECT_TRUE(base::Contains(metadata, "d=e"));
 
-  EXPECT_EQ(ip_address, description.ip_address);
-  EXPECT_EQ(kPort, description.address.port());
-  EXPECT_EQ(kIp, description.address.host());
+  EXPECT_EQ(ip_address, last_service_description_.ip_address);
+  EXPECT_EQ(kPort, last_service_description_.address.port());
+  EXPECT_EQ(kIp, last_service_description_.address.host());
 }
 
 // https://crbug.com/586628
-TEST_F(ServiceDiscoveryClientMacTest, ParseInvalidUnicodeRecord) {
+TEST_F(ServiceDiscoveryClientMacTest, ResolveInvalidUnicodeRecord) {
+  const std::string test_service_name = "Test.123._testing._tcp.local";
+  std::unique_ptr<ServiceResolver> resolver = client()->CreateServiceResolver(
+      test_service_name,
+      base::BindOnce(&ServiceDiscoveryClientMacTest::OnResolveComplete,
+                     base::Unretained(this)));
+
   const uint8_t record_bytes[] = {
     3, 'a', '=', 'b',
     // The bytes after name= are the UTF-8 encoded representation of
@@ -171,17 +194,28 @@ TEST_F(ServiceDiscoveryClientMacTest, ParseInvalidUnicodeRecord) {
   NSArray* addresses = @[ discovery_host ];
   [test_service setAddresses:addresses];
 
-  ServiceDescription description;
-  ParseNetService(test_service.get(), description);
+  ServiceResolverImplMac* resolver_impl =
+      static_cast<ServiceResolverImplMac*>(resolver.get());
+  resolver_impl->GetContainerForTesting()->SetServiceForTesting(
+      base::scoped_nsobject<NSNetService>(test_service));
+  resolver->StartResolving();
+
+  resolver_impl->GetContainerForTesting()->OnResolveUpdate(
+      ServiceResolver::STATUS_SUCCESS);
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, num_resolves_);
 
-  const std::vector<std::string>& metadata = description.metadata;
+  const std::vector<std::string>& metadata =
+      last_service_description_.metadata;
   EXPECT_EQ(2u, metadata.size());
   EXPECT_TRUE(base::Contains(metadata, "a=b"));
   EXPECT_TRUE(base::Contains(metadata, "cd=e9"));
 
-  EXPECT_EQ(ip_address, description.ip_address);
-  EXPECT_EQ(kPort, description.address.port());
-  EXPECT_EQ(kIp, description.address.host());
+  EXPECT_EQ(ip_address, last_service_description_.ip_address);
+  EXPECT_EQ(kPort, last_service_description_.address.port());
+  EXPECT_EQ(kIp, last_service_description_.address.host());
 }
 
 TEST_F(ServiceDiscoveryClientMacTest, ResolveInvalidServiceName) {