Remove MessageLoop destruction observer from //device/usb

device/usb/usb_device_handle_usbfs.cc

Index: device/usb/usb_device_handle_usbfs.cc
diff --git a/device/usb/usb_device_handle_usbfs.cc b/device/usb/usb_device_handle_usbfs.cc
index 791666605d4d11416d3dabacaa783e5cc7eda386..e347fe4dd228092425df7f26e6b823c0630e001a 100644
--- a/device/usb/usb_device_handle_usbfs.cc
+++ b/device/usb/usb_device_handle_usbfs.cc
@@ -19,7 +19,6 @@
 #include "base/cancelable_callback.h"
 #include "base/files/file_descriptor_watcher_posix.h"
 #include "base/logging.h"
-#include "base/message_loop/message_loop.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/stl_util.h"
 #include "base/threading/thread_restrictions.h"
@@ -134,18 +133,24 @@ UsbTransferStatus ConvertTransferResult(int rc) {
 
 }  // namespace
 
-class UsbDeviceHandleUsbfs::FileThreadHelper
-    : public base::MessageLoop::DestructionObserver {
+class UsbDeviceHandleUsbfs::FileThreadHelper {
  public:
   FileThreadHelper(int fd,
                    scoped_refptr<UsbDeviceHandleUsbfs> device_handle,
                    scoped_refptr<base::SequencedTaskRunner> task_runner);
-  ~FileThreadHelper() override;
+  ~FileThreadHelper();
 
-  static void Start(std::unique_ptr<FileThreadHelper> self);
+  void Start();
 
-  // base::MessageLoop::DestructionObserver overrides.
-  void WillDestroyCurrentMessageLoop() override;
+  void SetConfiguration(int configuration_value,
+                        const ResultCallback& callback);
+  void ReleaseInterface(int interface_number, const ResultCallback& callback);
+  void SetInterface(int interface_number,
+                    int alternate_setting,
+                    const ResultCallback& callback);
+  void ResetDevice(const ResultCallback& callback);
+  void ClearHalt(uint8_t endpoint_address, const ResultCallback& callback);
+  void DiscardUrb(Transfer* transfer);
 
  private:
   // Called when |fd_| is writable without blocking.
@@ -160,6 +165,44 @@ class UsbDeviceHandleUsbfs::FileThreadHelper
   DISALLOW_COPY_AND_ASSIGN(FileThreadHelper);
 };
 
+struct UsbDeviceHandleUsbfs::Transfer {
+  Transfer() = delete;
+  Transfer(scoped_refptr<net::IOBuffer> buffer,
+           const TransferCallback& callback,
+           scoped_refptr<base::SingleThreadTaskRunner> callback_runner);
+  Transfer(scoped_refptr<net::IOBuffer> buffer,
+           const IsochronousTransferCallback& callback);
+  ~Transfer();
+
+  void* operator new(std::size_t size, size_t number_of_iso_packets);
+  void RunCallback(UsbTransferStatus status, size_t bytes_transferred);
+  void RunIsochronousCallback(const std::vector<IsochronousPacket>& packets);
+
+  scoped_refptr<net::IOBuffer> control_transfer_buffer;
+  scoped_refptr<net::IOBuffer> buffer;
+  base::CancelableClosure timeout_closure;
+  bool cancelled = false;
+
+  // When the URB is |cancelled| these two flags track whether the URB has both
+  // been |discarded| and |reaped| since the possiblity of last-minute
+  // completion makes these two conditions race.
+  bool discarded = false;
+  bool reaped = false;
+
+ private:
+  TransferCallback callback;
+  IsochronousTransferCallback isoc_callback;
+  scoped_refptr<base::SingleThreadTaskRunner> callback_runner;
+
+  DISALLOW_COPY_AND_ASSIGN(Transfer);
+
+ public:
+  // The |urb| field must be the last in the struct so that the extra space
+  // allocated by the overridden new function above extends the length of its
+  // |iso_frame_desc| field.
+  usbdevfs_urb urb;
+};
+
 UsbDeviceHandleUsbfs::FileThreadHelper::FileThreadHelper(
     int fd,
     scoped_refptr<UsbDeviceHandleUsbfs> device_handle,
@@ -168,28 +211,98 @@ UsbDeviceHandleUsbfs::FileThreadHelper::FileThreadHelper(
 
 UsbDeviceHandleUsbfs::FileThreadHelper::~FileThreadHelper() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  base::MessageLoop::current()->RemoveDestructionObserver(this);
 }
 
-// static
-void UsbDeviceHandleUsbfs::FileThreadHelper::Start(
-    std::unique_ptr<FileThreadHelper> self) {
+void UsbDeviceHandleUsbfs::FileThreadHelper::Start() {
   base::ThreadRestrictions::AssertIOAllowed();
-  self->thread_checker_.DetachFromThread();
+  thread_checker_.DetachFromThread();
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   // Linux indicates that URBs are available to reap by marking the file
   // descriptor writable.
-  self->watch_controller_ = base::FileDescriptorWatcher::WatchWritable(
-      self->fd_, base::Bind(&FileThreadHelper::OnFileCanWriteWithoutBlocking,
-                            base::Unretained(self.get())));
+  watch_controller_ = base::FileDescriptorWatcher::WatchWritable(
+      fd_, base::Bind(&FileThreadHelper::OnFileCanWriteWithoutBlocking,
+                      base::Unretained(this)));
+}
 
-  // |self| is now owned by the current message loop.
-  base::MessageLoop::current()->AddDestructionObserver(self.release());
+void UsbDeviceHandleUsbfs::FileThreadHelper::SetConfiguration(
+    int configuration_value,
+    const ResultCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  int rc =
+      HANDLE_EINTR(ioctl(fd_, USBDEVFS_SETCONFIGURATION, &configuration_value));
+  if (rc)
+    USB_PLOG(DEBUG) << "Failed to set configuration " << configuration_value;
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&UsbDeviceHandleUsbfs::SetConfigurationComplete,
+                 device_handle_, configuration_value, rc == 0, callback));
 }
 
-void UsbDeviceHandleUsbfs::FileThreadHelper::WillDestroyCurrentMessageLoop() {
+void UsbDeviceHandleUsbfs::FileThreadHelper::ReleaseInterface(
+    int interface_number,
+    const ResultCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  delete this;
+  int rc =
+      HANDLE_EINTR(ioctl(fd_, USBDEVFS_RELEASEINTERFACE, &interface_number));
+  if (rc) {
+    USB_PLOG(DEBUG) << "Failed to release interface " << interface_number;
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
+  } else {
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::ReleaseInterfaceComplete,
+                              device_handle_, interface_number, callback));
+  }
+}
+
+void UsbDeviceHandleUsbfs::FileThreadHelper::SetInterface(
+    int interface_number,
+    int alternate_setting,
+    const ResultCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  usbdevfs_setinterface cmd = {0};
+  cmd.interface = interface_number;
+  cmd.altsetting = alternate_setting;
+  int rc = HANDLE_EINTR(ioctl(fd_, USBDEVFS_SETINTERFACE, &cmd));
+  if (rc) {
+    USB_PLOG(DEBUG) << "Failed to set interface " << interface_number
+                    << " to alternate setting " << alternate_setting;
+  }
+  task_runner_->PostTask(FROM_HERE, base::Bind(callback, rc == 0));
+}
+
+void UsbDeviceHandleUsbfs::FileThreadHelper::ResetDevice(
+    const ResultCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // TODO(reillyg): libusb releases interfaces before and then reclaims
+  // interfaces after a reset. We should probably do this too or document that
+  // callers have to call ClaimInterface as well.
+  int rc = HANDLE_EINTR(ioctl(fd_, USBDEVFS_RESET, nullptr));
+  if (rc)
+    USB_PLOG(DEBUG) << "Failed to reset the device";
+  task_runner_->PostTask(FROM_HERE, base::Bind(callback, rc == 0));
+}
+
+void UsbDeviceHandleUsbfs::FileThreadHelper::ClearHalt(
+    uint8_t endpoint_address,
+    const ResultCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  int tmp_endpoint = endpoint_address;
+  int rc = HANDLE_EINTR(ioctl(fd_, USBDEVFS_CLEAR_HALT, &tmp_endpoint));
+  if (rc) {
+    USB_PLOG(DEBUG) << "Failed to clear the stall condition on endpoint "
+                    << static_cast<int>(endpoint_address);
+  }
+  task_runner_->PostTask(FROM_HERE, base::Bind(callback, rc == 0));
+}
+
+void UsbDeviceHandleUsbfs::FileThreadHelper::DiscardUrb(Transfer* transfer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  HANDLE_EINTR(ioctl(fd_, USBDEVFS_DISCARDURB, &transfer->urb));
+
+  task_runner_->PostTask(
+      FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::UrbDiscarded, device_handle_,
+                            transfer));
 }
 
 void UsbDeviceHandleUsbfs::FileThreadHelper::OnFileCanWriteWithoutBlocking() {
@@ -221,44 +334,6 @@ void UsbDeviceHandleUsbfs::FileThreadHelper::OnFileCanWriteWithoutBlocking() {
       base::Bind(&UsbDeviceHandleUsbfs::ReapedUrbs, device_handle_, urbs));
 }
 
