DevTools: do not retain android_usb_devices in their manager.

chrome/browser/devtools/adb/android_usb_device.cc

 // Copyright (c) 2013 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 "chrome/browser/devtools/adb/android_usb_device.h"
 
 #include <set>
 
 #include "base/barrier_closure.h"
 #include "base/base64.h"
@@ skipping 27 matching lines @@
 const uint32 kMaxPayload = 4096;
 const uint32 kVersion = 0x01000000;
 
 static const char kHostConnectMessage[] = "host::";
 
 using content::BrowserThread;
 
 typedef std::vector<scoped_refptr<UsbDevice> > UsbDevices;
 typedef std::set<scoped_refptr<UsbDevice> > UsbDeviceSet;
 
-base::LazyInstance<AndroidUsbDevices>::Leaky g_devices =
+// Stores android wrappers around claimed usb devices on caller thread.
+base::LazyInstance<std::vector<AndroidUsbDevice*> >::Leaky g_devices =
     LAZY_INSTANCE_INITIALIZER;
 
 bool IsAndroidInterface(
     scoped_refptr<const UsbInterfaceDescriptor> interface) {
   if (interface->GetNumAltSettings() == 0)
     return false;
 
   scoped_refptr<const UsbInterfaceAltSettingDescriptor> idesc =
       interface->GetAltSetting(0);
 
@@ skipping 34 matching lines @@
     return NULL;
 
   if (!usb_handle->ClaimInterface(interface_id))
     return NULL;
 
   base::string16 serial;
   if (!usb_handle->GetSerial(&serial) || serial.empty())
     return NULL;
 
   return new AndroidUsbDevice(rsa_key, usb_handle, base::UTF16ToASCII(serial),
-                              inbound_address, outbound_address, zero_mask);
+                              inbound_address, outbound_address, zero_mask,
+                              interface_id);
 }
 
 uint32 Checksum(const std::string& data) {
   unsigned char* x = (unsigned char*)data.data();
   int count = data.length();
   uint32 sum = 0;
   while (count-- > 0)
     sum += *x++;
   return sum;
 }
@@ skipping 20 matching lines @@
       if (data[i] >= 0x20 && data[i] <= 0x7E)
         result += data[i];
       else
         result += ".";
     }
   }
   LOG(ERROR) << (outgoing ? "[out] " : "[ in] ") << result;
 #endif  // 0
 }
 
-void ReleaseInterface(scoped_refptr<UsbDeviceHandle> usb_device) {
-  usb_device->ReleaseInterface(1);
+void ReleaseInterface(scoped_refptr<UsbDeviceHandle> usb_device,
+                      int interface_id) {
+  usb_device->ReleaseInterface(interface_id);
   usb_device->Close();
 }
 
 }  // namespace
 
 AdbMessage::AdbMessage(uint32 command,
                        uint32 arg0,
                        uint32 arg1,
                        const std::string& body)
     : command(command),
       arg0(arg0),
       arg1(arg1),
       body(body) {
 }
 
 AdbMessage::~AdbMessage() {
 }
 
 static void RespondWithCountOnUIThread(base::Callback<void(int)> callback,
                                        int count) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   callback.Run(count);
 }
 
 static void RespondOnCallerThread(const AndroidUsbDevicesCallback& callback,
-                                  const AndroidUsbDevices& devices) {
-  callback.Run(devices);
+                                  AndroidUsbDevices* claimed_devices) {

[Vladislav Kaznacheev, 2014/04/10 14:38:11]

Unclear name (already claimed or newly claimed?) 
new_devices seems a better name.

[pfeldman, 2014/04/10 15:37:46]

Done.

+  scoped_ptr<AndroidUsbDevices> devices(claimed_devices);
+
+  // Add raw pointers to the newly claimed devices.
+  for (AndroidUsbDevices::iterator it = devices->begin(); it != devices->end();
+       ++it) {
+    g_devices.Get().push_back(*it);
+  }
+
+  // Return all claimed devices.
+  AndroidUsbDevices result(g_devices.Get().begin(), g_devices.Get().end());
+  callback.Run(result);
 }
 
 static void RespondOnFileThread(
     const AndroidUsbDevicesCallback& callback,
+    AndroidUsbDevices* devices,
     scoped_refptr<base::MessageLoopProxy> caller_message_loop_proxy) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  // Copy g_devices.Get() on file thread.
   caller_message_loop_proxy->PostTask(
       FROM_HERE,
-      base::Bind(&RespondOnCallerThread, callback, g_devices.Get()));
+      base::Bind(&RespondOnCallerThread, callback, devices));
 }
 
 static void OpenAndroidDevicesOnFileThread(

[Vladislav Kaznacheev, 2014/04/10 14:38:11]

's' at the end of 'devices' seems out of place

[pfeldman, 2014/04/10 15:37:46]

Done.

+    AndroidUsbDevices* devices,
     crypto::RSAPrivateKey* rsa_key,
     const base::Closure& barrier,
     scoped_refptr<UsbDevice> device,
     int interface_id,
     bool success) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   if (success) {
     scoped_refptr<UsbConfigDescriptor> config = device->ListInterfaces();
     scoped_refptr<UsbDeviceHandle> usb_handle = device->Open();
     if (usb_handle) {
-      scoped_refptr<AndroidUsbDevice> device =
+      scoped_refptr<AndroidUsbDevice> android_device =
         ClaimInterface(rsa_key, usb_handle, config->GetInterface(interface_id),
                        interface_id);
-      if (device.get())
-        g_devices.Get().push_back(device);
+      if (android_device.get())
+        devices->push_back(android_device.get());
       else
         usb_handle->Close();
     }
   }
   barrier.Run();
 }
 
 static void CountOnFileThread(
     const base::Callback<void(int)>& callback) {
   UsbService* service = UsbService::GetInstance();
@@ skipping 21 matching lines @@
     crypto::RSAPrivateKey* rsa_key,
     const AndroidUsbDevicesCallback& callback,
     scoped_refptr<base::MessageLoopProxy> caller_message_loop_proxy) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   UsbService* service = UsbService::GetInstance();
   UsbDevices usb_devices;
   if (service != NULL)
     service->GetDevices(&usb_devices);
 
-  AndroidUsbDevices& devices = g_devices.Get();
-
-  // GC Android devices with no actual usb device.
-  AndroidUsbDevices::iterator it = devices.begin();
-  UsbDeviceSet claimed_devices;
-  while (it != devices.end()) {
-    bool found_device = false;
-    for (UsbDevices::iterator it2 = usb_devices.begin();
-         it2 != usb_devices.end() && !found_device; ++it2) {
-      UsbDevice* usb_device = it2->get();
-      AndroidUsbDevice* device = it->get();
-      if (usb_device == device->usb_device()->device()) {
-        found_device = true;
-        claimed_devices.insert(usb_device);
-      }
-    }
-
-    if (!found_device)
-      it = devices.erase(it);
-    else
-      ++it;
-  }
-
   // Add new devices.
+  AndroidUsbDevices* devices = new AndroidUsbDevices();
   base::Closure barrier = base::BarrierClosure(
       usb_devices.size(), base::Bind(&RespondOnFileThread,
                                      callback,
+                                     devices,

[Vladislav Kaznacheev, 2014/04/10 14:38:11]

I suggest using base::Owned for this parameter.

[pfeldman, 2014/04/10 15:37:46]

Done.

                                      caller_message_loop_proxy));
 
   for (UsbDevices::iterator it = usb_devices.begin(); it != usb_devices.end();
        ++it) {
-    if (ContainsKey(claimed_devices, it->get())) {
-      barrier.Run();
-      continue;
-    }
-
     scoped_refptr<UsbConfigDescriptor> config = (*it)->ListInterfaces();
     if (!config) {
       barrier.Run();
       continue;
     }
 
     bool has_android_interface = false;
     for (size_t j = 0; j < config->GetNumInterfaces(); ++j) {
       if (!IsAndroidInterface(config->GetInterface(j)))
         continue;
 
       // Request permission on Chrome OS.
 #if defined(OS_CHROMEOS)
       (*it)->RequestUsbAcess(j, base::Bind(&OpenAndroidDevicesOnFileThread,
-                                           rsa_key, barrier, *it, j));
+                                           devices, rsa_key, barrier, *it, j));
 #else
