Clean up HID backend and API.

device/hid/hid_service_mac.cc

 // Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "device/hid/hid_service_mac.h"
 
-#include <map>
-#include <set>
+#include <CoreFoundation/CoreFoundation.h>
+#include <IOKit/hid/IOHIDManager.h>
+
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/memory/scoped_vector.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "device/hid/hid_connection.h"
 #include "device/hid/hid_connection_mac.h"
 #include "device/hid/hid_utils_mac.h"
 #include "net/base/io_buffer.h"
 
-#include <CoreFoundation/CoreFoundation.h>
-#include <IOKit/hid/IOHIDManager.h>
-
 namespace device {
 
 class HidServiceMac;
 
-HidServiceMac::HidServiceMac() : enumeration_runloop_init_(true, false) {
-  base::ThreadRestrictions::AssertIOAllowed();
+namespace {
 
-  hid_manager_ref_.reset(IOHIDManagerCreate(kCFAllocatorDefault,
-                                            kIOHIDOptionsTypeNone));
-  if (!hid_manager_ref_ ||
-      CFGetTypeID(hid_manager_ref_) != IOHIDManagerGetTypeID()) {
+typedef std::vector<IOHIDDeviceRef> HidDeviceList;

[Mark Mentovai, 2014/02/13 22:37:51]

#include <vector> to use this.

Or you could use a std::set (and <set>) to implicitly keep the same promises
that the CFSet used to populate this did.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done. I left it as a vector since the usage is isolated and consists only of
traversal, and building a vector should be slightly cheaper than building a set.
I don't feel strongly about it either way though.

+
+HidServiceMac* HidServiceFromContext(void* context) {
+  return reinterpret_cast<HidServiceMac*>(context);
+}
+
+// Callback for CFSetApplyFunction as used by EnumerateHidDevices.
+void HidEnumerationBackInserter(const void* value, void* context) {
+  HidDeviceList* devices = static_cast<HidDeviceList*>(context);
+  IOHIDDeviceRef device = static_cast<IOHIDDeviceRef>(value);
+  devices->push_back(device);
+}
+
+void EnumerateHidDevices(IOHIDManagerRef hid_manager,
+                         HidDeviceList* device_list) {
+  base::ScopedCFTypeRef<CFSetRef> devices(IOHIDManagerCopyDevices(hid_manager));
+  CFSetApplyFunction(devices, HidEnumerationBackInserter, device_list);

[Mark Mentovai, 2014/02/13 22:37:51]

Great! This is so much better than allocating space and copying the pointers out
of the CFSet just to iterate over them. Thanks.

+}
+
+}  // namespace
+
+HidServiceMac::HidServiceMac() {
+  hid_manager_.reset(IOHIDManagerCreate(NULL, kIOHIDManagerOptionNone));
+  if (!hid_manager_ || CFGetTypeID(hid_manager_) != IOHIDManagerGetTypeID()) {

[Mark Mentovai, 2014/02/13 22:37:51]

Since the type-check portion of this is pretty cheesey and you agree that it’s
probably just paranoia, it’s better as a DCHECK than an always-enabled test with
a graceful failure mode. You can leave the !hid_manager_ test alone with an
early return, though.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done.

     LOG(ERROR) << "Failed to initialize HidManager";
     return;
   }
-  CFRetain(hid_manager_ref_);
+
+  // Enumerate all the currently known devices.
+  Enumerate();
 
   // Register for plug/unplug notifications.
-  IOHIDManagerSetDeviceMatching(hid_manager_ref_.get(), NULL);
-  IOHIDManagerRegisterDeviceMatchingCallback(
-      hid_manager_ref_.get(),
-      &HidServiceMac::AddDeviceCallback,
-      this);
-  IOHIDManagerRegisterDeviceRemovalCallback(
-      hid_manager_ref_.get(),
-      &HidServiceMac::RemoveDeviceCallback,
-      this);
+  StartWatchingDevices();
 
-  // Blocking operation to enumerate all the pre-existing devices.
-  Enumerate();
-
-  // Save the owner message loop.
-  message_loop_ = base::MessageLoopProxy::current();
-
-  // Start a thread to monitor HID device changes.
-  // The reason to create a new thread is that by default the only thread in the
-  // browser process having a CFRunLoop is the UI thread. We do not want to
-  // run potentially blocking routines on it.
-  enumeration_runloop_thread_.reset(
-      new base::Thread("HidService Device Enumeration Thread"));
-
-  base::Thread::Options options;
-  options.message_loop_type = base::MessageLoop::TYPE_UI;
-  enumeration_runloop_thread_->StartWithOptions(options);
-  enumeration_runloop_thread_->message_loop()->PostTask(
-      FROM_HERE,
-      base::Bind(&HidServiceMac::ScheduleRunLoop, base::Unretained(this)));
-
-  enumeration_runloop_init_.Wait();
   initialized_ = true;

[Mark Mentovai, 2014/02/13 22:37:51]

This is unused.

Well, OK, not exactly. It’s unused in this class, but I see that it’s a
protected member of the base class. That’s not a good design. If anyone actually
calls the base’s initialized(), then it should be turned into a pure virtual
Initialized() (capital letter I), and implementations should either “return
true” (looks like that’s what this one should do) or do whatever else is
appropriate. But if there are no callers, or if in each implementation it would
be logically impossible to return anything other than “true” after the
constructor runs, then initialized() (regardless of how capital the I is) is
pointless and it should be removed altogether.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Yes I absolutely hate this. Removed altogether. The removal only changes the
behavior of the Windows impl slightly and I don't think preserving the old
behavior is worth separating construction and initialization.

 }
 
-HidServiceMac::~HidServiceMac() {
-  enumeration_runloop_thread_->message_loop()->PostTask(
-        FROM_HERE,
-        base::Bind(&HidServiceMac::UnscheduleRunLoop, base::Unretained(this)));
+HidServiceMac::~HidServiceMac() { StopWatchingDevices(); }

[Mark Mentovai, 2014/02/13 22:37:51]

This may be dangerous if you hit the early return in the constructor. You might
want to check whether hid_manager_ is non-NULL.

(I wouldn’t resurrect initialized_ just for this sake, since now that the
threading nastiness is gone, bool(hid_manager_) has effectively the same meaning
that initialized_ was intended to carry.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done.

 
-  enumeration_runloop_thread_->Stop();
+void HidServiceMac::StartWatchingDevices() {
+  IOHIDManagerSetDeviceMatching(hid_manager_, NULL);
+  IOHIDManagerRegisterDeviceMatchingCallback(
+      hid_manager_, &AddDeviceCallback, this);
+  IOHIDManagerRegisterDeviceRemovalCallback(
+      hid_manager_, &RemoveDeviceCallback, this);
+  IOHIDManagerScheduleWithRunLoop(
+      hid_manager_, CFRunLoopGetMain(), kCFRunLoopDefaultMode);

[Mark Mentovai, 2014/02/13 22:37:51]

Now that this is expected to run on the main thread, you should add some
thread-safety DCHECKs so that any publicly-callable function will traverse one.
Any private function that’s expected to run on a specific thread (like
Add/RemoveDeviceCallback) should also execute this type of DCHECK.

I don’t care if you do this by comparing CFRunLoopGetMain() to
CFRunLoopGetCurrent() when you want to check for the main thread, or use
something more Chromey like BrowserThread::CurrentlyOn(BrowserThread::UI). I
expect you’ll need to use the latter to check that you’re on the FILE thread, if
you’re actually allowed to depend on content in here—see below.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Thinking about it some more, I'd rather avoid the content dependency even if
it's not strictly necessary to do so.

The base HidService upon construction verifies that the current thread allows
I/O. It also owns a ThreadChecker which most methods can use to ensure they're
running on the original (ctor's) thread. These checks should fulfill our only
real requirements: that the public interface is always used on a single thread
that supports I/O.

The device match/removal callbacks also now DCHECK that
CFRunLoopGetCurrent()==CFRunLoopGetMain().

+  IOHIDManagerOpen(hid_manager_, kIOHIDOptionsTypeNone);
 }
 
-void HidServiceMac::ScheduleRunLoop() {
-  enumeration_runloop_ = CFRunLoopGetCurrent();
-
-  IOHIDManagerScheduleWithRunLoop(
-      hid_manager_ref_,
-      enumeration_runloop_,
-      kCFRunLoopDefaultMode);
-
-  IOHIDManagerOpen(hid_manager_ref_, kIOHIDOptionsTypeNone);
-
-  enumeration_runloop_init_.Signal();
-}
-
-void HidServiceMac::UnscheduleRunLoop() {
+void HidServiceMac::StopWatchingDevices() {
   IOHIDManagerUnscheduleFromRunLoop(
-        hid_manager_ref_,
-        enumeration_runloop_,
-        kCFRunLoopDefaultMode);
-
-  IOHIDManagerClose(hid_manager_ref_, kIOHIDOptionsTypeNone);
-
-}
-
-HidServiceMac* HidServiceMac::InstanceFromContext(void* context) {
-  return reinterpret_cast<HidServiceMac*>(context);
+      hid_manager_, CFRunLoopGetMain(), kCFRunLoopDefaultMode);
+  IOHIDManagerClose(hid_manager_, kIOHIDOptionsTypeNone);
 }
 
 void HidServiceMac::AddDeviceCallback(void* context,
                                       IOReturn result,
                                       void* sender,
-                                      IOHIDDeviceRef ref) {
-  HidServiceMac* service = InstanceFromContext(context);
-
-  // Takes ownership of ref. Will be released inside PlatformDeviceAdd.
-  CFRetain(ref);
-  service->message_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&HidServiceMac::PlatformAddDevice,
-                 base::Unretained(service),
-                 base::Unretained(ref)));
+                                      IOHIDDeviceRef hid_device) {
+  HidServiceMac* service = HidServiceFromContext(context);
+  BrowserThread::PostTask(BrowserThread::FILE,

[Mark Mentovai, 2014/02/13 22:37:51]

To use this, you need to #include "content/public/browser/browser_thread.h"

I am surprised that you would have BrowserThread incidentally defined in this
file with just the #includes here. I also don’t know if device/hid is even
allowed to depend on content. It doesn’t declare this dependency in
device/hid/hid.gyp, although if it’s supposed to use content and is in fact
allowed to, it would need to.

Do you know how this is supposed to work in the grand scheme of layered
components?

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

I decided to retain a handle to the current MessageLoop in the ctor. Instead of
posting to a specific Chrome thread, these callbacks will post to that original
MessageLoop.

+                          FROM_HERE,
+                          base::Bind(&PlatformAddDevice,
+                                     base::Unretained(service),
+                                     base::Unretained(hid_device)));
 }
 
 void HidServiceMac::RemoveDeviceCallback(void* context,
                                          IOReturn result,
                                          void* sender,
-                                         IOHIDDeviceRef ref) {
-  HidServiceMac* service = InstanceFromContext(context);
-
-  // Takes ownership of ref. Will be released inside PlatformDeviceRemove.
-  CFRetain(ref);
-  service->message_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&HidServiceMac::PlatformRemoveDevice,
-                 base::Unretained(service),
-                 base::Unretained(ref)));
+                                         IOHIDDeviceRef hid_device) {
+  HidServiceMac* service = HidServiceFromContext(context);
+  BrowserThread::PostTask(BrowserThread::FILE,
+                          FROM_HERE,
+                          base::Bind(&PlatformRemoveDevice,
+                                     base::Unretained(service),
+                                     base::Unretained(hid_device)));
 }
 
-IOHIDDeviceRef HidServiceMac::FindDevice(std::string id) {
-  base::ScopedCFTypeRef<CFSetRef> devices(
-      IOHIDManagerCopyDevices(hid_manager_ref_));
-  CFIndex count = CFSetGetCount(devices);
-  scoped_ptr<IOHIDDeviceRef[]> device_refs(new IOHIDDeviceRef[count]);
-  CFSetGetValues(devices, (const void **)(device_refs.get()));
-
-  for (CFIndex i = 0; i < count; i++) {
+IOHIDDeviceRef HidServiceMac::FindDevice(const std::string& device_id) {
+  HidDeviceList devices;
+  EnumerateHidDevices(hid_manager_, &devices);
+  for (HidDeviceList::const_iterator iter = devices.begin();
+       iter != devices.end();
+       ++iter) {
     int32_t int_property = 0;

[Mark Mentovai, 2014/02/13 22:37:51]

No need to initialize.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done.

-    if (GetHidIntProperty(device_refs[i], CFSTR(kIOHIDLocationIDKey),
-                       &int_property)) {
+    if (TryGetHidIntProperty(*iter,

[Mark Mentovai, 2014/02/13 22:37:51]

Since you’ve used *iter a couple of times in this function (and a couple of
times in Enumerate below), you might want to stick it in a local “IOHIDDeviceRef
device” or “IOHIDDeviceRef hid_device” variable—that’d also help the code be a
tiny bit more self-documenting. TryGetHidIntProperty(hid_device, …) makes a
little more sense than TryGetHidIntProperty(*iter, …). Same in Enumerate below.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done.

+                             CFSTR(kIOHIDLocationIDKey),
+                             &int_property)) {
       if (id == base::HexEncode(&int_property, sizeof(int_property))) {

[Mark Mentovai, 2014/02/13 22:37:51]

Stopping here. You obviously didn’t even compile this.

-        return device_refs[i];
+        return *iter;
       }
     }
   }
 
   return NULL;
 }
 
 void HidServiceMac::Enumerate() {
-  base::ScopedCFTypeRef<CFSetRef> devices(
-      IOHIDManagerCopyDevices(hid_manager_ref_));
-  CFIndex count = CFSetGetCount(devices);
-  scoped_ptr<IOHIDDeviceRef[]> device_refs(new IOHIDDeviceRef[count]);
-  CFSetGetValues(devices, (const void **)(device_refs.get()));
-
-  for (CFIndex i = 0; i < count; i++) {
-    // Takes ownership. Will be released inside PlatformDeviceAdd.
-    CFRetain(device_refs[i]);
-    PlatformAddDevice(device_refs[i]);
+  HidDeviceList devices;
+  EnumerateHidDevices(hid_manager_, &devices);
+  for (HidDeviceList::const_iterator iter = devices.begin();
+       iter != devices.end();
+       ++iter) {
+    // Takes ownership. Will be released inside PlatformAddDevice.
+    CFRetain(*iter);
+    PlatformAddDevice(*iter);
   }
 }
 
-void HidServiceMac::PlatformAddDevice(IOHIDDeviceRef raw_ref) {
-  HidDeviceInfo device;
-  int32_t int_property = 0;
-  std::string str_property;
-
-  // Auto-release.
-  base::ScopedCFTypeRef<IOHIDDeviceRef> ref(raw_ref);
+void HidServiceMac::PlatformAddDevice(IOHIDDeviceRef hid_device) {
+  HidDeviceInfo device_info;

[Mark Mentovai, 2014/02/13 22:37:51]

Don’t declare stuff ’til you use it.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done.

+  base::ScopedCFTypeRef<IOHIDDeviceRef> device(hid_device);

[Mark Mentovai, 2014/02/13 22:37:51]

The same discussion here applies to PlatformRemoveDevice.

This assumes that this function owns hid_device. When this function is called by
Enumerate, that’s reasonable, because Enumerate has added a reference by calling
CFRetain, and it’s expecting PlatformAddDevice to assume ownership of that
reference. But what about when it’s called by AddDeviceCallback?

It’s a little unorthodox for a callback function to be given a reference that
it’s expected to assume ownership of. It’s not unheard of, it’s just weird (and
in my experience, it’s more likely to happen in IOKit than in other parts of the
system). The header doesn’t say anything as far as I can see.

Let’s scrutinize this one a bit. Do you know for a fact that device-matching and
device-removal callbacks are expected to assume ownership of their
IOHIDDeviceRef argument? Is there any sign that this code is over-releasing now?
Would it leak if this scoper were removed? (A standalone stress test of this
design, maybe with a debugging malloc, could help answer that.)

Luckily for us, the source code is available. I’m looking at 10.9.0
IOKitUser-907.1.13/hid.subproj/IOHIDManager.c and it appears to me that there is
no extra reference. Do you agree? If so, get rid of this scoper and the one in
PlatformRemoveDevice, and get rid of the CFRetain in Enumerate.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

This appears to be correct, the manager does not retain a reference. Removed
scopers and CFRetain in Enumerate.

 
   // Unique identifier for HID device.
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDLocationIDKey), &int_property)) {
-    device.device_id = base::HexEncode(&int_property, sizeof(int_property));
-  } else {
-    // Not an available device.
+  int32_t location_id = 0;

[Mark Mentovai, 2014/02/13 22:37:51]

No need to initialize.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done.

+  if (!TryGetHidIntProperty(device, CFSTR(kIOHIDLocationIDKey), &location_id))

[Mark Mentovai, 2014/02/13 22:37:51]

Just a reminder that “is the location ID the right thing to use?” is something
that we need to revisit. How is this handled on other platforms? Do they also
show a device multiple times under the same ID?

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

I've looked into this and I'm pretty confident this is the right thing to do
now. I don't know what you mean by showing a device "multiple times under the
same ID." The goal here is to have a system-wide unique identifier for a
specific physically connected device, and that is what we get with this location
ID.

On Windows we use the device path, which is a string that looks something like
"\\?\hid#vid=1234&pid=1234#1234abcde{some-guid}", on Linux we have a long /sys
path which reflects the bus topology, and here we have the "location ID."

In all three cases, the key is ultimately derived by the OS from the physical
bus location of the device. I'm not aware of any alternative device property
that could be used for unique device identification on OS X.

     return;
-  }
 
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDVendorIDKey), &int_property)) {
-    device.vendor_id = int_property;
-  }
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDProductIDKey), &int_property)) {
-    device.product_id = int_property;
-  }
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDPrimaryUsageKey), &int_property)) {
-    device.usage = int_property;
-  }
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDPrimaryUsagePageKey), &int_property)) {
-    device.usage_page = int_property;
-  }
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDMaxInputReportSizeKey),
-                        &int_property)) {
-    device.input_report_size = int_property;
-  }
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDMaxOutputReportSizeKey),
-                        &int_property)) {
-    device.output_report_size = int_property;
-  }
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDMaxFeatureReportSizeKey),
-                     &int_property)) {
-    device.feature_report_size = int_property;
-  }
-  if (GetHidStringProperty(ref, CFSTR(kIOHIDProductKey), &str_property)) {
-    device.product_name = str_property;
-  }
-  if (GetHidStringProperty(ref, CFSTR(kIOHIDSerialNumberKey), &str_property)) {
-    device.serial_number = str_property;
-  }
-  HidService::AddDevice(device);
+  device_info.device_id = base::HexEncode(&location_id, sizeof(location_id));
+  device_info.vendor_id = GetHidIntProperty(device, CFSTR(kIOHIDVendorIDKey));
+  device_info.product_id = GetHidIntProperty(device, CFSTR(kIOHIDProductIDKey));
+  device_info.usage = GetHidIntProperty(device, CFSTR(kIOHIDPrimaryUsageKey));
+  device_info.usage_page =
+      GetHidIntProperty(device, CFSTR(kIOHIDPrimaryUsagePageKey));
+  device_info.input_report_size =
+      GetHidIntProperty(device, CFSTR(kIOHIDMaxInputReportSizeKey));
+  device_info.output_report_size =
+      GetHidIntProperty(device, CFSTR(kIOHIDMaxOutputReportSizeKey));
+  device_info.feature_report_size =
+      GetHidIntProperty(device, CFSTR(kIOHIDMaxFeatureReportSizeKey));
+  device_info.product_name =
+      GetHidStringProperty(device, CFSTR(kIOHIDProductKey));
+  device_info.serial_number =
+      GetHidStringProperty(device, CFSTR(kIOHIDSerialNumberKey));
+
+  HidService::AddDevice(device_info);
 }
 
