[DIAL] DialServiceImpl / DialSocket cleanups.

- Replace 2 instances of WeakPtr usage in DialServiceImpl with a
CancelableTaskTracker to track thread-hopping calls to obtain network
interfaces.
- Replace the callbacks bound to DialServiceImpl using WeakPtr passed
to DialSocket with a raw pointer DialServiceImpl. This is safe because
DialServiceImpl owns DialSockets.
- Remove WeakPtrFactory from DialServiceImpl.

Bug: 735590
Change-Id: I4965a33a7a706b057ee7d0f4c5cb6fb98b06dbc2
Reviewed-on: https://chromium-review.googlesource.com/574993Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487595}

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

@@ -163,16 +163,10 @@ NetworkInterfaceList GetNetworkList() {
 
 }  // namespace
 
-DialServiceImpl::DialSocket::DialSocket(
-    const base::Closure& discovery_request_cb,
-    const base::Callback<void(const DialDeviceData&)>& device_discovered_cb,
-    const base::Closure& on_error_cb)
-    : discovery_request_cb_(discovery_request_cb),
-      device_discovered_cb_(device_discovered_cb),
-      on_error_cb_(on_error_cb),
-      is_writing_(false),
-      is_reading_(false) {
+DialServiceImpl::DialSocket::DialSocket(DialServiceImpl* dial_service)
+    : is_writing_(false), is_reading_(false), dial_service_(dial_service) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(dial_service_);
 }
 
 DialServiceImpl::DialSocket::~DialSocket() {
@@ -243,7 +237,7 @@ bool DialServiceImpl::DialSocket::CheckResult(const char* operation,
     Close();
     std::string error_str(net::ErrorToString(result));
     VLOG(1) << "dial socket error: " << error_str;
-    on_error_cb_.Run();
+    dial_service_->NotifyOnError();
     return false;
   }
   return true;
@@ -266,7 +260,7 @@ void DialServiceImpl::DialSocket::OnSocketWrite(int send_buffer_size,
     VLOG(1) << "Sent " << result << " chars, expected " << send_buffer_size
             << " chars";
   }
-  discovery_request_cb_.Run();
+  dial_service_->NotifyOnDiscoveryRequest();
 }
 
 bool DialServiceImpl::DialSocket::ReadSocket() {
@@ -328,7 +322,7 @@ void DialServiceImpl::DialSocket::HandleResponse(int bytes_read) {
   // Attempt to parse response, notify observers if successful.
   DialDeviceData parsed_device;
   if (ParseResponse(response, response_time, &parsed_device))
-    device_discovered_cb_.Run(parsed_device);
+    dial_service_->NotifyOnDeviceDiscovered(parsed_device);
 }
 
 // static
@@ -398,8 +392,7 @@ DialServiceImpl::DialServiceImpl(net::NetLog* net_log)
                                                 kDialRequestIntervalMillis) +
                     TimeDelta::FromSeconds(kDialResponseTimeoutSecs)),
       request_interval_(
-          TimeDelta::FromMilliseconds(kDialRequestIntervalMillis)),
-      weak_factory_(this) {
+          TimeDelta::FromMilliseconds(kDialRequestIntervalMillis)) {
   IPAddress address;
   bool success = address.AssignFromIPLiteral(kDialRequestAddress);
   DCHECK(success);
@@ -453,16 +446,17 @@ void DialServiceImpl::StartDiscovery() {
 #if defined(OS_CHROMEOS)
   auto task_runner =
       content::BrowserThread::GetTaskRunnerForThread(BrowserThread::UI);
-  base::PostTaskAndReplyWithResult(
+  task_tracker_.PostTaskAndReplyWithResult(
       task_runner.get(), FROM_HERE,
       base::BindOnce(&GetBestBindAddressOnUIThread),
       base::BindOnce(&DialServiceImpl::DiscoverOnAddresses,
-                     weak_factory_.GetWeakPtr()));
+                     base::Unretained(this)));
 #else
-  base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()}, base::BindOnce(&GetNetworkList),
+  auto task_runner = base::CreateTaskRunnerWithTraits({base::MayBlock()});
+  task_tracker_.PostTaskAndReplyWithResult(
+      task_runner.get(), FROM_HERE, base::BindOnce(&GetNetworkList),
       base::BindOnce(&DialServiceImpl::SendNetworkList,
-                     weak_factory_.GetWeakPtr()));
+                     base::Unretained(this)));
 #endif
 }
 