-struct UsbDeviceHandleUsbfs::Transfer {
-  Transfer() = delete;
-  Transfer(scoped_refptr<net::IOBuffer> buffer,
-           const TransferCallback& callback,
-           scoped_refptr<base::SingleThreadTaskRunner> callback_runner);
-  Transfer(scoped_refptr<net::IOBuffer> buffer,
-           const IsochronousTransferCallback& callback);
-  ~Transfer();
-
-  void* operator new(std::size_t size, size_t number_of_iso_packets);
-  void RunCallback(UsbTransferStatus status, size_t bytes_transferred);
-  void RunIsochronousCallback(const std::vector<IsochronousPacket>& packets);
-
-  scoped_refptr<net::IOBuffer> control_transfer_buffer;
-  scoped_refptr<net::IOBuffer> buffer;
-  base::CancelableClosure timeout_closure;
-  bool cancelled = false;
-
-  // When the URB is |cancelled| these two flags track whether the URB has both
-  // been |discarded| and |reaped| since the possiblity of last-minute
-  // completion makes these two conditions race.
-  bool discarded = false;
-  bool reaped = false;
-
- private:
-  TransferCallback callback;
-  IsochronousTransferCallback isoc_callback;
-  scoped_refptr<base::SingleThreadTaskRunner> callback_runner;
-
-  DISALLOW_COPY_AND_ASSIGN(Transfer);
-
- public:
-  // The |urb| field must be the last in the struct so that the extra space
-  // allocated by the overridden new function above extends the length of its
-  // |iso_frame_desc| field.
-  usbdevfs_urb urb;
-};
-
 UsbDeviceHandleUsbfs::Transfer::Transfer(
     scoped_refptr<net::IOBuffer> buffer,
     const TransferCallback& callback,
@@ -325,11 +400,10 @@ UsbDeviceHandleUsbfs::UsbDeviceHandleUsbfs(
 
   task_runner_ = base::ThreadTaskRunnerHandle::Get();
 
-  std::unique_ptr<FileThreadHelper> helper(
-      new FileThreadHelper(fd_.get(), this, task_runner_));
-  helper_ = helper.get();
+  helper_.reset(new FileThreadHelper(fd_.get(), this, task_runner_));
   blocking_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&FileThreadHelper::Start, base::Passed(&helper)));
+      FROM_HERE,
+      base::Bind(&FileThreadHelper::Start, base::Unretained(helper_.get())));

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

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?

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

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.

 }
 
 scoped_refptr<UsbDevice> UsbDeviceHandleUsbfs::GetDevice() const {
@@ -359,8 +433,10 @@ void UsbDeviceHandleUsbfs::SetConfiguration(int configuration_value,
   // to the device so it must be performed on a thread where it is okay to
   // block.
   blocking_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::SetConfigurationBlocking,
-                            this, configuration_value, callback));
+      FROM_HERE,
+      base::Bind(&UsbDeviceHandleUsbfs::FileThreadHelper::SetConfiguration,
+                 base::Unretained(helper_.get()), configuration_value,
+                 callback));
 }
 
 void UsbDeviceHandleUsbfs::ClaimInterface(int interface_number,
@@ -394,8 +470,9 @@ void UsbDeviceHandleUsbfs::ReleaseInterface(int interface_number,
   // device to restore alternate setting 0 so it must be performed on a thread
   // where it is okay to block.
   blocking_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::ReleaseInterfaceBlocking,
-                            this, interface_number, callback));
+      FROM_HERE,
+      base::Bind(&UsbDeviceHandleUsbfs::FileThreadHelper::ReleaseInterface,
+                 base::Unretained(helper_.get()), interface_number, callback));
 }
 
 void UsbDeviceHandleUsbfs::SetInterfaceAlternateSetting(
@@ -406,8 +483,10 @@ void UsbDeviceHandleUsbfs::SetInterfaceAlternateSetting(
   // request to the device so it must be performed on a thread where it is okay
   // to block.
   blocking_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::SetInterfaceBlocking, this,
-                            interface_number, alternate_setting, callback));
+      FROM_HERE,
+      base::Bind(&UsbDeviceHandleUsbfs::FileThreadHelper::SetInterface,
+                 base::Unretained(helper_.get()), interface_number,
+                 alternate_setting, callback));
 }
 
 void UsbDeviceHandleUsbfs::ResetDevice(const ResultCallback& callback) {
@@ -416,7 +495,8 @@ void UsbDeviceHandleUsbfs::ResetDevice(const ResultCallback& callback) {
   // is okay to block.
   blocking_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&UsbDeviceHandleUsbfs::ResetDeviceBlocking, this, callback));
