Add HidConnection::Close and register OS X callbacks on the UI thread.

The platform implementation of a HID connection may need to cancel I/O
operations when the connection is closed. If this is done during the
object destructor then any pointers held by those pending operations are
immediately invalid. A separate Close method allows the cleanup to
happen while the object is still available to handle asynchronous
cancellation events.

The OS X implementation will take advantage of this immediately to
register and unregister its input report callback from the UI thread to
avoid a race between event delivery and object cleanup. I've added
comments explaining why not all operations on the IOHIDDevice object
could be moved to the UI thread.

BUG=418207

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

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

13 files changed, 164 insertions, 96 deletions

diff --git a/device/hid/hid_connection.cc b/device/hid/hid_connection.cc
index 5df2a01..5ff39f3 100644
--- a/device/hid/hid_connection.cc
+++ b/device/hid/hid_connection.cc
@@ -64,12 +64,21 @@ bool HasProtectedCollection(const HidDeviceInfo& device_info) {
 }  // namespace
 
 HidConnection::HidConnection(const HidDeviceInfo& device_info)
-    : device_info_(device_info) {
+    : device_info_(device_info), closed_(false) {
   has_protected_collection_ = HasProtectedCollection(device_info);
 }
 
 HidConnection::~HidConnection() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(closed_);
+}
+
+void HidConnection::Close() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!closed_);
+
+  PlatformClose();
+  closed_ = true;
 }
 
 void HidConnection::Read(const ReadCallback& callback) {
diff --git a/device/hid/hid_connection.h b/device/hid/hid_connection.h
index 0a3e599..8c1bc0d 100644
--- a/device/hid/hid_connection.h
+++ b/device/hid/hid_connection.h
@@ -30,6 +30,10 @@ class HidConnection : public base::RefCountedThreadSafe<HidConnection> {
   const HidDeviceInfo& device_info() const { return device_info_; }
   bool has_protected_collection() const { return has_protected_collection_; }
   const base::ThreadChecker& thread_checker() const { return thread_checker_; }
+  bool closed() const { return closed_; }
+
+  // Closes the connection. This must be called before the object is freed.
+  void Close();
 
   // The report ID (or 0 if report IDs are not supported by the device) is
   // always returned in the first byte of the buffer.
@@ -58,6 +62,7 @@ class HidConnection : public base::RefCountedThreadSafe<HidConnection> {
   explicit HidConnection(const HidDeviceInfo& device_info);
   virtual ~HidConnection();
 
+  virtual void PlatformClose() = 0;
   virtual void PlatformRead(const ReadCallback& callback) = 0;
   virtual void PlatformWrite(scoped_refptr<net::IOBuffer> buffer,
                              size_t size,
@@ -83,6 +88,7 @@ class HidConnection : public base::RefCountedThreadSafe<HidConnection> {
   const HidDeviceInfo device_info_;
   bool has_protected_collection_;
   base::ThreadChecker thread_checker_;
+  bool closed_;
 
   DISALLOW_COPY_AND_ASSIGN(HidConnection);
 };
diff --git a/device/hid/hid_connection_linux.cc b/device/hid/hid_connection_linux.cc
index 936f016..021d301 100644
--- a/device/hid/hid_connection_linux.cc
+++ b/device/hid/hid_connection_linux.cc
@@ -72,6 +72,9 @@ HidConnectionLinux::HidConnectionLinux(HidDeviceInfo device_info,
 }
 
 HidConnectionLinux::~HidConnectionLinux() {
+}
+
+void HidConnectionLinux::PlatformClose() {
   Disconnect();
   Flush();
 }
diff --git a/device/hid/hid_connection_linux.h b/device/hid/hid_connection_linux.h
index cbb3b8d..7a8b0bc 100644
--- a/device/hid/hid_connection_linux.h
+++ b/device/hid/hid_connection_linux.h
@@ -23,6 +23,7 @@ class HidConnectionLinux : public HidConnection,
   virtual ~HidConnectionLinux();
 
   // HidConnection implementation.
+  virtual void PlatformClose() override;
   virtual void PlatformRead(const ReadCallback& callback) override;
   virtual void PlatformWrite(scoped_refptr<net::IOBuffer> buffer,
                              size_t size,
diff --git a/device/hid/hid_connection_mac.cc b/device/hid/hid_connection_mac.cc
index ae1a142..86285f9 100644
--- a/device/hid/hid_connection_mac.cc
+++ b/device/hid/hid_connection_mac.cc
@@ -5,17 +5,23 @@
 #include "device/hid/hid_connection_mac.h"
 
 #include "base/bind.h"
+#include "base/location.h"
 #include "base/mac/foundation_util.h"
-#include "base/message_loop/message_loop.h"
+#include "base/single_thread_task_runner.h"
+#include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
 #include "device/hid/hid_connection_mac.h"
 
 namespace device {
 
-HidConnectionMac::HidConnectionMac(HidDeviceInfo device_info)
+HidConnectionMac::HidConnectionMac(
+    HidDeviceInfo device_info,
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
     : HidConnection(device_info),
-      device_(device_info.device_id, base::scoped_policy::RETAIN) {
-  message_loop_ = base::MessageLoopProxy::current();
-
+      device_(device_info.device_id, base::scoped_policy::RETAIN),
+      ui_task_runner_(ui_task_runner) {
+  task_runner_ = base::ThreadTaskRunnerHandle::Get();
+  DCHECK(task_runner_.get());
   DCHECK(device_.get());
 
   size_t expected_report_size = device_info.max_input_report_size;
@@ -23,31 +29,36 @@ HidConnectionMac::HidConnectionMac(HidDeviceInfo device_info)
     expected_report_size++;
   }
   inbound_buffer_.resize(expected_report_size);
-  if (inbound_buffer_.size() > 0) {
-    IOHIDDeviceRegisterInputReportCallback(
-        device_.get(),
-        &inbound_buffer_[0],
-        inbound_buffer_.size(),
-        &HidConnectionMac::InputReportCallback,
-        this);
-  }
+
+  ui_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&HidConnectionMac::StartInputReportCallbacks, this));
 }
 
 HidConnectionMac::~HidConnectionMac() {
-  if (inbound_buffer_.size() > 0) {
-    // Unregister the input report callback before this object is freed.
-    IOHIDDeviceRegisterInputReportCallback(
-        device_.get(), &inbound_buffer_[0], inbound_buffer_.size(), NULL, this);
-  }
-  Flush();
 }
 
-void HidConnectionMac::PlatformRead(const ReadCallback& callback) {
-  if (!device_) {
-    callback.Run(false, NULL, 0);
-    return;
+void HidConnectionMac::PlatformClose() {
+  // To avoid a race between input reports delivered on the UI thread and
+  // closing this connection from its native thread the callback must be
+  // unregistered on the UI thread. It is safe to pass the this pointer
+  // unretained because StartInputReportCallbacks still holds a reference to it.
+  // We don't want this closure, which will be release on the UI thread, to be
+  // the final reference. StopInputReportCallbacks will use ReleaseSoon() to
+  // release this back on the native thread.
+  ui_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&HidConnectionMac::StopInputReportCallbacks,
+                 base::Unretained(this)));
+
+  while (!pending_reads_.empty()) {
+    pending_reads_.front().callback.Run(false, NULL, 0);
+    pending_reads_.pop();
   }
+}
 
+void HidConnectionMac::PlatformRead(const ReadCallback& callback) {
+  DCHECK(thread_checker().CalledOnValidThread());
   PendingHidRead pending_read;
   pending_read.callback = callback;
   pending_reads_.push(pending_read);
@@ -62,16 +73,16 @@ void HidConnectionMac::PlatformWrite(scoped_refptr<net::IOBuffer> buffer,
 
 void HidConnectionMac::PlatformGetFeatureReport(uint8_t report_id,
                                                 const ReadCallback& callback) {
-  if (!device_) {
-    callback.Run(false, NULL, 0);
-    return;
-  }
-
   scoped_refptr<net::IOBufferWithSize> buffer(
       new net::IOBufferWithSize(device_info().max_feature_report_size));
   CFIndex report_size = buffer->size();
+
+  // The IOHIDDevice object is shared with the UI thread and so this function
+  // should probably be called there but it may block and the asynchronous
+  // version is NOT IMPLEMENTED. I've examined the open source implementation
+  // of this function and believe it to be thread-safe.
   IOReturn result =
-      IOHIDDeviceGetReport(device_,
+      IOHIDDeviceGetReport(device_.get(),
                            kIOHIDReportTypeFeature,
                            report_id,
                            reinterpret_cast<uint8_t*>(buffer->data()),
@@ -79,7 +90,8 @@ void HidConnectionMac::PlatformGetFeatureReport(uint8_t report_id,
   if (result == kIOReturnSuccess) {
     callback.Run(true, buffer, report_size);
   } else {
-    VLOG(1) << "Failed to get feature report: " << result;
+    VLOG(1) << "Failed to get feature report: " << base::StringPrintf("0x%08x",
+                                                                      result);
     callback.Run(false, NULL, 0);
   }
 }
@@ -91,6 +103,31 @@ void HidConnectionMac::PlatformSendFeatureReport(
   WriteReport(kIOHIDReportTypeFeature, buffer, size, callback);
 }
 
+void HidConnectionMac::StartInputReportCallbacks() {
+  DCHECK(ui_task_runner_->BelongsToCurrentThread());
+  if (inbound_buffer_.size() > 0) {
+    AddRef();  // Hold a reference to this while this callback is registered.
+    IOHIDDeviceRegisterInputReportCallback(
+        device_.get(),
+        &inbound_buffer_[0],
+        inbound_buffer_.size(),
+        &HidConnectionMac::InputReportCallback,
+        this);
+  }
+}
+
+void HidConnectionMac::StopInputReportCallbacks() {
+  DCHECK(ui_task_runner_->BelongsToCurrentThread());
+  if (inbound_buffer_.size() > 0) {
+    IOHIDDeviceRegisterInputReportCallback(
+        device_.get(), &inbound_buffer_[0], inbound_buffer_.size(), NULL, this);
+    // Release the reference to this taken by StartInputReportCallbacks but do
+    // so on the right thread as this is likely the final reference.
+    task_runner_->ReleaseSoon(FROM_HERE, this);
+  }
+}
+
+// static
 void HidConnectionMac::InputReportCallback(void* context,
                                            IOReturn result,
                                            void* sender,
@@ -98,12 +135,14 @@ void HidConnectionMac::InputReportCallback(void* context,
                                            uint32_t report_id,
                                            uint8_t* report_bytes,
                                            CFIndex report_length) {
+  HidConnectionMac* connection = static_cast<HidConnectionMac*>(context);
+  DCHECK(connection->ui_task_runner_->BelongsToCurrentThread());
   if (result != kIOReturnSuccess) {
-    VLOG(1) << "Failed to read input report: " << result;
+    VLOG(1) << "Failed to read input report: " << base::StringPrintf("0x%08x",
+                                                                     result);
     return;
   }
 
-  HidConnectionMac* connection = static_cast<HidConnectionMac*>(context);
   scoped_refptr<net::IOBufferWithSize> buffer;
   if (connection->device_info().has_report_id) {
     // report_id is already contained in report_bytes
@@ -115,47 +154,11 @@ void HidConnectionMac::InputReportCallback(void* context,
     memcpy(buffer->data() + 1, report_bytes, report_length);
   }
 
-  connection->message_loop_->PostTask(
+  connection->task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&HidConnectionMac::ProcessInputReport, connection, buffer));
 }
 
-void HidConnectionMac::WriteReport(IOHIDReportType type,
-                                   scoped_refptr<net::IOBuffer> buffer,
-                                   size_t size,
-                                   const WriteCallback& callback) {
-  if (!device_) {
-    callback.Run(false);
-    return;
-  }
-
-  uint8_t* data = reinterpret_cast<uint8_t*>(buffer->data());
-  DCHECK(size >= 1);
-  uint8_t report_id = data[0];
-  if (report_id == 0) {
-    // OS X only expects the first byte of the buffer to be the report ID if the
-    // report ID is non-zero.
-    ++data;
-    --size;
-  }
-
-  IOReturn res =
-      IOHIDDeviceSetReport(device_.get(), type, report_id, data, size);
-  if (res == kIOReturnSuccess) {
-    callback.Run(true);
-  } else {
-    VLOG(1) << "Failed to set report: " << res;
-    callback.Run(false);
-  }
-}
-
-void HidConnectionMac::Flush() {
-  while (!pending_reads_.empty()) {
-    pending_reads_.front().callback.Run(false, NULL, 0);
-    pending_reads_.pop();
-  }
-}
-
 void HidConnectionMac::ProcessInputReport(
     scoped_refptr<net::IOBufferWithSize> buffer) {
   DCHECK(thread_checker().CalledOnValidThread());
@@ -179,4 +182,32 @@ void HidConnectionMac::ProcessReadQueue() {
   }
 }
 
+void HidConnectionMac::WriteReport(IOHIDReportType report_type,
+                                   scoped_refptr<net::IOBuffer> buffer,
+                                   size_t size,
+                                   const WriteCallback& callback) {
+  uint8_t* data = reinterpret_cast<uint8_t*>(buffer->data());
+  DCHECK_GE(size, 1u);
+  uint8_t report_id = data[0];
+  if (report_id == 0) {
+    // OS X only expects the first byte of the buffer to be the report ID if the
+    // report ID is non-zero.
+    ++data;
+    --size;
+  }
+
+  // The IOHIDDevice object is shared with the UI thread and so this function
+  // should probably be called there but it may block and the asynchronous
+  // version is NOT IMPLEMENTED. I've examined the open source implementation
+  // of this function and believe it to be thread-safe.
+  IOReturn result =
+      IOHIDDeviceSetReport(device_.get(), report_type, report_id, data, size);
+  if (result == kIOReturnSuccess) {
+    callback.Run(true);
+  } else {
+    VLOG(1) << "Failed to set report: " << base::StringPrintf("0x%08x", result);
+    callback.Run(false);
+  }
+}
+
 }  // namespace device
diff --git a/device/hid/hid_connection_mac.h b/device/hid/hid_connection_mac.h
index f1ce414..aa23fc9 100644
--- a/device/hid/hid_connection_mac.h
+++ b/device/hid/hid_connection_mac.h
@@ -15,7 +15,7 @@
 #include "device/hid/hid_connection.h"
 
 namespace base {
-class MessageLoopProxy;
+class SingleThreadTaskRunner;
 }
 
 namespace net {
@@ -26,12 +26,15 @@ namespace device {
 
 class HidConnectionMac : public HidConnection {
  public:
-  explicit HidConnectionMac(HidDeviceInfo device_info);
+  explicit HidConnectionMac(
+      HidDeviceInfo device_info,
+      scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
 
  private:
   virtual ~HidConnectionMac();
 
   // HidConnection implementation.
+  virtual void PlatformClose() override;
   virtual void PlatformRead(const ReadCallback& callback) override;
   virtual void PlatformWrite(scoped_refptr<net::IOBuffer> buffer,
                              size_t size,
@@ -43,6 +46,8 @@ class HidConnectionMac : public HidConnection {
       size_t size,
       const WriteCallback& callback) override;
 
+  void StartInputReportCallbacks();
+  void StopInputReportCallbacks();
   static void InputReportCallback(void* context,
                                   IOReturn result,
                                   void* sender,
@@ -50,18 +55,17 @@ class HidConnectionMac : public HidConnection {
                                   uint32_t report_id,
                                   uint8_t* report_bytes,
                                   CFIndex report_length);
+  void ProcessInputReport(scoped_refptr<net::IOBufferWithSize> buffer);
+  void ProcessReadQueue();
 
-  void WriteReport(IOHIDReportType type,
+  void WriteReport(IOHIDReportType report_type,
                    scoped_refptr<net::IOBuffer> buffer,
                    size_t size,
                    const WriteCallback& callback);
 
-  void Flush();
-  void ProcessInputReport(scoped_refptr<net::IOBufferWithSize> buffer);
-  void ProcessReadQueue();
-
   base::ScopedCFTypeRef<IOHIDDeviceRef> device_;
-  scoped_refptr<base::MessageLoopProxy> message_loop_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
   std::vector<uint8_t> inbound_buffer_;
 
   std::queue<PendingHidReport> pending_reports_;
diff --git a/device/hid/hid_connection_unittest.cc b/device/hid/hid_connection_unittest.cc
index 97b977e..28e136c 100644
--- a/device/hid/hid_connection_unittest.cc
+++ b/device/hid/hid_connection_unittest.cc
@@ -149,6 +149,8 @@ TEST_F(HidConnectionTest, ReadWrite) {
       ASSERT_EQ(i + j - 1, read_callback.buffer()->data()[j]);
     }
   }
+
+  conn->Close();
 }
 
 }  // namespace device
diff --git a/device/hid/hid_connection_win.cc b/device/hid/hid_connection_win.cc
index 4f39c7c..a8414da 100644
--- a/device/hid/hid_connection_win.cc
+++ b/device/hid/hid_connection_win.cc
@@ -125,6 +125,9 @@ HidConnectionWin::HidConnectionWin(const HidDeviceInfo& device_info)
 }
 
 HidConnectionWin::~HidConnectionWin() {
+}
+
+void HidConnectionWin::PlatformClose() {
   CancelIo(file_.Get());
 }
 
diff --git a/device/hid/hid_connection_win.h b/device/hid/hid_connection_win.h
index 25e5d7b..b3e3ce4 100644
--- a/device/hid/hid_connection_win.h
+++ b/device/hid/hid_connection_win.h
@@ -27,6 +27,7 @@ class HidConnectionWin : public HidConnection {
   ~HidConnectionWin();
 
   // HidConnection implementation.
+  virtual void PlatformClose() override;
   virtual void PlatformRead(const ReadCallback& callback) override;
   virtual void PlatformWrite(scoped_refptr<net::IOBuffer> buffer,
                              size_t size,
diff --git a/device/hid/hid_service.cc b/device/hid/hid_service.cc
index 455999f..dc5599a 100644
--- a/device/hid/hid_service.cc
+++ b/device/hid/hid_service.cc
@@ -51,7 +51,7 @@ HidService* HidService::GetInstance(
 #if defined(OS_LINUX) && defined(USE_UDEV)
     g_service = new HidServiceLinux(ui_task_runner);
 #elif defined(OS_MACOSX)
-    g_service = new HidServiceMac();
+    g_service = new HidServiceMac(ui_task_runner);
 #elif defined(OS_WIN)
     g_service = new HidServiceWin();
 #endif
diff --git a/device/hid/hid_service_mac.cc b/device/hid/hid_service_mac.cc
index fe48761..3e397dc 100644
--- a/device/hid/hid_service_mac.cc
+++ b/device/hid/hid_service_mac.cc
@@ -12,13 +12,14 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
-#include "base/message_loop/message_loop.h"
-#include "base/message_loop/message_loop_proxy.h"
+#include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/threading/thread_restrictions.h"
 #include "device/hid/hid_connection_mac.h"
 
@@ -144,10 +145,11 @@ void GetCollectionInfos(IOHIDDeviceRef device,
 
 }  // namespace
 
-HidServiceMac::HidServiceMac() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  message_loop_ = base::MessageLoopProxy::current();
-  DCHECK(message_loop_.get());
+HidServiceMac::HidServiceMac(
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
+    : ui_task_runner_(ui_task_runner) {
+  task_runner_ = base::ThreadTaskRunnerHandle::Get();
+  DCHECK(task_runner_.get());
   hid_manager_.reset(IOHIDManagerCreate(NULL, 0));
   if (!hid_manager_) {
     LOG(ERROR) << "Failed to initialize HidManager";
@@ -195,10 +197,10 @@ void HidServiceMac::AddDeviceCallback(void* context,
   // Claim ownership of the device.
   CFRetain(hid_device);
   HidServiceMac* service = HidServiceFromContext(context);
-  service->message_loop_->PostTask(FROM_HERE,
-                                   base::Bind(&HidServiceMac::PlatformAddDevice,
-                                              base::Unretained(service),
-                                              base::Unretained(hid_device)));
+  service->task_runner_->PostTask(FROM_HERE,
+                                  base::Bind(&HidServiceMac::PlatformAddDevice,
+                                             base::Unretained(service),
+                                             base::Unretained(hid_device)));
 }
 
 void HidServiceMac::RemoveDeviceCallback(void* context,
@@ -207,7 +209,7 @@ void HidServiceMac::RemoveDeviceCallback(void* context,
                                          IOHIDDeviceRef hid_device) {
   DCHECK(CFRunLoopGetMain() == CFRunLoopGetCurrent());
   HidServiceMac* service = HidServiceFromContext(context);
-  service->message_loop_->PostTask(
+  service->task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&HidServiceMac::PlatformRemoveDevice,
                  base::Unretained(service),
@@ -273,7 +275,8 @@ scoped_refptr<HidConnection> HidServiceMac::Connect(
   HidDeviceInfo device_info;
   if (!GetDeviceInfo(device_id, &device_info))
     return NULL;
-  return scoped_refptr<HidConnection>(new HidConnectionMac(device_info));
+  return scoped_refptr<HidConnection>(
+      new HidConnectionMac(device_info, ui_task_runner_));
 }
 
 }  // namespace device
diff --git a/device/hid/hid_service_mac.h b/device/hid/hid_service_mac.h
index eb9da5b..83a5097 100644
--- a/device/hid/hid_service_mac.h
+++ b/device/hid/hid_service_mac.h
@@ -15,7 +15,7 @@
 #include "device/hid/hid_service.h"
 
 namespace base {
-class MessageLoopProxy;
+class SingleThreadTaskRunner;
 }
 
 namespace device {
@@ -24,7 +24,7 @@ class HidConnection;
 
 class HidServiceMac : public HidService {
  public:
-  HidServiceMac();
+  HidServiceMac(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
 
   virtual scoped_refptr<HidConnection> Connect(const HidDeviceId& device_id)
       override;
@@ -53,8 +53,11 @@ class HidServiceMac : public HidService {
   // Platform HID Manager
   base::ScopedCFTypeRef<IOHIDManagerRef> hid_manager_;
 
-  // The message loop for the thread on which this service was created.
-  scoped_refptr<base::MessageLoopProxy> message_loop_;
+  // The task runner for the thread on which this service was created.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  // The task runner for the UI thread of the application using this service.
+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(HidServiceMac);
 };
diff --git a/extensions/browser/api/hid/hid_connection_resource.cc b/extensions/browser/api/hid/hid_connection_resource.cc
index 94883d8..f1c58b1 100644
--- a/extensions/browser/api/hid/hid_connection_resource.cc
+++ b/extensions/browser/api/hid/hid_connection_resource.cc
@@ -30,7 +30,9 @@ HidConnectionResource::HidConnectionResource(
     scoped_refptr<device::HidConnection> connection)
     : ApiResource(owner_extension_id), connection_(connection) {}
 
-HidConnectionResource::~HidConnectionResource() {}
+HidConnectionResource::~HidConnectionResource() {
+  connection_->Close();
+}
 
 bool HidConnectionResource::IsPersistent() const {
   return false;