Remove MessageLoop destruction observer from hid_connection_linux.cc

device/hid/hid_connection_linux.cc

Index: device/hid/hid_connection_linux.cc
diff --git a/device/hid/hid_connection_linux.cc b/device/hid/hid_connection_linux.cc
index 5172b0b39ea6a366c22af751e3d48ebf5b933734..27fe5d11bbadd8c050bdcda3a42104d9b60ff99a 100644
--- a/device/hid/hid_connection_linux.cc
+++ b/device/hid/hid_connection_linux.cc
@@ -14,9 +14,7 @@
 
 #include "base/bind.h"
 #include "base/files/file_descriptor_watcher_posix.h"
-#include "base/files/file_path.h"
 #include "base/macros.h"
-#include "base/message_loop/message_loop.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -33,44 +31,90 @@
 
 namespace device {
 
-class HidConnectionLinux::FileThreadHelper
-    : public base::MessageLoop::DestructionObserver {
+class HidConnectionLinux::FileThreadHelper {
  public:
-  FileThreadHelper(base::PlatformFile platform_file,
+  FileThreadHelper(base::ScopedFD fd,
                    scoped_refptr<HidDeviceInfo> device_info,
                    base::WeakPtr<HidConnectionLinux> connection,
                    scoped_refptr<base::SingleThreadTaskRunner> task_runner)

[robliao, 2017/04/07 01:08:24]

Does this have to be a SingleThreadTaskRunner or can SequenceTaskRunner also do
the job (if STR is okay, this change can be separate from this CL).

It seems like it just pulls it from the current thread task runner handle, which
I presume is the UI thread?

[fdoray, 2017/04/07 20:26:23]

s/task_runner/origin_task_runner/?

[Reilly Grant (use Gerrit), 2017/04/07 22:10:48]

In the USB code SingleThreadTaskRunner is necessary for some historical reasons
but here I can switch to SequencedTaskRunner, so I did.

[robliao, 2017/04/07 23:24:45]

Ping for fdoray's comment:
s/task_runner/origin_task_runner/

[Reilly Grant (use Gerrit), 2017/04/08 00:57:02]

Done.

-      : platform_file_(platform_file),
-        connection_(connection),
-        task_runner_(task_runner) {
+      : fd_(std::move(fd)), connection_(connection), task_runner_(task_runner) {
     // 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();
   }
 
-  ~FileThreadHelper() override {
-    DCHECK(thread_checker_.CalledOnValidThread());
-    base::MessageLoop::current()->RemoveDestructionObserver(this);
-  }
+  ~FileThreadHelper() { DCHECK(sequence_checker_.CalledOnValidSequence()); }
 
   // Starts the FileDescriptorWatcher that reads input events from the device.
   // Must be called on a thread that has a base::MessageLoopForIO.
-  static void Start(std::unique_ptr<FileThreadHelper> self) {
+  void Start() {
     base::ThreadRestrictions::AssertIOAllowed();
-    self->thread_checker_.DetachFromThread();
+    sequence_checker_.DetachFromSequence();

[robliao, 2017/04/07 01:08:24]

I think you can move this call to the constructor since all subsequent
FileThreadHelper calls occur on the blocking task runner.

After that, you can DCHECK that we're called on a valid sequence here and
subsequent calls will be checked against this sequence.

[Reilly Grant (use Gerrit), 2017/04/07 22:10:48]

Done.

+
+    file_watcher_ = base::FileDescriptorWatcher::WatchReadable(
+        fd_.get(), base::Bind(&FileThreadHelper::OnFileCanReadWithoutBlocking,
+                              base::Unretained(this)));
+  }
 
-    self->file_watcher_ = base::FileDescriptorWatcher::WatchReadable(
-        self->platform_file_,
-        base::Bind(&FileThreadHelper::OnFileCanReadWithoutBlocking,
-                   base::Unretained(self.get())));
+  void Write(scoped_refptr<net::IOBuffer> buffer,
+             size_t size,
+             const WriteCallback& callback) {
+    DCHECK(sequence_checker_.CalledOnValidSequence());
+    ssize_t result = HANDLE_EINTR(write(fd_.get(), buffer->data(), size));
+    if (result < 0) {
+      HID_PLOG(EVENT) << "Write failed";
+      task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
+    } else {
+      if (static_cast<size_t>(result) != size) {

[fdoray, 2017/04/07 20:26:23]

no braces

[Reilly Grant (use Gerrit), 2017/04/07 22:10:48]

Done.

+        HID_LOG(EVENT) << "Incomplete HID write: " << result << " != " << size;
+      }
+      task_runner_->PostTask(FROM_HERE, base::Bind(callback, true));
+    }
+  }
+
+  void GetFeatureReport(uint8_t report_id,
+                        scoped_refptr<net::IOBufferWithSize> buffer,
+                        const ReadCallback& callback) {
+    DCHECK(sequence_checker_.CalledOnValidSequence());
+    int result = HANDLE_EINTR(
+        ioctl(fd_.get(), HIDIOCGFEATURE(buffer->size()), buffer->data()));
+    if (result < 0) {
+      HID_PLOG(EVENT) << "Failed to get feature report";
+      task_runner_->PostTask(FROM_HERE,
+                             base::Bind(callback, false, nullptr, 0));
+    } else if (result == 0) {
+      HID_LOG(EVENT) << "Get feature result too short.";
+      task_runner_->PostTask(FROM_HERE,
+                             base::Bind(callback, false, nullptr, 0));
+    } else if (report_id == 0) {
+      // Linux adds a 0 to the beginning of the data received from the device.
+      scoped_refptr<net::IOBuffer> copied_buffer(new net::IOBuffer(result - 1));
+      memcpy(copied_buffer->data(), buffer->data() + 1, result - 1);
+      task_runner_->PostTask(
+          FROM_HERE, base::Bind(callback, true, copied_buffer, result - 1));
+    } else {
+      task_runner_->PostTask(FROM_HERE,
+                             base::Bind(callback, true, buffer, result));
+    }
+  }
 
-    // |self| is now owned by the current message loop.
-    base::MessageLoop::current()->AddDestructionObserver(self.release());
+  void SendFeatureReport(scoped_refptr<net::IOBuffer> buffer,
+                         size_t size,
+                         const WriteCallback& callback) {
+    DCHECK(sequence_checker_.CalledOnValidSequence());
+    int result =
+        HANDLE_EINTR(ioctl(fd_.get(), HIDIOCSFEATURE(size), buffer->data()));
+    if (result < 0) {
+      HID_PLOG(EVENT) << "Failed to send feature report";
+      task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
+    } else {
+      task_runner_->PostTask(FROM_HERE, base::Bind(callback, true));
+    }
   }
 
  private:
   void OnFileCanReadWithoutBlocking() {
-    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(sequence_checker_.CalledOnValidSequence());
 
     scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(report_buffer_size_));
     char* data = buffer->data();
@@ -82,7 +126,7 @@ class HidConnectionLinux::FileThreadHelper
       length--;
     }
 
-    ssize_t bytes_read = HANDLE_EINTR(read(platform_file_, data, length));
+    ssize_t bytes_read = HANDLE_EINTR(read(fd_.get(), data, length));
     if (bytes_read < 0) {
       if (errno != EAGAIN) {
         HID_PLOG(EVENT) << "Read failed";
@@ -105,14 +149,8 @@ class HidConnectionLinux::FileThreadHelper
                                       connection_, buffer, bytes_read));
   }
 
-  // base::MessageLoop::DestructionObserver:
-  void WillDestroyCurrentMessageLoop() override {
-    DCHECK(thread_checker_.CalledOnValidThread());
-    delete this;
-  }
-
-  base::ThreadChecker thread_checker_;
-  base::PlatformFile platform_file_;
+  base::SequenceChecker sequence_checker_;
+  base::ScopedFD fd_;
   size_t report_buffer_size_;
   bool has_report_id_;
   base::WeakPtr<HidConnectionLinux> connection_;
@@ -124,38 +162,30 @@ class HidConnectionLinux::FileThreadHelper
 
 HidConnectionLinux::HidConnectionLinux(
     scoped_refptr<HidDeviceInfo> device_info,
-    base::File device_file,
-    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
+    base::ScopedFD fd,
+    scoped_refptr<base::SequencedTaskRunner> blocking_task_runner)
     : HidConnection(device_info),
-      file_task_runner_(file_task_runner),
+      fd_(fd.get()),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      blocking_task_runner_(blocking_task_runner),

[fdoray, 2017/04/07 20:26:23]

std::move(blocking_task_runner)

[Reilly Grant (use Gerrit), 2017/04/07 22:10:48]

Done.

       weak_factory_(this) {
-  task_runner_ = base::ThreadTaskRunnerHandle::Get();
-  device_file_ = std::move(device_file);
-
-  // The helper is passed a weak pointer to this connection so that it can be
-  // cleaned up after the connection is closed.
-  std::unique_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)));
+  helper_.reset(new FileThreadHelper(std::move(fd), device_info,

[fdoray, 2017/04/07 20:26:23]

MakeUnique

[Reilly Grant (use Gerrit), 2017/04/07 22:10:48]

Done.

+                                     weak_factory_.GetWeakPtr(), task_runner_));
+  blocking_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FileThreadHelper::Start, base::Unretained(helper_.get())));
 }
 
 HidConnectionLinux::~HidConnectionLinux() {
-  DCHECK(helper_ == nullptr);
+  DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 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
+  // By closing the device on the FILE thread 1) the requirement that

[robliao, 2017/04/07 01:08:24]

s/FILE thread/blocking sequenced task runner/

[Reilly Grant (use Gerrit), 2017/04/07 22:10:48]

Done.

+  // base::ScopedFD is destroyed 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_);
-  helper_ = nullptr;
-  file_task_runner_->PostTask(FROM_HERE,
-                              base::Bind(&HidConnectionLinux::CloseDevice,
-                                         base::Passed(&device_file_)));
+  blocking_task_runner_->DeleteSoon(FROM_HERE, helper_.release());
 
   while (!pending_reads_.empty()) {
     pending_reads_.front().callback.Run(false, NULL, 0);
@@ -175,13 +205,10 @@ void HidConnectionLinux::PlatformWrite(scoped_refptr<net::IOBuffer> buffer,
                                        const WriteCallback& callback) {
   // Linux expects the first byte of the buffer to always be a report ID so the
   // buffer can be used directly.
-  file_task_runner_->PostTask(
+  blocking_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&HidConnectionLinux::BlockingWrite,
-                 device_file_.GetPlatformFile(), buffer, size,
-                 base::Bind(&HidConnectionLinux::FinishWrite,
-                            weak_factory_.GetWeakPtr(), size, callback),
-                 task_runner_));
+      base::Bind(&FileThreadHelper::Write, base::Unretained(helper_.get()),
+                 buffer, size, callback));
 }
 
 void HidConnectionLinux::PlatformGetFeatureReport(
@@ -194,14 +221,10 @@ void HidConnectionLinux::PlatformGetFeatureReport(
       new net::IOBufferWithSize(device_info()->max_feature_report_size() + 1));
   buffer->data()[0] = report_id;
 
-  file_task_runner_->PostTask(
+  blocking_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(
-          &HidConnectionLinux::BlockingIoctl, device_file_.GetPlatformFile(),
-          HIDIOCGFEATURE(buffer->size()), buffer,
-          base::Bind(&HidConnectionLinux::FinishGetFeatureReport,
-                     weak_factory_.GetWeakPtr(), report_id, buffer, callback),
-          task_runner_));
+      base::Bind(&FileThreadHelper::GetFeatureReport,
+                 base::Unretained(helper_.get()), report_id, buffer, callback));
 }
 
 void HidConnectionLinux::PlatformSendFeatureReport(
@@ -210,88 +233,10 @@ void HidConnectionLinux::PlatformSendFeatureReport(
     const WriteCallback& callback) {
   // Linux expects the first byte of the buffer to always be a report ID so the
   // buffer can be used directly.
-  file_task_runner_->PostTask(
+  blocking_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&HidConnectionLinux::BlockingIoctl,
-                 device_file_.GetPlatformFile(), HIDIOCSFEATURE(size), buffer,
-                 base::Bind(&HidConnectionLinux::FinishSendFeatureReport,
-                            weak_factory_.GetWeakPtr(), callback),
-                 task_runner_));
-}
-
-void HidConnectionLinux::FinishWrite(size_t expected_size,
-                                     const WriteCallback& callback,
-                                     ssize_t result) {
-  if (result < 0) {
-    HID_PLOG(EVENT) << "Write failed";
-    callback.Run(false);
-  } else {
-    if (static_cast<size_t>(result) != expected_size) {
-      HID_LOG(EVENT) << "Incomplete HID write: " << result
-                     << " != " << expected_size;
-    }
-    callback.Run(true);
-  }
-}
-
-void HidConnectionLinux::FinishGetFeatureReport(
-    uint8_t report_id,
-    scoped_refptr<net::IOBuffer> buffer,
-    const ReadCallback& callback,
-    int result) {
-  if (result < 0) {
-    HID_PLOG(EVENT) << "Failed to get feature report";
-    callback.Run(false, NULL, 0);
-  } else if (result == 0) {
-    HID_LOG(EVENT) << "Get feature result too short.";
-    callback.Run(false, NULL, 0);
-  } else if (report_id == 0) {
-    // Linux adds a 0 to the beginning of the data received from the device.
-    scoped_refptr<net::IOBuffer> copied_buffer(new net::IOBuffer(result - 1));
-    memcpy(copied_buffer->data(), buffer->data() + 1, result - 1);
-    callback.Run(true, copied_buffer, result - 1);
-  } else {
-    callback.Run(true, buffer, result);
-  }
-}
-
-void HidConnectionLinux::FinishSendFeatureReport(const WriteCallback& callback,
-                                                 int result) {
-  if (result < 0) {
-    HID_PLOG(EVENT) << "Failed to send feature report";
-    callback.Run(false);
-  } else {
-    callback.Run(true);
-  }
-}
-
-// static
-void HidConnectionLinux::BlockingWrite(
-    base::PlatformFile platform_file,
-    scoped_refptr<net::IOBuffer> buffer,
-    size_t size,
-    const InternalWriteCallback& callback,
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  base::ThreadRestrictions::AssertIOAllowed();
-  ssize_t result = HANDLE_EINTR(write(platform_file, buffer->data(), size));
-  task_runner->PostTask(FROM_HERE, base::Bind(callback, result));
-}
-
-// static
-void HidConnectionLinux::BlockingIoctl(
-    base::PlatformFile platform_file,
-    int request,
-    scoped_refptr<net::IOBuffer> buffer,
-    const IoctlCallback& callback,
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  base::ThreadRestrictions::AssertIOAllowed();
-  int result = ioctl(platform_file, request, buffer->data());
-  task_runner->PostTask(FROM_HERE, base::Bind(callback, result));
-}
-
-// static
-void HidConnectionLinux::CloseDevice(base::File device_file) {
-  device_file.Close();
+      base::Bind(&FileThreadHelper::SendFeatureReport,
+                 base::Unretained(helper_.get()), buffer, size, callback));
 }
 
 void HidConnectionLinux::ProcessInputReport(scoped_refptr<net::IOBuffer> buffer,
@@ -311,9 +256,8 @@ void HidConnectionLinux::ProcessReadQueue() {
     PendingHidReport report = pending_reports_.front();
 
     pending_reports_.pop();
-    if (CompleteRead(report.buffer, report.size, read.callback)) {
+    if (CompleteRead(report.buffer, report.size, read.callback))
       pending_reads_.pop();
-    }
   }
 }