Remove UI thread WeakPtrFactory from DialService

A previous change to use the network service introduced two
WeakPtrFactory members to DialServiceImpl.  Although this gets around
the advice to only use WeakPtrs from one factory on a single sequence,
it doesn't actually solve the problem: ~DialServiceImpl still runs on
the IO thread, which may cause some UI thread WeakPtrs to be
invalidated, resulting in a DCHECK (or a race).  The UI thread doesn't
actually need any of DialServiceImpl's data though, so this change just
makes the functions previously using it into free functions which then
call back onto the IO thread with a proper WeakPtr.

Bug: 934611, 936531
Change-Id: Iee1612cd9934bef109bbfa79d497685dbd275d8a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504980
Commit-Queue: Brandon Tolsch <btolsch@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638283}

chrome/browser/media/router/discovery/dial/dial_service.cc

@@ -58,6 +58,17 @@ using net::UDPSocket;
 
 namespace media_router {
 
+#if !defined(OS_CHROMEOS)
+void PostSendNetworkList(
+    base::WeakPtr<DialServiceImpl> impl,
+    const base::Optional<net::NetworkInterfaceList>& networks) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  base::PostTaskWithTraits(FROM_HERE, {BrowserThread::IO},
+                           base::BindOnce(&DialServiceImpl::SendNetworkList,
+                                          std::move(impl), networks));
+}
+#endif  // !defined(OS_CHROMEOS)
+
 namespace {
 
 // The total number of requests to make per discovery cycle.
@@ -153,6 +164,24 @@ net::IPAddressList GetBestBindAddressOnUIThread() {
   }
   return bind_address_list;
 }
+#else
+// This function and PostSendNetworkList together handle DialServiceImpl's use
+// of the network service, while keeping all of DialServiceImpl running on the
+// IO thread.  DialServiceImpl has a legacy threading model, where it was
+// designed to be called from the UI thread and run on the IO thread.  Although
+// a WeakPtr is desired for safety when posting tasks, they are not
+// thread/sequence-safe.  DialServiceImpl's simple use of the network service,
+// however, doesn't actually require that any of its state be accessed on the UI
+// thread.  Therefore, the UI thread functions can be free functions which just
+// pass-through an IO thread WeakPtr which will be used when passing the network
+// service result back to the IO thread.  This model will change when the
+// network service is fully launched and this code is updated.
+void GetNetworkListOnUIThread(base::WeakPtr<DialServiceImpl> impl) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  content::GetNetworkService()->GetNetworkList(
+      net::INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES,
+      base::BindOnce(&PostSendNetworkList, std::move(impl)));
+}
 #endif  // defined(OS_CHROMEOS)
 
 }  // namespace
@@ -385,6 +414,7 @@ DialServiceImpl::DialServiceImpl(net::NetLog* net_log)
                     TimeDelta::FromSeconds(kDialResponseTimeoutSecs)),
       request_interval_(
           TimeDelta::FromMilliseconds(kDialRequestIntervalMillis)) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   IPAddress address;
   bool success = address.AssignFromIPLiteral(kDialRequestAddress);
   DCHECK(success);
@@ -443,69 +473,55 @@ void DialServiceImpl::StartDiscovery() {
       base::BindOnce(&DialServiceImpl::DiscoverOnAddresses,
                      base::Unretained(this)));
 #else
-  task_tracker_.PostTask(
-      task_runner.get(), FROM_HERE,
-      base::BindOnce(&DialServiceImpl::GetNetworkListOnUIThread,
-                     base::Unretained(this)));
+  task_tracker_.PostTask(task_runner.get(), FROM_HERE,
+                         base::BindOnce(&GetNetworkListOnUIThread,
+                                        weak_ptr_factory_.GetWeakPtr()));
 #endif
 }
 
