Prevent timed-out USB transfers from being cancelled twice.

device/usb/usb_device_handle_usbfs.cc

 // Copyright 2016 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/usb/usb_device_handle_usbfs.h"
 
 #if defined(OS_ANDROID) && __ANDROID_API__ < 21
 #include <linux/usb_ch9.h>
 #else
 #include <linux/usb/ch9.h>
@@ skipping 218 matching lines @@
   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;
-  TransferCallback callback;
-  IsochronousTransferCallback isoc_callback;
-  scoped_refptr<base::SingleThreadTaskRunner> callback_runner;
   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;
 
   // 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;
 
  private:
+  TransferCallback callback;

[juncai, 2016/12/20 18:18:54]

The comment for the above |urb| variable says: "The |urb| field must be the last
in the struct ...", so by moving these three variables after |urb|, it may
affect the overridden new function?

[Reilly Grant (use Gerrit), 2016/12/20 20:28:59]

Done, good catch.

+  IsochronousTransferCallback isoc_callback;
+  scoped_refptr<base::SingleThreadTaskRunner> callback_runner;
+
   DISALLOW_COPY_AND_ASSIGN(Transfer);
 };
 
 UsbDeviceHandleUsbfs::Transfer::Transfer(
     scoped_refptr<net::IOBuffer> buffer,
     const TransferCallback& callback,
     scoped_refptr<base::SingleThreadTaskRunner> callback_runner)
     : buffer(buffer), callback(callback), callback_runner(callback_runner) {
   memset(&urb, 0, sizeof(urb));
   urb.usercontext = this;
@@ skipping 18 matching lines @@
   void* p = ::operator new(
       size + sizeof(usbdevfs_iso_packet_desc) * number_of_iso_packets);
   Transfer* transfer = static_cast<Transfer*>(p);
   transfer->urb.number_of_packets = number_of_iso_packets;
   return p;
 }
 
 void UsbDeviceHandleUsbfs::Transfer::RunCallback(UsbTransferStatus status,
                                                  size_t bytes_transferred) {
   DCHECK_NE(urb.type, USBDEVFS_URB_TYPE_ISO);
-  DCHECK(!callback.is_null());
+  DCHECK(callback);
   if (!callback_runner || callback_runner->BelongsToCurrentThread()) {
     callback.Run(status, buffer, bytes_transferred);
   } else {
     callback_runner->PostTask(
         FROM_HERE, base::Bind(callback, status, buffer, bytes_transferred));
   }
+  callback.Reset();
 }
 
 void UsbDeviceHandleUsbfs::Transfer::RunIsochronousCallback(
     const std::vector<IsochronousPacket>& packets) {
   DCHECK_EQ(urb.type, USBDEVFS_URB_TYPE_ISO);
-  DCHECK(!isoc_callback.is_null());
+  DCHECK(isoc_callback);
   isoc_callback.Run(buffer, packets);
+  isoc_callback.Reset();
 }
 
 UsbDeviceHandleUsbfs::UsbDeviceHandleUsbfs(
     scoped_refptr<UsbDevice> device,
     base::ScopedFD fd,
     scoped_refptr<base::SequencedTaskRunner> blocking_task_runner)
     : device_(device),
       fd_(std::move(fd)),
       blocking_task_runner_(blocking_task_runner) {
   DCHECK(device_);
@@ skipping 13 matching lines @@
   return device_;
 }
 
 void UsbDeviceHandleUsbfs::Close() {
   if (!device_)
     return;  // Already closed.
 
   // On the |task_runner_| thread check |device_| to see if the handle is
   // closed. On the |blocking_task_runner_| thread check |fd_.is_valid()| to
   // see if the handle is closed.
+  device_->HandleClosed(this);
+  device_ = nullptr;
+
   for (const auto& transfer : transfers_)
     CancelTransfer(transfer.get(), USB_TRANSFER_CANCELLED);
-  device_->HandleClosed(this);
-  device_ = nullptr;
+
   blocking_task_runner_->PostTask(
       FROM_HERE, base::Bind(&UsbDeviceHandleUsbfs::CloseBlocking, this));
 }
 
 void UsbDeviceHandleUsbfs::SetConfiguration(int configuration_value,
                                             const ResultCallback& callback) {
   // USBDEVFS_SETCONFIGURATION synchronously issues a SET_CONFIGURATION request
   // to the device so it must be performed on a thread where it is okay to
   // block.
   blocking_task_runner_->PostTask(
@@ skipping 485 matching lines @@
         return transfer.get() == transfer_ptr;
       });
   DCHECK(it != transfers_.end());
   std::unique_ptr<Transfer> transfer = std::move(*it);
   transfers_.erase(it);
   return transfer;
 }
 
 void UsbDeviceHandleUsbfs::CancelTransfer(Transfer* transfer,
                                           UsbTransferStatus status) {
+  if (transfer->cancelled)
+    return;
+
   // |transfer| must stay in |transfers_| as it is still being processed by the
   // kernel and will be reaped later.
   transfer->cancelled = true;
   transfer->timeout_closure.Cancel();
 
   if (transfer->urb.type == USBDEVFS_URB_TYPE_ISO) {
     std::vector<IsochronousPacket> packets(transfer->urb.number_of_packets);
     for (size_t i = 0; i < packets.size(); ++i) {
       packets[i].length = transfer->urb.iso_frame_desc[i].length;
       packets[i].transferred_length = 0;
@@ skipping 18 matching lines @@
       base::Bind(&UsbDeviceHandleUsbfs::UrbDiscarded, this, transfer));
 }
 
 void UsbDeviceHandleUsbfs::UrbDiscarded(Transfer* transfer) {
   transfer->discarded = true;
   if (transfer->reaped)
     RemoveFromTransferList(transfer);
 }
 
 }  // namespace device