@@ -537,12 +531,7 @@ void DialServiceImpl::BindAndAddSocket(const IPAddress& bind_ip_address) {
 
 std::unique_ptr<DialServiceImpl::DialSocket>
 DialServiceImpl::CreateDialSocket() {
-  return base::MakeUnique<DialServiceImpl::DialSocket>(
-      base::Bind(&DialServiceImpl::NotifyOnDiscoveryRequest,
-                 weak_factory_.GetWeakPtr()),
-      base::Bind(&DialServiceImpl::NotifyOnDeviceDiscovered,
-                 weak_factory_.GetWeakPtr()),
-      base::Bind(&DialServiceImpl::NotifyOnError, weak_factory_.GetWeakPtr()));
+  return base::MakeUnique<DialServiceImpl::DialSocket>(this);
 }
 
 void DialServiceImpl::SendOneRequest() {

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

@@ -11,8 +11,8 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/task/cancelable_task_tracker.h"
 #include "base/timer/timer.h"
 #include "net/base/ip_address.h"
 #include "net/log/net_log_source.h"
@@ -111,13 +111,7 @@ class DialServiceImpl : public DialService {
   // DialSocket lives on the IO thread.
   class DialSocket {
    public:
-    // TODO(imcheng): Consider writing a DialSocket::Delegate interface that
-    // declares methods for these callbacks, and taking a ptr to the delegate
-    // here.
-    DialSocket(
-        const base::Closure& discovery_request_cb,
-        const base::Callback<void(const DialDeviceData&)>& device_discovered_cb,
-        const base::Closure& on_error_cb);
+    explicit DialSocket(DialServiceImpl* dial_service);
     ~DialSocket();
 
     // Creates a socket using |net_log| and |net_log_source| and binds it to
@@ -135,6 +129,11 @@ class DialServiceImpl : public DialService {
     bool IsClosed();
 
    private:
+    FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestNotifyOnError);
+    FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestOnDeviceDiscovered);
+    FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestOnDiscoveryRequest);
+    FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestResponseParsing);
+
     // Checks the result of a socket operation.  The name of the socket
     // operation is given by |operation| and the result of the operation is
     // given by |result|. If the result is an error, closes the socket,
@@ -174,25 +173,15 @@ class DialServiceImpl : public DialService {
     // The source of of the last socket read.
     net::IPEndPoint recv_address_;
 
-    // The callback to be invoked when a discovery request was made.
-    base::Closure discovery_request_cb_;
-
-    // The callback to be invoked when a device has been discovered.
-    base::Callback<void(const DialDeviceData&)> device_discovered_cb_;
-
-    // The callback to be invoked when there is an error with socket operations.
-    base::Closure on_error_cb_;
-
     // Marks whether there is an active write callback.
     bool is_writing_;
 
     // Marks whether there is an active read callback.
     bool is_reading_;
 
-    FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestNotifyOnError);
-    FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestOnDeviceDiscovered);
-    FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestOnDiscoveryRequest);
-    FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestResponseParsing);
+    // Pointer to the DialServiceImpl that owns this socket.
+    DialServiceImpl* const dial_service_;
+
     DISALLOW_COPY_AND_ASSIGN(DialSocket);
   };
 
@@ -277,7 +266,7 @@ class DialServiceImpl : public DialService {
   // List of observers.
   base::ObserverList<Observer> observer_list_;
 
-  base::WeakPtrFactory<DialServiceImpl> weak_factory_;
+  base::CancelableTaskTracker task_tracker_;
 
   friend class DialServiceTest;
   FRIEND_TEST_ALL_PREFIXES(DialServiceTest, TestSendMultipleRequests);

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

@@ -15,6 +15,7 @@
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/ip_address.h"
 #include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"
 #include "net/base/network_interfaces.h"
 #include "net/log/test_net_log.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -129,6 +130,12 @@ TEST_F(DialServiceTest, TestOnDiscoveryRequest) {
   dial_socket_->OnSocketWrite(num_bytes, num_bytes);
 }
 
+TEST_F(DialServiceTest, TestNotifyOnError) {
+  EXPECT_CALL(mock_observer_, OnError(A<DialService*>(),
+                                      DialService::DIAL_SERVICE_NO_INTERFACES));
+  dial_socket_->OnSocketWrite(0, net::ERR_CONNECTION_REFUSED);
+}
+
 TEST_F(DialServiceTest, TestOnDeviceDiscovered) {
   dial_service_.discovery_active_ = true;
   int response_size = arraysize(kValidResponse) - 1;