+      base::Bind(&UsbDeviceHandleUsbfs::FileThreadHelper::ResetDevice,
+                 base::Unretained(helper_.get()), callback));
 }
 
 void UsbDeviceHandleUsbfs::ClearHalt(uint8_t endpoint_address,
@@ -425,8 +505,9 @@ void UsbDeviceHandleUsbfs::ClearHalt(uint8_t endpoint_address,
   // request to the device so it must be performed on a thread where it is okay
   // to block.
   blocking_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::ClearHaltBlocking, this,
-                            endpoint_address, callback));
+      FROM_HERE,
+      base::Bind(&UsbDeviceHandleUsbfs::FileThreadHelper::ClearHalt,
+                 base::Unretained(helper_.get()), endpoint_address, callback));
 }
 
 void UsbDeviceHandleUsbfs::ControlTransfer(UsbEndpointDirection direction,
@@ -527,29 +608,13 @@ UsbDeviceHandleUsbfs::~UsbDeviceHandleUsbfs() {
 
 void UsbDeviceHandleUsbfs::ReleaseFileDescriptor() {
   ignore_result(fd_.release());
-  delete helper_;
+  helper_.reset();
 }
 
 void UsbDeviceHandleUsbfs::CloseBlocking() {
   fd_.reset(-1);
-  delete helper_;
-}
-
-void UsbDeviceHandleUsbfs::SetConfigurationBlocking(
-    int configuration_value,
-    const ResultCallback& callback) {
-  if (!fd_.is_valid()) {
-    task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
-    return;
-  }
-
-  int rc = HANDLE_EINTR(
-      ioctl(fd_.get(), USBDEVFS_SETCONFIGURATION, &configuration_value));
-  if (rc)
-    USB_PLOG(DEBUG) << "Failed to set configuration " << configuration_value;
-  task_runner_->PostTask(
-      FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::SetConfigurationComplete,
-                            this, configuration_value, rc == 0, callback));
+  helper_.reset();
+  ;

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

;

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

Done.

 }
 
 void UsbDeviceHandleUsbfs::SetConfigurationComplete(
@@ -565,26 +630,6 @@ void UsbDeviceHandleUsbfs::SetConfigurationComplete(
   callback.Run(success);
 }
 
-void UsbDeviceHandleUsbfs::ReleaseInterfaceBlocking(
-    int interface_number,
-    const ResultCallback& callback) {
-  if (!fd_.is_valid()) {
-    task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
-    return;
-  }
-
-  int rc = HANDLE_EINTR(
-      ioctl(fd_.get(), USBDEVFS_RELEASEINTERFACE, &interface_number));
-  if (rc) {
-    USB_PLOG(DEBUG) << "Failed to release interface " << interface_number;
-    task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
-  } else {
-    task_runner_->PostTask(
-        FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::ReleaseInterfaceComplete,
-                              this, interface_number, callback));
-  }
-}
-
 void UsbDeviceHandleUsbfs::ReleaseInterfaceComplete(
     int interface_number,
     const ResultCallback& callback) {
@@ -595,57 +640,6 @@ void UsbDeviceHandleUsbfs::ReleaseInterfaceComplete(
   callback.Run(true);
 }
 
-void UsbDeviceHandleUsbfs::SetInterfaceBlocking(
-    int interface_number,
-    int alternate_setting,
-    const ResultCallback& callback) {
-  if (!fd_.is_valid()) {
-    task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
-    return;
-  }
-
-  usbdevfs_setinterface cmd = {0};
-  cmd.interface = interface_number;
-  cmd.altsetting = alternate_setting;
-  int rc = HANDLE_EINTR(ioctl(fd_.get(), USBDEVFS_SETINTERFACE, &cmd));
-  if (rc) {
-    USB_PLOG(DEBUG) << "Failed to set interface " << interface_number
-                    << " to alternate setting " << alternate_setting;
-  }
-  task_runner_->PostTask(FROM_HERE, base::Bind(callback, rc == 0));
-}
-
-void UsbDeviceHandleUsbfs::ResetDeviceBlocking(const ResultCallback& callback) {
-  if (!fd_.is_valid()) {
-    task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
-    return;
-  }
-
-  // TODO(reillyg): libusb releases interfaces before and then reclaims
-  // interfaces after a reset. We should probably do this too or document that
-  // callers have to call ClaimInterface as well.
-  int rc = HANDLE_EINTR(ioctl(fd_.get(), USBDEVFS_RESET, nullptr));
-  if (rc)
-    USB_PLOG(DEBUG) << "Failed to reset the device";
-  task_runner_->PostTask(FROM_HERE, base::Bind(callback, rc == 0));
-}
-
-void UsbDeviceHandleUsbfs::ClearHaltBlocking(uint8_t endpoint_address,
-                                             const ResultCallback& callback) {
-  if (!fd_.is_valid()) {
-    task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
-    return;
-  }
-
-  int tmp_endpoint = endpoint_address;
-  int rc = HANDLE_EINTR(ioctl(fd_.get(), USBDEVFS_CLEAR_HALT, &tmp_endpoint));
-  if (rc) {
-    USB_PLOG(DEBUG) << "Failed to clear the stall condition on endpoint "
-                    << static_cast<int>(endpoint_address);
-  }
-  task_runner_->PostTask(FROM_HERE, base::Bind(callback, rc == 0));
-}
-
 void UsbDeviceHandleUsbfs::IsochronousTransferInternal(
     uint8_t endpoint_address,
     scoped_refptr<net::IOBuffer> buffer,
@@ -875,18 +869,10 @@ void UsbDeviceHandleUsbfs::CancelTransfer(Transfer* transfer,
   }
 
   blocking_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&UsbDeviceHandleUsbfs::DiscardUrbBlocking, this, transfer));
+      FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::FileThreadHelper::DiscardUrb,
+                            base::Unretained(helper_.get()), transfer));
 }
 
-void UsbDeviceHandleUsbfs::DiscardUrbBlocking(Transfer* transfer) {
-  if (fd_.is_valid())
-    HANDLE_EINTR(ioctl(fd_.get(), USBDEVFS_DISCARDURB, &transfer->urb));
-
-  task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&UsbDeviceHandleUsbfs::UrbDiscarded, this, transfer));
-}
 
 void UsbDeviceHandleUsbfs::UrbDiscarded(Transfer* transfer) {
   transfer->discarded = true;