-      OpenAndroidDevicesOnFileThread(rsa_key, barrier, *it, j, true);
+      OpenAndroidDevicesOnFileThread(devices, rsa_key, barrier, *it, j, true);
 #endif  // defined(OS_CHROMEOS)
 
       has_android_interface = true;
       break;
     }
     if (!has_android_interface)
       barrier.Run();
   }
 }
 
 // static
 void AndroidUsbDevice::CountDevices(
     const base::Callback<void(int)>& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                           base::Bind(&CountOnFileThread, callback));
 }
 
 // static
 void AndroidUsbDevice::Enumerate(crypto::RSAPrivateKey* rsa_key,
                                  const AndroidUsbDevicesCallback& callback) {
+
+  // Collect devices with closed handles.
+  for (std::vector<AndroidUsbDevice*>::iterator it = g_devices.Get().begin();
+       it != g_devices.Get().end(); ++it) {
+    if (!(*it)->terminated_) {

[Vladislav Kaznacheev, 2014/04/10 14:38:11]

AndroidUsbDevice::terminated() exists (but not used yet)

[pfeldman, 2014/04/10 15:37:46]

Done.

+      BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+          base::Bind(&AndroidUsbDevice::TerminateIfReleased, *it));
+    }
+  }
+
+  // Then look for the new devices.
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                           base::Bind(&EnumerateOnFileThread, rsa_key, callback,
                                      base::MessageLoopProxy::current()));
 }
 
 AndroidUsbDevice::AndroidUsbDevice(crypto::RSAPrivateKey* rsa_key,
                                    scoped_refptr<UsbDeviceHandle> usb_device,
                                    const std::string& serial,
                                    int inbound_address,
                                    int outbound_address,
-                                   int zero_mask)
+                                   int zero_mask,
+                                   int interface_id)
     : message_loop_(NULL),
       rsa_key_(rsa_key->Copy()),
       usb_device_(usb_device),
       serial_(serial),
       inbound_address_(inbound_address),
       outbound_address_(outbound_address),
       zero_mask_(zero_mask),
+      interface_id_(interface_id),
       is_connected_(false),
       signature_sent_(false),
       last_socket_id_(256),
-      terminated_(false) {
+      terminated_(false),
+      weak_factory_(this) {
 }
 
 void AndroidUsbDevice::InitOnCallerThread() {
   if (message_loop_)
     return;
   message_loop_ = base::MessageLoop::current();
   Queue(new AdbMessage(AdbMessage::kCommandCNXN, kVersion, kMaxPayload,
                        kHostConnectMessage));
-  ReadHeader(true);
+  ReadHeader();
 }
 
 net::StreamSocket* AndroidUsbDevice::CreateSocket(const std::string& command) {
+  if (terminated_)
+    return NULL;
+
   uint32 socket_id = ++last_socket_id_;
   sockets_[socket_id] = new AndroidUsbSocket(this, socket_id, command,
       base::Bind(&AndroidUsbDevice::SocketDeleted, this));
   return sockets_[socket_id];
 }
 
 void AndroidUsbDevice::Send(uint32 command,
                             uint32 arg0,
                             uint32 arg1,
                             const std::string& body) {
   scoped_refptr<AdbMessage> m = new AdbMessage(command, arg0, arg1, body);
   // Delay open request if not yet connected.
   if (!is_connected_) {
     pending_messages_.push_back(m);
     return;
   }
   Queue(m);
 }
 
 AndroidUsbDevice::~AndroidUsbDevice() {
   Terminate();
-  usb_device_->AddRef();
-  BrowserThread::ReleaseSoon(BrowserThread::FILE, FROM_HERE,
-                             usb_device_.get());
 }
 
 void AndroidUsbDevice::Queue(scoped_refptr<AdbMessage> message) {
   // Queue header.
   std::vector<uint32> header;
   header.push_back(message->command);
   header.push_back(message->arg0);
   header.push_back(message->arg1);
   bool append_zero = true;
   if (message->body.empty())
@@ skipping 29 matching lines @@
 
 void AndroidUsbDevice::ProcessOutgoing() {
   if (outgoing_queue_.empty() || terminated_)
     return;
 
   BulkMessage message = outgoing_queue_.front();
   outgoing_queue_.pop();
   DumpMessage(true, message.first->data(), message.second);
   usb_device_->BulkTransfer(USB_DIRECTION_OUTBOUND, outbound_address_,
       message.first, message.second, kUsbTimeout,
-      base::Bind(&AndroidUsbDevice::OutgoingMessageSent, this));
+      base::Bind(&AndroidUsbDevice::OutgoingMessageSent,
+                 weak_factory_.GetWeakPtr()));
 }
 
 void AndroidUsbDevice::OutgoingMessageSent(UsbTransferStatus status,
                                            scoped_refptr<net::IOBuffer> buffer,
                                            size_t result) {
   if (status != USB_TRANSFER_COMPLETED)
     return;
   message_loop_->PostTask(FROM_HERE,
-                          base::Bind(&AndroidUsbDevice::ProcessOutgoing,
-                                     this));
+                          base::Bind(&AndroidUsbDevice::ProcessOutgoing, this));
 }
 
-void AndroidUsbDevice::ReadHeader(bool initial) {
+void AndroidUsbDevice::ReadHeader() {
   if (terminated_)
     return;
-  if (!initial && HasOneRef())
-    return;  // Stop polling.
   scoped_refptr<net::IOBuffer> buffer = new net::IOBuffer(kHeaderSize);
   usb_device_->BulkTransfer(USB_DIRECTION_INBOUND, inbound_address_,
       buffer, kHeaderSize, kUsbTimeout,
-      base::Bind(&AndroidUsbDevice::ParseHeader, this));
+      base::Bind(&AndroidUsbDevice::ParseHeader,
+                 weak_factory_.GetWeakPtr()));
 }
 
 void AndroidUsbDevice::ParseHeader(UsbTransferStatus status,
                                    scoped_refptr<net::IOBuffer> buffer,
                                    size_t result) {
   if (status == USB_TRANSFER_TIMEOUT) {
     message_loop_->PostTask(FROM_HERE,
-                            base::Bind(&AndroidUsbDevice::ReadHeader, this,
-                                       false));
+                            base::Bind(&AndroidUsbDevice::ReadHeader, this));
     return;
   }
 
   if (status != USB_TRANSFER_COMPLETED || result != kHeaderSize) {
     TransferError(status);
     return;
   }
 
   DumpMessage(false, buffer->data(), result);
   std::vector<uint32> header(6);
@@ skipping 16 matching lines @@
   }
 
   message_loop_->PostTask(FROM_HERE,
                           base::Bind(&AndroidUsbDevice::ReadBody, this,
                                      message, data_length, data_check));
 }
 
 void AndroidUsbDevice::ReadBody(scoped_refptr<AdbMessage> message,
                                 uint32 data_length,
                                 uint32 data_check) {
+  if (terminated_)
+    return;
   scoped_refptr<net::IOBuffer> buffer = new net::IOBuffer(data_length);
   usb_device_->BulkTransfer(USB_DIRECTION_INBOUND, inbound_address_,
       buffer, data_length, kUsbTimeout,
-      base::Bind(&AndroidUsbDevice::ParseBody, this, message, data_length,
-                 data_check));
+      base::Bind(&AndroidUsbDevice::ParseBody, weak_factory_.GetWeakPtr(),
+                 message, data_length, data_check));
 }
 
 void AndroidUsbDevice::ParseBody(scoped_refptr<AdbMessage> message,
                                  uint32 data_length,
                                  uint32 data_check,
                                  UsbTransferStatus status,
                                  scoped_refptr<net::IOBuffer> buffer,
                                  size_t result) {
   if (status == USB_TRANSFER_TIMEOUT) {
     message_loop_->PostTask(FROM_HERE,
@@ skipping 60 matching lines @@
     case AdbMessage::kCommandCLSE:
       {
         AndroidUsbSockets::iterator it = sockets_.find(message->arg1);
         if (it != sockets_.end())
           it->second->HandleIncoming(message);
       }
       break;
     default:
       break;
   }
-  ReadHeader(false);
+  ReadHeader();
 }
 
 void AndroidUsbDevice::TransferError(UsbTransferStatus status) {
   message_loop_->PostTask(FROM_HERE,
-                          base::Bind(&AndroidUsbDevice::Terminate,
-                                     this));
+                          base::Bind(&AndroidUsbDevice::Terminate, this));
+}
+
+void AndroidUsbDevice::TerminateIfReleased() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  if (usb_device_->device())

[Vladislav Kaznacheev, 2014/04/10 14:38:11]

usb_device_ pointer can be set to NULL by a different thread. This frightens me.

How about we bind a copy as a parameter? 

[pfeldman, 2014/04/10 15:37:46]

Done.

+    return;
+  message_loop_->PostTask(FROM_HERE,
+                          base::Bind(&AndroidUsbDevice::Terminate, this));
 }
 
 void AndroidUsbDevice::Terminate() {
+  std::vector<AndroidUsbDevice*>::iterator it =
+      std::find(g_devices.Get().begin(), g_devices.Get().end(), this);
+  if (it != g_devices.Get().end())
+    g_devices.Get().erase(it);
+
   if (terminated_)
     return;
 
   terminated_ = true;

[Vladislav Kaznacheev, 2014/04/10 14:38:11]

terminated_ is always in sync with !usb_device_. Do we need both?

[pfeldman, 2014/04/10 15:37:46]

Done.

 
   // Iterate over copy.
   AndroidUsbSockets sockets(sockets_);
   for (AndroidUsbSockets::iterator it = sockets.begin();
        it != sockets.end(); ++it) {
     it->second->Terminated();
   }
+  DCHECK(sockets_.empty());
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&ReleaseInterface, usb_device_));
+      base::Bind(&ReleaseInterface, usb_device_, interface_id_));
+  usb_device_ = NULL;
 }
 
 void AndroidUsbDevice::SocketDeleted(uint32 socket_id) {
   sockets_.erase(socket_id);
 }