DevTools: do not retain android_usb_devices in their manager.

chrome/browser/devtools/adb/android_usb_socket.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_socket.h"
 
 #include "base/message_loop/message_loop.h"
 
 namespace {
 
@@ skipping 16 matching lines @@
 AndroidUsbSocket::AndroidUsbSocket(scoped_refptr<AndroidUsbDevice> device,
                                    uint32 socket_id,
                                    const std::string& command,
                                    base::Callback<void(uint32)> delete_callback)
     : device_(device),
       command_(command),
       delete_callback_(delete_callback),
       local_id_(socket_id),
       remote_id_(0),
       is_connected_(false),
       is_closed_(false) {

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

is_closed_ seems to be redundant now and can be replaced with !device_. In fact
it is only used at one point, see my comment at line 118.

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

Done.

 }
 
 AndroidUsbSocket::~AndroidUsbSocket() {
   DCHECK(CalledOnValidThread());
   if (is_connected_)
     Disconnect();
-  delete_callback_.Run(local_id_);
+  if (!delete_callback_.is_null())
+    delete_callback_.Run(local_id_);
 }
 
 void AndroidUsbSocket::HandleIncoming(scoped_refptr<AdbMessage> message) {
+  if (!device_)
+    return;
+
   CHECK_EQ(message->arg1, local_id_);
   switch (message->command) {
     case AdbMessage::kCommandOKAY:
       if (!is_connected_) {
         remote_id_ = message->arg0;
         is_connected_ = true;
         net::CompletionCallback callback = connect_callback_;
         connect_callback_.Reset();
         callback.Run(net::OK);
         // "this" can be NULL.
@@ skipping 14 matching lines @@
         callback.Run(net::OK);
         // "this" can be NULL.
       } else {
         RespondToReaders(false);
         // "this" can be NULL.
       }
       break;
     case AdbMessage::kCommandCLSE:
       if (is_connected_)
         device_->Send(AdbMessage::kCommandCLSE, local_id_, 0, "");
-      is_connected_ = false;
-      is_closed_ = true;
-      RespondToReaders(true);
+      Terminated();
       // "this" can be NULL.
       break;
     default:
       break;
   }
 }
 
 void AndroidUsbSocket::Terminated() {
   is_connected_ = false;
   is_closed_ = true;
+
+  // Break the socket -> device connection, release the device.
+  delete_callback_.Run(local_id_);
+  delete_callback_.Reset();
+  device_ = NULL;
+
+  // Respond to pending callbacks.
   if (!connect_callback_.is_null()) {
     net::CompletionCallback callback = connect_callback_;
     connect_callback_.Reset();
     callback.Run(net::ERR_FAILED);
     // "this" can be NULL.
     return;
   }
   RespondToReaders(true);
 }
 
 int AndroidUsbSocket::Read(net::IOBuffer* buffer,
                            int length,
                            const net::CompletionCallback& callback) {
   if (!is_connected_)
     return is_closed_ ? 0 : net::ERR_SOCKET_NOT_CONNECTED;

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

Just out of curiosity, why not return net::ERR_SOCKET_NOT_CONNECTED in both
cases?

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

Done.

 
   if (read_buffer_.empty()) {
     read_requests_.push_back(IORequest(buffer, length, callback));
     return net::ERR_IO_PENDING;
   }
 
   size_t bytes_to_copy = static_cast<size_t>(length) > read_buffer_.length() ?
       read_buffer_.length() : static_cast<size_t>(length);
   memcpy(buffer->data(), read_buffer_.data(), bytes_to_copy);
   if (read_buffer_.length() > bytes_to_copy)
@@ skipping 22 matching lines @@
   return false;
 }
 
 bool AndroidUsbSocket::SetSendBufferSize(int32 size) {
   NOTIMPLEMENTED();
   return false;
 }
 
 int AndroidUsbSocket::Connect(const net::CompletionCallback& callback) {
   DCHECK(CalledOnValidThread());
-  if (device_->terminated())
+  if (!device_)
     return net::ERR_FAILED;
   connect_callback_ = callback;
   device_->Send(AdbMessage::kCommandOPEN, local_id_, 0, command_);
   return net::ERR_IO_PENDING;
 }
 
 void AndroidUsbSocket::Disconnect() {
-  is_connected_ = false;
+  if (!device_)
+    return;
   device_->Send(AdbMessage::kCommandCLSE, local_id_, remote_id_, "");
-  RespondToReaders(true);
+  Terminated();
 }
 
 bool AndroidUsbSocket::IsConnected() const {
   DCHECK(CalledOnValidThread());
   return is_connected_;
 }
 
 bool AndroidUsbSocket::IsConnectedAndIdle() const {
   NOTIMPLEMENTED();
   return false;
@@ skipping 42 matching lines @@
   return net::kProtoUnknown;
 }
 
 bool AndroidUsbSocket::GetSSLInfo(net::SSLInfo* ssl_info) {
   return false;
 }
 
 void AndroidUsbSocket::RespondToReaders(bool disconnect) {
   std::deque<IORequest> read_requests;
   read_requests.swap(read_requests_);
   while (!read_requests.empty() && (!read_buffer_.empty() || disconnect)) {

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

Theoretically you can replace |disconnect| with |is_closed_| or |!device_| and
remove the parameter altogether.

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

Lets keep it.

     IORequest read_request = read_requests.front();
     read_requests.pop_front();
     size_t bytes_to_copy =
         static_cast<size_t>(read_request.length) > read_buffer_.length() ?
             read_buffer_.length() : static_cast<size_t>(read_request.length);
     memcpy(read_request.buffer->data(), read_buffer_.data(), bytes_to_copy);
     if (read_buffer_.length() > bytes_to_copy)
       read_buffer_ = read_buffer_.substr(bytes_to_copy);
     else
       read_buffer_ = "";
     read_request.callback.Run(bytes_to_copy);
   }
 }
 
 void AndroidUsbSocket::RespondToWriters() {
   if (!write_requests_.empty()) {
     IORequest write_request = write_requests_.front();
     write_requests_.pop_front();
     write_request.callback.Run(write_request.length);
   }
 }