Fix Linux HID service and connection FileThreadHelper lifetimes.

Both HidServiceLinux and HidConnectionLinux need a helper class that is
owned by the FILE thread so that they can perform possibly blocking I/O
operations.

At some point HidServiceLinux::FileThreadHelper's lifetime became both
managed by the lifetime of the FILE thread message loop (as it was a
MessageLoop::DestructionObserver) and manually managed by the
HidServiceLinux that created it. Neither mechanism actually worked
because a) the helper was never actually added as a message loop
destruction observer and b) the |helper_| field of the HidServiceLinux
was never populated, so the helper would just leak.

The ideal solution is that these helpers must both managed by the
lifetime of the object they support, and also observe the lifetime of
the message loop on which they run.

Review URL: https://codereview.chromium.org/1304073005

Cr-Commit-Position: refs/heads/master@{#345388}

5 files changed, 54 insertions, 44 deletions

diff --git a/device/hid/device_monitor_linux.h b/device/hid/device_monitor_linux.h
index d05e31b..824fa65 100644
--- a/device/hid/device_monitor_linux.h
+++ b/device/hid/device_monitor_linux.h
@@ -63,7 +63,7 @@ class DeviceMonitorLinux : public base::MessageLoop::DestructionObserver,
   int monitor_fd_;
   base::MessagePumpLibevent::FileDescriptorWatcher monitor_watcher_;
 
-  base::ObserverList<Observer> observers_;
+  base::ObserverList<Observer, true> observers_;
 
   base::ThreadChecker thread_checker_;
 
diff --git a/device/hid/hid_connection_linux.cc b/device/hid/hid_connection_linux.cc
index b2a2fc5..ec47bf7 100644
--- a/device/hid/hid_connection_linux.cc
+++ b/device/hid/hid_connection_linux.cc
@@ -31,7 +31,8 @@
 namespace device {
 
 class HidConnectionLinux::FileThreadHelper
-    : public base::MessagePumpLibevent::Watcher {
+    : public base::MessagePumpLibevent::Watcher,
+      public base::MessageLoop::DestructionObserver {
  public:
   FileThreadHelper(base::PlatformFile platform_file,
                    scoped_refptr<HidDeviceInfo> device_info,
@@ -47,19 +48,23 @@ class HidConnectionLinux::FileThreadHelper
 
   ~FileThreadHelper() override {
     DCHECK(thread_checker_.CalledOnValidThread());
+    base::MessageLoop::current()->RemoveDestructionObserver(this);
   }
 
   // Starts the FileDescriptorWatcher that reads input events from the device.
-  // Must be called on a thread that has a base::MessageLoopForIO. The helper
-  // object is owned by the thread where it was started.
-  void Start() {
+  // Must be called on a thread that has a base::MessageLoopForIO.
+  static void Start(scoped_ptr<FileThreadHelper> self) {
     base::ThreadRestrictions::AssertIOAllowed();
-    thread_checker_.DetachFromThread();
+    self->thread_checker_.DetachFromThread();
+
     if (!base::MessageLoopForIO::current()->WatchFileDescriptor(
-            platform_file_, true, base::MessageLoopForIO::WATCH_READ,
-            &file_watcher_, this)) {
+            self->platform_file_, true, base::MessageLoopForIO::WATCH_READ,
+            &self->file_watcher_, self.get())) {
       HID_LOG(ERROR) << "Failed to start watching device file.";
     }
+
+    // |self| is now owned by the current message loop.
+    base::MessageLoop::current()->AddDestructionObserver(self.release());
   }
 
  private:
@@ -105,6 +110,12 @@ class HidConnectionLinux::FileThreadHelper
     NOTREACHED();  // Only listening for reads.
   }
 
+  // base::MessageLoop::DestructionObserver:
+  void WillDestroyCurrentMessageLoop() override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    delete this;
+  }
+
   base::ThreadChecker thread_checker_;
   base::PlatformFile platform_file_;
   size_t report_buffer_size_;
@@ -112,6 +123,8 @@ class HidConnectionLinux::FileThreadHelper
   base::WeakPtr<HidConnectionLinux> connection_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   base::MessagePumpLibevent::FileDescriptorWatcher file_watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(FileThreadHelper);
 };
 
 HidConnectionLinux::HidConnectionLinux(
@@ -126,10 +139,12 @@ HidConnectionLinux::HidConnectionLinux(
 
   // The helper is passed a weak pointer to this connection so that it can be
   // cleaned up after the connection is closed.
-  helper_ = new FileThreadHelper(device_file_.GetPlatformFile(), device_info,
-                                 weak_factory_.GetWeakPtr(), task_runner_);
-  file_task_runner_->PostTask(FROM_HERE, base::Bind(&FileThreadHelper::Start,
-                                                    base::Unretained(helper_)));
+  scoped_ptr<FileThreadHelper> helper(
+      new FileThreadHelper(device_file_.GetPlatformFile(), device_info,
+                           weak_factory_.GetWeakPtr(), task_runner_));
+  helper_ = helper.get();
+  file_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&FileThreadHelper::Start, base::Passed(&helper)));
 }
 
 HidConnectionLinux::~HidConnectionLinux() {
diff --git a/device/hid/hid_service.h b/device/hid/hid_service.h
index 1634a64..811d26f 100644
--- a/device/hid/hid_service.h
+++ b/device/hid/hid_service.h
@@ -89,7 +89,7 @@ class HidService {
   DeviceMap devices_;
   bool enumeration_ready_;
   std::vector<GetDevicesCallback> pending_enumerations_;
-  base::ObserverList<Observer> observer_list_;
+  base::ObserverList<Observer, true> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(HidService);
 };
diff --git a/device/hid/hid_service_linux.cc b/device/hid/hid_service_linux.cc
index 7d1e26b..e6e09f2 100644
--- a/device/hid/hid_service_linux.cc
+++ b/device/hid/hid_service_linux.cc
@@ -67,18 +67,27 @@ class HidServiceLinux::FileThreadHelper
  public:
   FileThreadHelper(base::WeakPtr<HidServiceLinux> service,
                    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-      : observer_(this), service_(service), task_runner_(task_runner) {
-    DeviceMonitorLinux* monitor = DeviceMonitorLinux::GetInstance();
-    observer_.Add(monitor);
-    monitor->Enumerate(
-        base::Bind(&FileThreadHelper::OnDeviceAdded, base::Unretained(this)));
-    task_runner->PostTask(
-        FROM_HERE,
-        base::Bind(&HidServiceLinux::FirstEnumerationComplete, service_));
-  }
+      : observer_(this), service_(service), task_runner_(task_runner) {}
 
   ~FileThreadHelper() override {
     DCHECK(thread_checker_.CalledOnValidThread());
+    base::MessageLoop::current()->RemoveDestructionObserver(this);
+  }
+
+  static void Start(scoped_ptr<FileThreadHelper> self) {
+    base::ThreadRestrictions::AssertIOAllowed();
+    self->thread_checker_.DetachFromThread();
+
+    DeviceMonitorLinux* monitor = DeviceMonitorLinux::GetInstance();
+    self->observer_.Add(monitor);
+    monitor->Enumerate(base::Bind(&FileThreadHelper::OnDeviceAdded,
+                                  base::Unretained(self.get())));
+    self->task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&HidServiceLinux::FirstEnumerationComplete, self->service_));
+
+    // |self| is now owned by the current message loop.
+    base::MessageLoop::current()->AddDestructionObserver(self.release());
   }
 
  private:
@@ -179,7 +188,6 @@ class HidServiceLinux::FileThreadHelper
   // base::MessageLoop::DestructionObserver:
   void WillDestroyCurrentMessageLoop() override {
     DCHECK(thread_checker_.CalledOnValidThread());
-    base::MessageLoop::current()->RemoveDestructionObserver(this);
     delete this;
   }
 
@@ -189,27 +197,19 @@ class HidServiceLinux::FileThreadHelper
   // This weak pointer is only valid when checked on this task runner.
   base::WeakPtr<HidServiceLinux> service_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(FileThreadHelper);
 };
 
 HidServiceLinux::HidServiceLinux(
     scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
     : file_task_runner_(file_task_runner), weak_factory_(this) {
   task_runner_ = base::ThreadTaskRunnerHandle::Get();
-  // The device watcher is passed a weak pointer back to this service so that it
-  // can be cleaned up after the service is destroyed however this weak pointer
-  // must be constructed on the this thread where it will be checked.
+  scoped_ptr<FileThreadHelper> helper(
+      new FileThreadHelper(weak_factory_.GetWeakPtr(), task_runner_));
+  helper_ = helper.get();
   file_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&HidServiceLinux::StartHelper,
-                            weak_factory_.GetWeakPtr(), task_runner_));
-}
-
-// static
-void HidServiceLinux::StartHelper(
-    base::WeakPtr<HidServiceLinux> weak_ptr,
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  // Helper is a message loop destruction observer and will delete itself when
-  // this thread's message loop is destroyed.
-  new FileThreadHelper(weak_ptr, task_runner);
+      FROM_HERE, base::Bind(&FileThreadHelper::Start, base::Passed(&helper)));
 }
 
 void HidServiceLinux::Connect(const HidDeviceId& device_id,
@@ -242,7 +242,7 @@ void HidServiceLinux::Connect(const HidDeviceId& device_id,
 }
 
 HidServiceLinux::~HidServiceLinux() {
-  file_task_runner_->DeleteSoon(FROM_HERE, helper_.release());
+  file_task_runner_->DeleteSoon(FROM_HERE, helper_);
 }
 
 #if defined(OS_CHROMEOS)
diff --git a/device/hid/hid_service_linux.h b/device/hid/hid_service_linux.h
index 66cfbd3..6bcb1a9 100644
--- a/device/hid/hid_service_linux.h
+++ b/device/hid/hid_service_linux.h
@@ -32,11 +32,6 @@ class HidServiceLinux : public HidService {
 
   ~HidServiceLinux() override;
 
-  // Constructs this services helper object that lives on the FILE thread.
-  static void StartHelper(
-      base::WeakPtr<HidServiceLinux> weak_ptr,
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
-
   // These functions implement the process of locating, requesting access to and
   // opening a device. Because this operation crosses multiple threads these
   // functions are static and the necessary parameters are passed as a single
@@ -57,7 +52,7 @@ class HidServiceLinux : public HidService {
 
   // The helper lives on the FILE thread and holds a weak reference back to the
   // service that owns it.
-  scoped_ptr<FileThreadHelper> helper_;
+  FileThreadHelper* helper_;
   base::WeakPtrFactory<HidServiceLinux> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(HidServiceLinux);