-void DialServiceImpl::GetNetworkListOnUIThread() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  content::GetNetworkService()->GetNetworkList(
-      net::INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES,
-      base::BindOnce(&DialServiceImpl::PostSendNetworkList,
-                     weak_ptr_factory_for_ui_.GetWeakPtr()));
-}
-
-void DialServiceImpl::PostSendNetworkList(
-    const base::Optional<net::NetworkInterfaceList>& networks) {
-  if (!networks.has_value())
-    VLOG(1) << "Could not retrieve network list!";
-
-  base::PostTaskWithTraits(
-      FROM_HERE, {BrowserThread::IO},
-      base::BindOnce(
-          &DialServiceImpl::SendNetworkList,
-          weak_ptr_factory_for_io_.GetWeakPtr(),
-          networks.has_value() ? *networks : net::NetworkInterfaceList()));
-}
-
-void DialServiceImpl::SendNetworkList(const NetworkInterfaceList& networks) {
+void DialServiceImpl::SendNetworkList(
+    const base::Optional<NetworkInterfaceList>& networks) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   using InterfaceIndexAddressFamily = std::pair<uint32_t, net::AddressFamily>;
   std::set<InterfaceIndexAddressFamily> interface_index_addr_family_seen;
   net::IPAddressList ip_addresses;
 
-  // Binds a socket to each IPv4 network interface found. Note that
-  // there may be duplicates in |networks|, so address family + interface index
-  // is used to identify unique interfaces.
-  // TODO(mfoltz): Support IPV6 multicast.  http://crbug.com/165286
-  for (const auto& network : networks) {
-    net::AddressFamily addr_family = net::GetAddressFamily(network.address);
-    VLOG(2) << "Found " << network.name << ", " << network.address.ToString()
-            << ", address family: " << addr_family;
-    if (addr_family == net::ADDRESS_FAMILY_IPV4) {
-      InterfaceIndexAddressFamily interface_index_addr_family =
-          std::make_pair(network.interface_index, addr_family);
-      bool inserted =
-          interface_index_addr_family_seen.insert(interface_index_addr_family)
-              .second;
-      // We have not seen this interface before, so add its IP address to the
-      // discovery list.
-      if (inserted) {
-        VLOG(2) << "Encountered "
-                << "interface index: " << network.interface_index << ", "
-                << "address family: " << addr_family << " for the first time, "
-                << "adding IP address " << network.address.ToString()
-                << " to list.";
-        ip_addresses.push_back(network.address);
-      } else {
-        VLOG(2) << "Already encountered "
-                << "interface index: " << network.interface_index << ", "
-                << "address family: " << addr_family << " before, not adding.";
+  if (networks.has_value()) {
+    // Binds a socket to each IPv4 network interface found. Note that
+    // there may be duplicates in |networks|, so address family + interface
+    // index is used to identify unique interfaces.
+    // TODO(mfoltz): Support IPV6 multicast.  http://crbug.com/165286
+    for (const auto& network : *networks) {
+      net::AddressFamily addr_family = net::GetAddressFamily(network.address);
+      VLOG(2) << "Found " << network.name << ", " << network.address.ToString()
+              << ", address family: " << addr_family;
+      if (addr_family == net::ADDRESS_FAMILY_IPV4) {
+        InterfaceIndexAddressFamily interface_index_addr_family =
+            std::make_pair(network.interface_index, addr_family);
+        bool inserted =
+            interface_index_addr_family_seen.insert(interface_index_addr_family)
+                .second;
+        // We have not seen this interface before, so add its IP address to the
+        // discovery list.
+        if (inserted) {
+          VLOG(2) << "Encountered "
+                  << "interface index: " << network.interface_index << ", "
+                  << "address family: " << addr_family
+                  << " for the first time, "
+                  << "adding IP address " << network.address.ToString()
+                  << " to list.";
+          ip_addresses.push_back(network.address);
+        } else {
+          VLOG(2) << "Already encountered "
+                  << "interface index: " << network.interface_index << ", "
+                  << "address family: " << addr_family
+                  << " before, not adding.";
+        }
       }
     }
+  } else {
+    VLOG(1) << "Could not retrieve network list!";
   }
 
   DiscoverOnAddresses(ip_addresses);

chrome/browser/media/router/discovery/dial/dial_service.h

@@ -21,7 +21,7 @@ namespace net {
 class IPEndPoint;
 class StringIOBuffer;
 class NetLog;
-}
+}  // namespace net
 
 namespace media_router {
 
@@ -106,6 +106,10 @@ class DialServiceImpl : public DialService {
   bool HasObserver(const Observer* observer) const override;
 
  private:
+  friend void PostSendNetworkList(
+      base::WeakPtr<DialServiceImpl> impl,
+      const base::Optional<net::NetworkInterfaceList>& networks);
+
   // Represents a socket binding to a single network interface.
   // DialSocket lives on the IO thread.
   class DialSocket {
@@ -185,16 +189,9 @@ class DialServiceImpl : public DialService {
   // Starts the control flow for one discovery cycle.
   void StartDiscovery();
 
-  // Task to retrieve networks on UI thread.
-  void GetNetworkListOnUIThread();
-
-  // Callback invoked to send retrieved networks on IO thread.
-  void PostSendNetworkList(
-      const base::Optional<net::NetworkInterfaceList>& networks);
-
   // For each network interface in |list|, finds all unqiue IPv4 network
   // interfaces and call |DiscoverOnAddresses()| with their IP addresses.
-  void SendNetworkList(const net::NetworkInterfaceList& list);
+  void SendNetworkList(const base::Optional<net::NetworkInterfaceList>& list);
 
   // Calls |BindAndAddSocket()| for each address in |ip_addresses|, calls
   // |SendOneRequest()|, and start the timer to finish discovery if needed.
@@ -269,11 +266,8 @@ class DialServiceImpl : public DialService {
 
   base::CancelableTaskTracker task_tracker_;
 
-  // WeakPrtFactory for WeakPtrs that are invalidated on UI thread.
-  base::WeakPtrFactory<DialServiceImpl> weak_ptr_factory_for_ui_{this};
-
-  // WeakPrtFactory for WeakPtrs that are invalidated on IO thread.
-  base::WeakPtrFactory<DialServiceImpl> weak_ptr_factory_for_io_{this};
+  // WeakPtrFactory for WeakPtrs that are invalidated on IO thread.
+  base::WeakPtrFactory<DialServiceImpl> weak_ptr_factory_{this};
 
   friend class DialServiceTest;
   FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestSendMultipleRequests);