Remove MessageLoop destruction observer from //device/usb

Remove MessageLoop destruction observer from //device/usb

This patch updates UsbDeviceHandleUsbfs::FileThreadHelper to no longer
be a MessageLoop::DestructionObserver and instead ensures that
UsbService::ShutDown will close all open connections, destroying the
FILE thread state.

BUG=650723
Review-Url: https://codereview.chromium.org/2797433005
Cr-Commit-Position: refs/heads/master@{#461960}
Committed: https://chromium.googlesource.com/chromium/src/+/56564d4099ebbaecf1cfb804cc09a6d198101fbb

[Reilly Grant (use Gerrit), 2017-04-04 17:22:54]

Description was changed from

==========
Merge branch 'device_monitor_linux' into settings_popup_crash


Remove MessageLoop destruction observer from //device/usb

This patch updates UsbDeviceHandleUsbfs::FileThreadHelper to no longer
be a MessageLoop::DestructionObserver and instead ensures that
UsbService::ShutDown will close all open connections, destroying the
FILE thread state.

BUG=
==========

to

==========
Remove MessageLoop destruction observer from //device/usb

This patch updates UsbDeviceHandleUsbfs::FileThreadHelper to no longer
be a MessageLoop::DestructionObserver and instead ensures that
UsbService::ShutDown will close all open connections, destroying the
FILE thread state.

BUG=
==========

[Reilly Grant (use Gerrit), 2017-04-04 17:22:54]

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

[Reilly Grant (use Gerrit), 2017-04-04 17:24:09]

Description was changed from

==========
Remove MessageLoop destruction observer from //device/usb

This patch updates UsbDeviceHandleUsbfs::FileThreadHelper to no longer
be a MessageLoop::DestructionObserver and instead ensures that
UsbService::ShutDown will close all open connections, destroying the
FILE thread state.

BUG=
==========

to

==========
Remove MessageLoop destruction observer from //device/usb

This patch updates UsbDeviceHandleUsbfs::FileThreadHelper to no longer
be a MessageLoop::DestructionObserver and instead ensures that
UsbService::ShutDown will close all open connections, destroying the
FILE thread state.

BUG=650723
==========

[Reilly Grant (use Gerrit), 2017-04-04 17:24:21]

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

[commit-bot: I haz the power, 2017-04-04 17:25:00]

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-04 17:25:14]

Please take a look.

[commit-bot: I haz the power, 2017-04-04 17:38:49]

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

[commit-bot: I haz the power, 2017-04-04 17:38:50]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Reilly Grant (use Gerrit), 2017-04-04 18:34:17]

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

[commit-bot: I haz the power, 2017-04-04 18:34:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scheib, 2017-04-04 20:17:15]

https://codereview.chromium.org/2797433005/diff/20001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.cc (right):

https://codereview.chromium.org/2797433005/diff/20001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:406: base::Bind(&FileThreadHelper::Start,
base::Unretained(helper_.get())));
blocking_task_runner is ref counted. And helper_ is unique ptr. How are we
certain that the blocking_task_runner is shut down before the helper_ is
destroyed?

https://codereview.chromium.org/2797433005/diff/20001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:617: ;
;

[Reilly Grant (use Gerrit), 2017-04-04 20:30:45]

Add back in the correct closed device checks

[Reilly Grant (use Gerrit), 2017-04-04 20:31:40]

https://codereview.chromium.org/2797433005/diff/20001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.cc (right):

https://codereview.chromium.org/2797433005/diff/20001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:406: base::Bind(&FileThreadHelper::Start,
base::Unretained(helper_.get())));
On 2017/04/04 20:17:15, scheib wrote:
> blocking_task_runner is ref counted. And helper_ is unique ptr. How are we
> certain that the blocking_task_runner is shut down before the helper_ is
> destroyed?

helper_ can only be destroyed on the FILE thread and that task is posted from
Close(). If device_ is not null then Close() has not yet been called.

https://codereview.chromium.org/2797433005/diff/20001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:617: ;
On 2017/04/04 20:17:15, scheib wrote:
> ;

Done.

[scheib, 2017-04-04 20:42:04]

https://codereview.chromium.org/2797433005/diff/40001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.cc (right):

https://codereview.chromium.org/2797433005/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:428: }
Could release reference to blocking_task_runner_ here.

[Reilly Grant (use Gerrit), 2017-04-04 23:51:38]

Further clarify cross-thread ownership

[Reilly Grant (use Gerrit), 2017-04-04 23:52:19]

https://codereview.chromium.org/2797433005/diff/40001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.cc (right):

https://codereview.chromium.org/2797433005/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:428: }
On 2017/04/04 20:42:04, scheib wrote:
> Could release reference to blocking_task_runner_ here.

It turns out that that doesn't work with the Android code but I've added more
documentation that should clarify the ownership semantics.

[scheib, 2017-04-05 00:51:28]

lgtm

[Reilly Grant (use Gerrit), 2017-04-05 01:29:15]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2017-04-05 01:30:16]

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-05 02:59:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491355755507570,
"parent_rev": "b638cb237c9a6a1ae7d360ec721b77381c4dbe3b", "commit_rev":
"56564d4099ebbaecf1cfb804cc09a6d198101fbb"}

[commit-bot: I haz the power, 2017-04-05 03:00:15]

Description was changed from

==========
Remove MessageLoop destruction observer from //device/usb

This patch updates UsbDeviceHandleUsbfs::FileThreadHelper to no longer
be a MessageLoop::DestructionObserver and instead ensures that
UsbService::ShutDown will close all open connections, destroying the
FILE thread state.

BUG=650723
==========

to

==========
Remove MessageLoop destruction observer from //device/usb

This patch updates UsbDeviceHandleUsbfs::FileThreadHelper to no longer
be a MessageLoop::DestructionObserver and instead ensures that
UsbService::ShutDown will close all open connections, destroying the
FILE thread state.

BUG=650723

Review-Url: https://codereview.chromium.org/2797433005
Cr-Commit-Position: refs/heads/master@{#461960}
Committed:
https://chromium.googlesource.com/chromium/src/+/56564d4099ebbaecf1cfb804cc09...
==========

[commit-bot: I haz the power, 2017-04-05 03:00:17]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/56564d4099ebbaecf1cfb804cc09...