Make PrivetTrafficDetector live on the UI thread.

Split all IO thread code into a separate helper class. Now there is no
refcounting, so ownership is clearer. Now only the helper class has a
WeakPtrFactory, so WeakPtrs are only used on the IO thread.

BUG=873529

Change-Id: I326aa6f69839c193507df4c63250aa1560bbd0ef
Reviewed-on: https://chromium-review.googlesource.com/1177444
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Robbie McElrath <rmcelrath@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583813}

chrome/browser/printing/cloud_print/privet_notifications.cc

@@ -210,10 +210,6 @@ PrivetNotificationService::PrivetNotificationService(
 
 PrivetNotificationService::~PrivetNotificationService() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-#if BUILDFLAG(ENABLE_MDNS)
-  if (traffic_detector_)
-    traffic_detector_->Stop();
-#endif
 }
 
 void PrivetNotificationService::DeviceChanged(
@@ -329,18 +325,15 @@ void PrivetNotificationService::Start() {
 
 void PrivetNotificationService::OnNotificationsEnabledChanged() {
 #if BUILDFLAG(ENABLE_MDNS)
-  if (traffic_detector_)
-    traffic_detector_->Stop();
-  traffic_detector_ = nullptr;
+  traffic_detector_.reset();
 
   if (IsForced()) {
     StartLister();
   } else if (*enable_privet_notification_member_) {
     ReportPrivetUmaEvent(PRIVET_SERVICE_STARTED);
-    traffic_detector_ = base::MakeRefCounted<PrivetTrafficDetector>(
+    traffic_detector_ = std::make_unique<PrivetTrafficDetector>(
         profile_, base::BindRepeating(&PrivetNotificationService::StartLister,
                                       AsWeakPtr()));
-    traffic_detector_->Start();
   } else {
     device_lister_.reset();
     service_discovery_client_ = nullptr;

chrome/browser/printing/cloud_print/privet_notifications.h

@@ -140,7 +140,7 @@ class PrivetNotificationService
   BooleanPrefMember enable_privet_notification_member_;
 
 #if BUILDFLAG(ENABLE_MDNS)
-  scoped_refptr<PrivetTrafficDetector> traffic_detector_;
+  std::unique_ptr<PrivetTrafficDetector> traffic_detector_;
 #endif
 };
 

chrome/browser/printing/cloud_print/privet_traffic_detector.cc

@@ -7,12 +7,12 @@
 #include <utility>
 
 #include "base/metrics/histogram_macros.h"
-#include "base/single_thread_task_runner.h"
 #include "base/sys_byteorder.h"
 #include "base/task/post_task.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/network_service_instance.h"
 #include "content/public/browser/storage_partition.h"
 #include "net/base/address_family.h"
@@ -73,33 +73,47 @@ namespace cloud_print {
 PrivetTrafficDetector::PrivetTrafficDetector(
     content::BrowserContext* profile,
     const base::RepeatingClosure& on_traffic_detected)
-    : on_traffic_detected_(on_traffic_detected),
-      restart_attempts_(kMaxRestartAttempts),
-      receiver_binding_(this),
-      profile_(profile),
-      weak_ptr_factory_(this) {
+    : helper_(new Helper(profile, on_traffic_detected)) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&PrivetTrafficDetector::Helper::ScheduleRestart,
+                     base::Unretained(helper_)));
 }
 
 PrivetTrafficDetector::~PrivetTrafficDetector() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this);
+  content::BrowserThread::DeleteSoon(content::BrowserThread::IO, FROM_HERE,
+                                     helper_);
 }
 
-void PrivetTrafficDetector::Start() {
+void PrivetTrafficDetector::OnConnectionChanged(
+    network::mojom::ConnectionType type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this);
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&PrivetTrafficDetector::ScheduleRestart,
-                     weak_ptr_factory_.GetWeakPtr()));
+      base::BindOnce(&PrivetTrafficDetector::Helper::HandleConnectionChanged,
+                     base::Unretained(helper_), type));
 }
 
-void PrivetTrafficDetector::Stop() {
+PrivetTrafficDetector::Helper::Helper(
+    content::BrowserContext* profile,
+    const base::RepeatingClosure& on_traffic_detected)
+    : profile_(profile),
+      on_traffic_detected_(on_traffic_detected),
+      restart_attempts_(kMaxRestartAttempts),
+      receiver_binding_(this),
+      weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this);
 }
 
-void PrivetTrafficDetector::HandleConnectionChanged(
+PrivetTrafficDetector::Helper::~Helper() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+}
+
+void PrivetTrafficDetector::Helper::HandleConnectionChanged(
     network::mojom::ConnectionType type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   restart_attempts_ = kMaxRestartAttempts;
@@ -108,25 +122,26 @@ void PrivetTrafficDetector::HandleConnectionChanged(
   }
 }
 
