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

This is a reland of f4240b29

This fixes two issues:

- A real UAF in ServiceResolverImplMac::OnResolveComplete, where the
  callback can delete |this|, so StopResolving() is now called before
  the callback.
- A test-only UAF caused by the background thread outliving the object.
  The thread is now flushed and joined. The -stop methods for both
  NetServiceBrowser and NetServiceResolver are now called in their
  respective -deallocs, in case the Stop posted task does not run.

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}

Bug: 1072841
Change-Id: Ice53eb92d6c73c75ad6f276b301eaa1841536c8d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2248076Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779350}

chrome/browser/local_discovery/service_discovery_client_mac.h

@@ -15,19 +15,15 @@
 #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;
 }
 
-namespace local_discovery {
+@class NetServiceBrowser;
+@class NetServiceResolver;
 
-template <class T>
-class ServiceDiscoveryThreadDeleter {
- public:
-  inline void operator()(T* t) { t->DeleteSoon(); }
-};
+namespace local_discovery {
 
 // Implementation of ServiceDiscoveryClient that uses the Bonjour SDK.
 // https://developer.apple.com/library/mac/documentation/Networking/Conceptual/
@@ -37,6 +33,8 @@ class ServiceDiscoveryClientMac : public ServiceDiscoverySharedClient {
   ServiceDiscoveryClientMac();
 
  private:
+  friend class ServiceDiscoveryClientMacTest;
+
   ~ServiceDiscoveryClientMac() override;
 
   // ServiceDiscoveryClient implementation.
@@ -60,42 +58,6 @@ 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,
@@ -112,57 +74,28 @@ class ServiceWatcherImplMac : public ServiceWatcher {
   void SetActivelyRefreshServices(bool actively_refresh_services) override;
   std::string GetServiceType() const override;
 
-  std::string service_type_;
+  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_;
   ServiceWatcher::UpdatedCallback callback_;
-  bool started_;
+  bool started_ = false;
+
+  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_;
 
-  std::unique_ptr<NetServiceBrowserContainer,
-                  ServiceDiscoveryThreadDeleter<NetServiceBrowserContainer>>
-      container_;
-  base::WeakPtrFactory<ServiceWatcherImplMac> weak_factory_;
+  base::WeakPtrFactory<ServiceWatcherImplMac> weak_factory_{this};
 
   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,
@@ -170,29 +103,37 @@ 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_;
+  bool has_resolved_ = false;
 
-  std::unique_ptr<NetServiceContainer,
-                  ServiceDiscoveryThreadDeleter<NetServiceContainer>>
-      container_;
-  base::WeakPtrFactory<ServiceResolverImplMac> weak_factory_;
+  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};
 
   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

@@ -10,6 +10,7 @@
 #include "base/mac/scoped_nsobject.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
+#include "base/threading/thread.h"
 #include "chrome/browser/local_discovery/service_discovery_client.h"
 #include "chrome/browser/local_discovery/service_discovery_client_mac.h"
 #import "chrome/browser/ui/cocoa/test/cocoa_test_helper.h"
@@ -76,6 +77,11 @@ class ServiceDiscoveryClientMacTest : public CocoaTest {
     num_resolves_++;
   }
 
+  void StopDiscoveryThread() {
+    client_->service_discovery_thread_->FlushForTesting();
+    client_->service_discovery_thread_.reset();
+  }
+
   ServiceDiscoveryClient* client() { return client_.get(); }
 
  protected:
@@ -113,15 +119,13 @@ TEST_F(ServiceDiscoveryClientMacTest, ServiceWatcher) {
       ServiceWatcher::UPDATE_REMOVED, test_service_name);
   EXPECT_EQ(last_service_name_, test_service_name + "." + test_service_type);
   EXPECT_EQ(num_updates_, 3);
-}
 
-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)));
+  // Explicitly flush and stop the thread that |watcher| is using before
+  // |watcher| goes out of scope.
+  StopDiscoveryThread();
+}
 
+TEST_F(ServiceDiscoveryClientMacTest, ParseServiceRecord) {
   const uint8_t record_bytes[] = {2, 'a', 'b', 3, 'd', '=', 'e'};
   base::scoped_nsobject<TestNSNetService> test_service([[TestNSNetService alloc]
       initWithData:[NSData dataWithBytes:record_bytes
@@ -139,38 +143,21 @@ TEST_F(ServiceDiscoveryClientMacTest, ServiceResolver) {
   NSArray* addresses = @[ discoveryHost ];
   [test_service setAddresses:addresses];
 
-  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);
+  ServiceDescription description;
+  ParseNetService(test_service.get(), description);
 
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_EQ(1, num_resolves_);
-
-  const std::vector<std::string>& metadata =
-      last_service_description_.metadata;
+  const std::vector<std::string>& metadata = description.metadata;
   EXPECT_EQ(2u, metadata.size());
   EXPECT_TRUE(base::Contains(metadata, "ab"));
   EXPECT_TRUE(base::Contains(metadata, "d=e"));
 
-  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());
+  EXPECT_EQ(ip_address, description.ip_address);
+  EXPECT_EQ(kPort, description.address.port());
+  EXPECT_EQ(kIp, description.address.host());
 }
 
 // https://crbug.com/586628
-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)));
-
+TEST_F(ServiceDiscoveryClientMacTest, ParseInvalidUnicodeRecord) {
   const uint8_t record_bytes[] = {
     3, 'a', '=', 'b',
     // The bytes after name= are the UTF-8 encoded representation of
@@ -194,28 +181,17 @@ TEST_F(ServiceDiscoveryClientMacTest, ResolveInvalidUnicodeRecord) {
   NSArray* addresses = @[ discovery_host ];
   [test_service setAddresses:addresses];
 
-  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_);
+  ServiceDescription description;
+  ParseNetService(test_service.get(), description);
 
-  const std::vector<std::string>& metadata =
-      last_service_description_.metadata;
+  const std::vector<std::string>& metadata = 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, last_service_description_.ip_address);
-  EXPECT_EQ(kPort, last_service_description_.address.port());
-  EXPECT_EQ(kIp, last_service_description_.address.host());
+  EXPECT_EQ(ip_address, description.ip_address);
+  EXPECT_EQ(kPort, description.address.port());
+  EXPECT_EQ(kIp, description.address.host());
 }
 
 TEST_F(ServiceDiscoveryClientMacTest, ResolveInvalidServiceName) {