Fix a race in the creation of HidConnectionLinux::Helper.

The creation of this object on the FILE thread races with the possible
destruction of the object on the UI thread. The scoped_ptr<> that owns
the pointer may try to destroy the object on the UI thread if the
connection is closed before the helper is started. Generally, updating
the helper_ pointer from the FILE thread is a bad idea.

This patch moves creation of the helper to the UI thread, it is then
detached from that thread when it is started. The HidConnectionLinux no
longer uses a scoped_ptr<> to hold it and instead must always free it
explicitly with DeleteSoon.

BUG=

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

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

device/hid/hid_connection_linux.cc

@@ -38,19 +38,26 @@ class HidConnectionLinux::Helper : public base::MessagePumpLibevent::Watcher {
       : platform_file_(platform_file),
         connection_(connection),
         task_runner_(task_runner) {
-    if (!base::MessageLoopForIO::current()->WatchFileDescriptor(
-            platform_file_, true, base::MessageLoopForIO::WATCH_READ,
-            &file_watcher_, this)) {
-      LOG(ERROR) << "Failed to start watching device file.";
-    }
-
     // Report buffers must always have room for the report ID.
     report_buffer_size_ = device_info.max_input_report_size + 1;
     has_report_id_ = device_info.has_report_id;
   }
 
   virtual ~Helper() { DCHECK(thread_checker_.CalledOnValidThread()); }
 
+  // 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() {
+    base::ThreadRestrictions::AssertIOAllowed();
+    thread_checker_.DetachFromThread();
+    if (!base::MessageLoopForIO::current()->WatchFileDescriptor(
+            platform_file_, true, base::MessageLoopForIO::WATCH_READ,
+            &file_watcher_, this)) {
+      LOG(ERROR) << "Failed to start watching device file.";
+    }
+  }
+
  private:
   // base::MessagePumpLibevent::Watcher implementation.
   void OnFileCanReadWithoutBlocking(int fd) override {
@@ -99,7 +106,7 @@ class HidConnectionLinux::Helper : public base::MessagePumpLibevent::Watcher {
 };
 
 HidConnectionLinux::HidConnectionLinux(
-    HidDeviceInfo device_info,
+    const HidDeviceInfo& device_info,
     base::File device_file,
     scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
     : HidConnection(device_info),
@@ -109,22 +116,24 @@ HidConnectionLinux::HidConnectionLinux(
   device_file_ = device_file.Pass();
 
   // The helper is passed a weak pointer to this connection so that it can be
-  // cleaned up after the connection is closed. The weak pointer must be
-  // constructed on this thread.
-  file_task_runner_->PostTask(FROM_HERE,
-                              base::Bind(&HidConnectionLinux::StartHelper, this,
-                                         weak_factory_.GetWeakPtr()));
+  // cleaned up after the connection is closed.
+  helper_ = new Helper(device_file_.GetPlatformFile(), device_info,
+                       weak_factory_.GetWeakPtr(), task_runner_);
+  file_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&Helper::Start, base::Unretained(helper_)));
 }
 
 HidConnectionLinux::~HidConnectionLinux() {
+  DCHECK(helper_ == nullptr);
 }
 
 void HidConnectionLinux::PlatformClose() {
   // By closing the device file on the FILE thread (1) the requirement that
   // base::File::Close is called on a thread where I/O is allowed is satisfied
   // and (2) any tasks posted to this task runner that refer to this file will
   // complete before it is closed.
-  file_task_runner_->DeleteSoon(FROM_HERE, helper_.release());
+  file_task_runner_->DeleteSoon(FROM_HERE, helper_);
+  helper_ = nullptr;
   file_task_runner_->PostTask(FROM_HERE,
                               base::Bind(&HidConnectionLinux::CloseDevice,
                                          base::Passed(&device_file_)));
@@ -237,14 +246,6 @@ void HidConnectionLinux::FinishSendFeatureReport(const WriteCallback& callback,
   }
 }
 
-void HidConnectionLinux::StartHelper(
-    base::WeakPtr<HidConnectionLinux> weak_ptr) {
-  base::ThreadRestrictions::AssertIOAllowed();
-
-  helper_.reset(new Helper(device_file_.GetPlatformFile(), device_info(),
-                           weak_ptr, task_runner_));
-}
-
 // static
 void HidConnectionLinux::BlockingWrite(
     base::PlatformFile platform_file,

device/hid/hid_connection_linux.h

@@ -20,7 +20,7 @@ namespace device {
 class HidConnectionLinux : public HidConnection {
  public:
   HidConnectionLinux(
-      HidDeviceInfo device_info,
+      const HidDeviceInfo& device_info,
       base::File device_file,
       scoped_refptr<base::SingleThreadTaskRunner> file_thread_runner);
 
@@ -56,10 +56,6 @@ class HidConnectionLinux : public HidConnection {
                               int result);
   void FinishSendFeatureReport(const WriteCallback& callback, int result);
 
-  // Starts the FileDescriptorWatcher that reads input events from the device.
-  // Must be called on a thread that has a base::MessageLoopForIO.
-  void StartHelper(base::WeakPtr<HidConnectionLinux> weak_ptr);
-
   // Writes to the device. This operation may block.
   static void BlockingWrite(
       base::PlatformFile platform_file,
@@ -82,7 +78,7 @@ class HidConnectionLinux : public HidConnection {
   void ProcessReadQueue();
 
   base::File device_file_;
-  scoped_ptr<Helper> helper_;
+  Helper* helper_;
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;