-void HidServiceMac::PlatformRemoveDevice(IOHIDDeviceRef raw_ref) {
+void HidServiceMac::PlatformRemoveDevice(IOHIDDeviceRef hid_device) {
   std::string device_id;

[Mark Mentovai, 2014/02/13 22:37:51]

Don’t declare until use. You save a line of code that way, too.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done.

   int32_t int_property = 0;

[Mark Mentovai, 2014/02/13 22:37:51]

No need to initialize.

[Ken Rockot(use gerrit already), 2014/02/18 19:43:09]

Done.

-  // Auto-release.
-  base::ScopedCFTypeRef<IOHIDDeviceRef> ref(raw_ref);
-  if (!GetHidIntProperty(ref, CFSTR(kIOHIDLocationIDKey), &int_property)) {
+  base::ScopedCFTypeRef<IOHIDDeviceRef> device(hid_device);
+  if (!TryGetHidIntProperty(device, CFSTR(kIOHIDLocationIDKey), &int_property))
     return;
-  }
   device_id = base::HexEncode(&int_property, sizeof(int_property));
   HidService::RemoveDevice(device_id);
 }
 
-scoped_refptr<HidConnection>
-HidServiceMac::Connect(std::string device_id) {
+scoped_refptr<HidConnection> HidServiceMac::Connect(
+    const std::string& device_id) {
   if (!ContainsKey(devices_, device_id))
     return NULL;
 
-  IOHIDDeviceRef ref = FindDevice(device_id);
-  if (ref == NULL)
+  IOHIDDeviceRef hid_device = FindDevice(device_id);
+  if (hid_device == NULL)
     return NULL;
   return scoped_refptr<HidConnection>(
-      new HidConnectionMac(this, devices_[device_id], ref));
+      new HidConnectionMac(this, devices_[device_id], hid_device));
 }
 
 }  // namespace device