[hid] Avoid calling UnlinkTransfer after Close

This CL addresses two issues:

In HidServiceWin, |platform_device_id_map| is initialized
incorrectly for devices that do not use report IDs. The item
added to the map incorrectly has an empty |report_ids| vector
which effectively means that the corresponding device ID will
never be used. To fix it, the |report_ids| vector is
initialized with a single element with value 0, indicating that
the corresponding device ID should be used for all reports.

In HidConnectionWin, when the connection is closed the |transfers_|
list is cleared. If |transfers_| was non-empty, this will invoke
the destructor of any PendingHidTransfer objects that were in the
list. If the callback calls UnlinkTransfer, the DCHECK is hit
since |transfers_| is already cleared. To fix it, the callbacks
should check |signaled| and only call UnlinkTransfer when the
file handle was signaled by the OS.

Bug: 1163277
Change-Id: Ic7897a9373afdd0a2045c8bc89bce7478f9ddf29
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612505Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840755}

services/device/hid/hid_connection_win.cc

@@ -229,10 +229,15 @@ void HidConnectionWin::OnReadInputReport(
     scoped_refptr<base::RefCountedBytes> buffer,
     PendingHidTransfer* transfer_raw,
     bool signaled) {
-  std::unique_ptr<PendingHidTransfer> transfer = UnlinkTransfer(transfer_raw);
+  if (!signaled) {
+    HID_LOG(DEBUG) << "HID read failed.";
+    return;
+  }
+
+  auto transfer = UnlinkTransfer(transfer_raw);
   DWORD bytes_transferred;
-  if (!signaled || !GetOverlappedResult(file_handle, transfer->GetOverlapped(),
-                                        &bytes_transferred, FALSE)) {
+  if (!GetOverlappedResult(file_handle, transfer->GetOverlapped(),
+                           &bytes_transferred, FALSE)) {
     HID_PLOG(DEBUG) << "HID read failed";
     return;
   }
@@ -243,16 +248,13 @@ void HidConnectionWin::OnReadInputReport(
   }
 
   uint8_t report_id = buffer->data()[0];
-  if (IsReportIdProtected(report_id, HidReportType::kInput)) {
-    ReadNextInputReportOnHandle(file_handle);
-    return;
+  if (!IsReportIdProtected(report_id, HidReportType::kInput)) {
+    // Hold a reference to |this| to prevent a callback executed by
+    // ProcessInputReport from freeing this object.
+    scoped_refptr<HidConnection> self(this);
+    ProcessInputReport(buffer, bytes_transferred);
   }
 
-  // Hold a reference to |this| to prevent a callback executed by
-  // ProcessInputReport from freeing this object.
-  scoped_refptr<HidConnection> self(this);
-  ProcessInputReport(buffer, bytes_transferred);
-
   ReadNextInputReportOnHandle(file_handle);
 }
 
@@ -262,31 +264,45 @@ void HidConnectionWin::OnReadFeatureComplete(
     ReadCallback callback,
     PendingHidTransfer* transfer_raw,
     bool signaled) {
-  std::unique_ptr<PendingHidTransfer> transfer = UnlinkTransfer(transfer_raw);
+  if (!signaled) {
+    HID_LOG(DEBUG) << "HID read failed.";
+    std::move(callback).Run(false, nullptr, 0);
+    return;
+  }
+
+  auto transfer = UnlinkTransfer(transfer_raw);
   DWORD bytes_transferred;
-  if (signaled && GetOverlappedResult(file_handle, transfer->GetOverlapped(),
-                                      &bytes_transferred, FALSE)) {
-    DCHECK_LE(bytes_transferred, buffer->size());
-    std::move(callback).Run(true, buffer, bytes_transferred);
-  } else {
+  if (!GetOverlappedResult(file_handle, transfer->GetOverlapped(),
+                           &bytes_transferred, FALSE)) {
     HID_PLOG(DEBUG) << "HID read failed";
     std::move(callback).Run(false, nullptr, 0);
+    return;
   }
+
+  DCHECK_LE(bytes_transferred, buffer->size());
+  std::move(callback).Run(true, buffer, bytes_transferred);
 }
 
 void HidConnectionWin::OnWriteComplete(HANDLE file_handle,
                                        WriteCallback callback,
                                        PendingHidTransfer* transfer_raw,
                                        bool signaled) {
-  std::unique_ptr<PendingHidTransfer> transfer = UnlinkTransfer(transfer_raw);
+  if (!signaled) {
+    HID_LOG(DEBUG) << "HID write failed.";
+    std::move(callback).Run(false);
+    return;
+  }
+
+  auto transfer = UnlinkTransfer(transfer_raw);
   DWORD bytes_transferred;
-  if (signaled && GetOverlappedResult(file_handle, transfer->GetOverlapped(),
-                                      &bytes_transferred, FALSE)) {
-    std::move(callback).Run(true);
-  } else {
+  if (!GetOverlappedResult(file_handle, transfer->GetOverlapped(),
+                           &bytes_transferred, FALSE)) {
     HID_PLOG(DEBUG) << "HID write failed";
     std::move(callback).Run(false);
+    return;
   }
+
+  std::move(callback).Run(true);
 }
 
 std::unique_ptr<PendingHidTransfer> HidConnectionWin::UnlinkTransfer(

services/device/hid/hid_service_win.cc

@@ -715,7 +715,10 @@ void HidServiceWin::AddDeviceBlocking(
     // Create a HidCollectionInfo for |device_path| and update the relevant
     // HidDeviceInfo properties.
     auto collection = preparsed_data->CreateHidCollectionInfo();
-    platform_device_id_map.emplace_back(collection->report_ids, device_path);
+    if (collection->report_ids.empty())
+      platform_device_id_map.emplace_back(std::vector<uint8_t>{0}, device_path);
+    else
+      platform_device_id_map.emplace_back(collection->report_ids, device_path);
     collections.push_back(std::move(collection));
     max_input_report_size = std::max(
         max_input_report_size, preparsed_data->GetReportByteLength(HidP_Input));