Fix crash when closing a device from a USB transfer timeout

Fix crash when closing a device from a USB transfer timeout

Code such as the USB device enumeration logic will close the device
immediately in response to a transfer error. This would cause
reentrancy into the CancelTransfer function, leading to a crash. This
patch updates UsbDeviceHandleUsbfs::Close and CancelTransfer to be
reentrancy safe and adds a test case that reproduces the crash without
this change applied.

BUG=714770
Review-Url: https://codereview.chromium.org/2843613002
Cr-Commit-Position: refs/heads/master@{#466815}
Committed: https://chromium.googlesource.com/chromium/src/+/7ac2374e5a71a1c49f8cf433726ce6f4afb81c4e

[Reilly Grant (use Gerrit), 2017-04-24 20:10:37]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 20:11:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Reilly Grant (use Gerrit), 2017-04-24 20:14:39]

reillyg@chromium.org changed reviewers:
+ mcasas@chromium.org

[Reilly Grant (use Gerrit), 2017-04-24 20:14:39]

PTAL

[commit-bot: I haz the power, 2017-04-24 21:03:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 21:03:48]

Dry run: This issue passed the CQ dry run.

[mcasas, 2017-04-24 21:19:23]

lgtm

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
File device/usb/usb_device_handle_unittest.cc (right):

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
device/usb/usb_device_handle_unittest.cc:124: EXPECT_EQ(status,
UsbTransferStatus::TIMEOUT);
nit: EXPECT_EQ(expected, actual) [1]  so reverse them.

(confusingly enough, gtest is opposite from the js test harness)

[1]
https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?type...

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
device/usb/usb_device_handle_unittest.cc:439:
ASSERT_TRUE(claim_interface.WaitForResult());
nit: This preamble l.424-439 looks very similar to e.g.
l. 392-408 and l.359-375. Consider refactoring them out.

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
File device/usb/usb_device_handle_usbfs.cc (right):

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
device/usb/usb_device_handle_usbfs.cc:897: if (timeout > 0) {
nit: (timeout is unsigned int).
if (timeout == 0)
  return;
// the rest.

[Reilly Grant (use Gerrit), 2017-04-24 22:14:01]

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
File device/usb/usb_device_handle_unittest.cc (right):

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
device/usb/usb_device_handle_unittest.cc:124: EXPECT_EQ(status,
UsbTransferStatus::TIMEOUT);
On 2017/04/24 21:19:23, mcasas wrote:
> nit: EXPECT_EQ(expected, actual) [1]  so reverse them.
> 
> (confusingly enough, gtest is opposite from the js test harness)
> 
> [1]
>
https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?type...

Done.

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
device/usb/usb_device_handle_unittest.cc:439:
ASSERT_TRUE(claim_interface.WaitForResult());
On 2017/04/24 21:19:22, mcasas wrote:
> nit: This preamble l.424-439 looks very similar to e.g.
> l. 392-408 and l.359-375. Consider refactoring them out.

The preamble varies subtly depending on what kind of transfer we're testing.

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
File device/usb/usb_device_handle_usbfs.cc (right):

https://codereview.chromium.org/2843613002/diff/1/device/usb/usb_device_handl...
device/usb/usb_device_handle_usbfs.cc:897: if (timeout > 0) {
On 2017/04/24 21:19:23, mcasas wrote:
> nit: (timeout is unsigned int).
> if (timeout == 0)
>   return;
> // the rest.

Done.

[Reilly Grant (use Gerrit), 2017-04-24 22:14:30]

Address mcasas@ nits

[Reilly Grant (use Gerrit), 2017-04-24 22:16:33]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2017-04-24 22:16:34]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2843613002/#ps20001
(title: "Address mcasas@ nits")

[commit-bot: I haz the power, 2017-04-24 22:17:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 23:12:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493072193668010,
"parent_rev": "84e360a7f1dddd9ce125ad6f1de090b7d207c569", "commit_rev":
"7ac2374e5a71a1c49f8cf433726ce6f4afb81c4e"}

[commit-bot: I haz the power, 2017-04-24 23:13:00]

Description was changed from

==========
Fix crash when closing a device from a USB transfer timeout

Code such as the USB device enumeration logic will close the device
immediately in response to a transfer error. This would cause
reentrancy into the CancelTransfer function, leading to a crash. This
patch updates UsbDeviceHandleUsbfs::Close and CancelTransfer to be
reentrancy safe and adds a test case that reproduces the crash without
this change applied.

BUG=714770
==========

to

==========
Fix crash when closing a device from a USB transfer timeout

Code such as the USB device enumeration logic will close the device
immediately in response to a transfer error. This would cause
reentrancy into the CancelTransfer function, leading to a crash. This
patch updates UsbDeviceHandleUsbfs::Close and CancelTransfer to be
reentrancy safe and adds a test case that reproduces the crash without
this change applied.

BUG=714770

Review-Url: https://codereview.chromium.org/2843613002
Cr-Commit-Position: refs/heads/master@{#466815}
Committed:
https://chromium.googlesource.com/chromium/src/+/7ac2374e5a71a1c49f8cf433726c...
==========

[commit-bot: I haz the power, 2017-04-24 23:13:02]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7ac2374e5a71a1c49f8cf433726c...