-void PrivetTrafficDetector::ScheduleRestart() {
+void PrivetTrafficDetector::Helper::ScheduleRestart() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   ResetConnection();
   weak_ptr_factory_.InvalidateWeakPtrs();
   base::PostDelayedTaskWithTraits(
       FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()},
-      base::BindOnce(&GetNetworkListInBackground,
-                     base::BindOnce(&PrivetTrafficDetector::Restart,
-                                    weak_ptr_factory_.GetWeakPtr())),
+      base::BindOnce(
+          &GetNetworkListInBackground,
+          base::BindOnce(&Helper::Restart, weak_ptr_factory_.GetWeakPtr())),
       base::TimeDelta::FromSeconds(3));
 }
 
-void PrivetTrafficDetector::Restart(net::NetworkInterfaceList networks) {
+void PrivetTrafficDetector::Helper::Restart(
+    net::NetworkInterfaceList networks) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   networks_ = std::move(networks);
   Bind();
 }
 
-void PrivetTrafficDetector::Bind() {
+void PrivetTrafficDetector::Helper::Bind() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   if (!start_time_.is_null()) {
     base::TimeDelta time_delta = base::Time::Now() - start_time_;
@@ -155,19 +170,19 @@ void PrivetTrafficDetector::Bind() {
   socket_options->multicast_loopback_mode = false;
 
   socket_->Bind(bind_endpoint, std::move(socket_options),
-                base::BindOnce(&PrivetTrafficDetector::OnBindComplete, this,
-                               multicast_addr));
+                base::BindOnce(&Helper::OnBindComplete,
+                               weak_ptr_factory_.GetWeakPtr(), multicast_addr));
 }
 
-void PrivetTrafficDetector::OnBindComplete(
+void PrivetTrafficDetector::Helper::OnBindComplete(
     net::IPEndPoint multicast_addr,
     int rv,
     const base::Optional<net::IPEndPoint>& ip_endpoint) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   if (rv == net::OK) {
-    socket_->JoinGroup(
-        multicast_addr.address(),
-        base::BindOnce(&PrivetTrafficDetector::OnJoinGroupComplete, this));
+    socket_->JoinGroup(multicast_addr.address(),
+                       base::BindOnce(&Helper::OnJoinGroupComplete,
+                                      weak_ptr_factory_.GetWeakPtr()));
     return;
   }
 
@@ -175,7 +190,7 @@ void PrivetTrafficDetector::OnBindComplete(
     ScheduleRestart();
 }
 
-bool PrivetTrafficDetector::IsSourceAcceptable() const {
+bool PrivetTrafficDetector::Helper::IsSourceAcceptable() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   for (const auto& network : networks_) {
     if (net::IPAddressMatchesPrefix(recv_addr_.address(), network.address,
@@ -186,7 +201,7 @@ bool PrivetTrafficDetector::IsSourceAcceptable() const {
   return false;
 }
 
-bool PrivetTrafficDetector::IsPrivetPacket(
+bool PrivetTrafficDetector::Helper::IsPrivetPacket(
     base::span<const uint8_t> data) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   if (data.size() <= sizeof(net::dns_protocol::Header) ||
@@ -211,7 +226,7 @@ bool PrivetTrafficDetector::IsPrivetPacket(
                      substring_end) != buffer_end;
 }
 
-void PrivetTrafficDetector::OnJoinGroupComplete(int rv) {
+void PrivetTrafficDetector::Helper::OnJoinGroupComplete(int rv) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   if (rv == net::OK) {
     // Reset on success.
@@ -224,22 +239,13 @@ void PrivetTrafficDetector::OnJoinGroupComplete(int rv) {
     ScheduleRestart();
 }
 
-void PrivetTrafficDetector::ResetConnection() {
+void PrivetTrafficDetector::Helper::ResetConnection() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   socket_.reset();
   receiver_binding_.Close();
 }
 
-void PrivetTrafficDetector::OnConnectionChanged(
-    network::mojom::ConnectionType type) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&PrivetTrafficDetector::HandleConnectionChanged,
-                     weak_ptr_factory_.GetWeakPtr(), type));
-}
-
-void PrivetTrafficDetector::OnReceived(
+void PrivetTrafficDetector::Helper::OnReceived(
     int32_t result,
     const base::Optional<net::IPEndPoint>& src_addr,
     base::Optional<base::span<const uint8_t>> data) {

chrome/browser/printing/cloud_print/privet_traffic_detector.h

@@ -9,7 +9,6 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
-#include "content/public/browser/browser_thread.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "net/base/ip_endpoint.h"
 #include "services/network/public/cpp/network_connection_tracker.h"
@@ -28,69 +27,70 @@ namespace cloud_print {
 // listening for traffic.
 // When the network changes, restarts itself to start listening for traffic
 // again on the new network(s).
+// The class lives on the UI thread, with a helper that lives on the IO thread.
 class PrivetTrafficDetector
-    : public base::RefCountedThreadSafe<
-          PrivetTrafficDetector,
-          content::BrowserThread::DeleteOnIOThread>,
-      private network::NetworkConnectionTracker::NetworkConnectionObserver,
-      public network::mojom::UDPSocketReceiver {
+    : public network::NetworkConnectionTracker::NetworkConnectionObserver {
  public:
   // Called on the UI thread.
   PrivetTrafficDetector(content::BrowserContext* profile,
                         const base::RepeatingClosure& on_traffic_detected);
-
-  // Called on the UI thread.
-  void Start();
-  void Stop();
-
- private:
-  friend struct content::BrowserThread::DeleteOnThread<
-      content::BrowserThread::IO>;
-  friend class base::DeleteHelper<PrivetTrafficDetector>;
-
   ~PrivetTrafficDetector() override;
 
-  // Unless otherwise noted, all methods are called on the IO thread.
-  void HandleConnectionChanged(network::mojom::ConnectionType type);
-  void ScheduleRestart();
-  void Restart(net::NetworkInterfaceList networks);
-  void Bind();
-  void OnBindComplete(net::IPEndPoint multicast_addr,
-                      int rv,
-                      const base::Optional<net::IPEndPoint>& ip_address);
-  bool IsSourceAcceptable() const;
-  bool IsPrivetPacket(base::span<const uint8_t> data) const;
-  void OnJoinGroupComplete(int rv);
-  void ResetConnection();
-
   // network::NetworkConnectionTracker::NetworkConnectionObserver:
-  // Called on the UI thread.
   void OnConnectionChanged(network::mojom::ConnectionType type) override;
 
-  // network::mojom::UDPSocketReceiver implementation
-  void OnReceived(int32_t result,
-                  const base::Optional<net::IPEndPoint>& src_addr,
-                  base::Optional<base::span<const uint8_t>> data) override;
-
-  // Initialized on the UI thread, but only accessed on the IO thread.
-  base::RepeatingClosure on_traffic_detected_;
-  int restart_attempts_;
-
-  // Only accessed on the IO thread.
-  net::NetworkInterfaceList networks_;
-  net::IPEndPoint recv_addr_;
-  base::Time start_time_;
-  network::mojom::UDPSocketPtr socket_;
-
-  // Implementation of socket receiver callback.
-  // Initialized on the UI thread, but only accessed on the IO thread.
-  mojo::Binding<network::mojom::UDPSocketReceiver> receiver_binding_;
-
-  // Initialized on the UI thread, but only accessed on the IO thread for the
-  // purpose of passing it back to the UI thread. Safe because it is const.
-  content::BrowserContext* const profile_;
-
-  base::WeakPtrFactory<PrivetTrafficDetector> weak_ptr_factory_;
+ private:
+  // Constructed by PrivetTrafficDetector on the UI thread. but lives on the IO
+  // thread and destroyed on the IO thread.
+  class Helper : public network::mojom::UDPSocketReceiver {
+   public:
+    Helper(content::BrowserContext* profile,
+           const base::RepeatingClosure& on_traffic_detected);
+    ~Helper() override;
+
+    // network::mojom::UDPSocketReceiver:
+    void OnReceived(int32_t result,
+                    const base::Optional<net::IPEndPoint>& src_addr,
+                    base::Optional<base::span<const uint8_t>> data) override;
+
+    void HandleConnectionChanged(network::mojom::ConnectionType type);
+    void ScheduleRestart();
+
+   private:
+    void Restart(net::NetworkInterfaceList networks);
+    void Bind();
+    void OnBindComplete(net::IPEndPoint multicast_addr,
+                        int rv,
+                        const base::Optional<net::IPEndPoint>& ip_address);
+    bool IsSourceAcceptable() const;
+    bool IsPrivetPacket(base::span<const uint8_t> data) const;
+    void OnJoinGroupComplete(int rv);
+    void ResetConnection();
+
+    // Initialized on the UI thread, but only accessed on the IO thread for the
+    // purpose of passing it back to the UI thread. Safe because it is const.
+    content::BrowserContext* const profile_;
+
+    // Initialized on the UI thread, but only accessed on the IO thread.
+    base::RepeatingClosure on_traffic_detected_;
+    int restart_attempts_;
+
+    // Only accessed on the IO thread.
+    net::NetworkInterfaceList networks_;
+    net::IPEndPoint recv_addr_;
+    base::Time start_time_;
+    network::mojom::UDPSocketPtr socket_;
+
+    // Implementation of socket receiver callback.
+    // Initialized on the UI thread, but only accessed on the IO thread.
+    mojo::Binding<network::mojom::UDPSocketReceiver> receiver_binding_;
+
+    base::WeakPtrFactory<Helper> weak_ptr_factory_;
+
+    DISALLOW_COPY_AND_ASSIGN(Helper);
+  };
+
+  Helper* const helper_;
 
   DISALLOW_COPY_AND_ASSIGN(PrivetTrafficDetector);
 };