From 851cfc61ae374c3ff134a00d279c583078c32fee Mon Sep 17 00:00:00 2001 From: vkuzkokov Date: Wed, 22 Oct 2014 05:16:01 -0700 Subject: [PATCH] [DevTools] Port forwarding doesn't keep USB connection That means that when chrome://inspect is closed device is released. BUG=387067 Review URL: https://codereview.chromium.org/644963003 Cr-Commit-Position: refs/heads/master@{#300667} --- .../devtools/device/android_device_manager.cc | 16 ++++--- .../devtools/device/android_device_manager.h | 6 ++- .../browser/devtools/device/android_web_socket.cc | 30 +++++++++--- .../devtools/device/devtools_android_bridge.cc | 42 +++++++++-------- .../devtools/device/devtools_android_bridge.h | 20 ++++---- .../devtools/device/port_forwarding_controller.cc | 54 +++++++++++----------- .../devtools/device/port_forwarding_controller.h | 6 ++- 7 files changed, 100 insertions(+), 74 deletions(-) diff --git a/chrome/browser/devtools/device/android_device_manager.cc b/chrome/browser/devtools/device/android_device_manager.cc index df99de7ba913..458bb9dff086 100644 --- a/chrome/browser/devtools/device/android_device_manager.cc +++ b/chrome/browser/devtools/device/android_device_manager.cc @@ -326,7 +326,7 @@ AndroidDeviceManager::DeviceProvider::~DeviceProvider() { void AndroidDeviceManager::Device::QueryDeviceInfo( const DeviceInfoCallback& callback) { - device_message_loop_->PostTask( + message_loop_proxy_->PostTask( FROM_HERE, base::Bind(&DeviceProvider::QueryDeviceInfo, provider_, @@ -338,7 +338,7 @@ void AndroidDeviceManager::Device::QueryDeviceInfo( void AndroidDeviceManager::Device::OpenSocket(const std::string& socket_name, const SocketCallback& callback) { - device_message_loop_->PostTask( + message_loop_proxy_->PostTask( FROM_HERE, base::Bind(&DeviceProvider::OpenSocket, provider_, @@ -351,7 +351,7 @@ void AndroidDeviceManager::Device::SendJsonRequest( const std::string& socket_name, const std::string& request, const CommandCallback& callback) { - device_message_loop_->PostTask( + message_loop_proxy_->PostTask( FROM_HERE, base::Bind(&DeviceProvider::SendJsonRequest, provider_, @@ -366,7 +366,7 @@ void AndroidDeviceManager::Device::SendJsonRequest( void AndroidDeviceManager::Device::HttpUpgrade(const std::string& socket_name, const std::string& url, const SocketCallback& callback) { - device_message_loop_->PostTask( + message_loop_proxy_->PostTask( FROM_HERE, base::Bind(&DeviceProvider::HttpUpgrade, provider_, @@ -382,17 +382,21 @@ AndroidDeviceManager::Device::Device( scoped_refptr device_message_loop, scoped_refptr provider, const std::string& serial) - : device_message_loop_(device_message_loop), + : message_loop_proxy_(device_message_loop), provider_(provider), serial_(serial), weak_factory_(this) { } AndroidDeviceManager::Device::~Device() { + std::set sockets_copy(sockets_); + for (AndroidWebSocket* socket : sockets_copy) + socket->OnSocketClosed(); + provider_->AddRef(); DeviceProvider* raw_ptr = provider_.get(); provider_ = NULL; - device_message_loop_->PostTask( + message_loop_proxy_->PostTask( FROM_HERE, base::Bind(&ReleaseDeviceAndProvider, base::Unretained(raw_ptr), diff --git a/chrome/browser/devtools/device/android_device_manager.h b/chrome/browser/devtools/device/android_device_manager.h index 835f1606ed83..ec65e03095bb 100644 --- a/chrome/browser/devtools/device/android_device_manager.h +++ b/chrome/browser/devtools/device/android_device_manager.h @@ -83,8 +83,9 @@ class AndroidDeviceManager void Connected(int result, scoped_ptr socket); void OnFrameRead(const std::string& message); void OnSocketClosed(); + void Terminate(); - scoped_refptr device_; + Device* device_; WebSocketImpl* socket_impl_; Delegate* delegate_; base::WeakPtrFactory weak_factory_; @@ -126,9 +127,10 @@ class AndroidDeviceManager virtual ~Device(); - scoped_refptr device_message_loop_; + scoped_refptr message_loop_proxy_; scoped_refptr provider_; std::string serial_; + std::set sockets_; base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(Device); diff --git a/chrome/browser/devtools/device/android_web_socket.cc b/chrome/browser/devtools/device/android_web_socket.cc index 35ddf13ec5d3..7f9f5b7b8068 100644 --- a/chrome/browser/devtools/device/android_web_socket.cc +++ b/chrome/browser/devtools/device/android_web_socket.cc @@ -132,12 +132,14 @@ AndroidDeviceManager::AndroidWebSocket::AndroidWebSocket( const std::string& socket_name, const std::string& url, Delegate* delegate) - : device_(device), + : device_(device.get()), socket_impl_(nullptr), delegate_(delegate), weak_factory_(this) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(delegate_); + DCHECK(device_); + device_->sockets_.insert(this); device_->HttpUpgrade( socket_name, url, base::Bind(&AndroidWebSocket::Connected, weak_factory_.GetWeakPtr())); @@ -145,15 +147,15 @@ AndroidDeviceManager::AndroidWebSocket::AndroidWebSocket( AndroidDeviceManager::AndroidWebSocket::~AndroidWebSocket() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (socket_impl_) - device_->device_message_loop_->DeleteSoon(FROM_HERE, socket_impl_); + Terminate(); } void AndroidDeviceManager::AndroidWebSocket::SendFrame( const std::string& message) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(socket_impl_); - device_->device_message_loop_->PostTask( + DCHECK(device_); + device_->message_loop_proxy_->PostTask( FROM_HERE, base::Bind(&WebSocketImpl::SendFrame, base::Unretained(socket_impl_), message)); @@ -170,7 +172,7 @@ void AndroidDeviceManager::AndroidWebSocket::Connected( socket_impl_ = new WebSocketImpl(base::MessageLoopProxy::current(), weak_factory_.GetWeakPtr(), socket.Pass()); - device_->device_message_loop_->PostTask( + device_->message_loop_proxy_->PostTask( FROM_HERE, base::Bind(&WebSocketImpl::StartListening, base::Unretained(socket_impl_))); @@ -185,13 +187,27 @@ void AndroidDeviceManager::AndroidWebSocket::OnFrameRead( void AndroidDeviceManager::AndroidWebSocket::OnSocketClosed() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + Terminate(); delegate_->OnSocketClosed(); } +void AndroidDeviceManager::AndroidWebSocket::Terminate() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + if (socket_impl_) { + DCHECK(device_); + device_->message_loop_proxy_->DeleteSoon(FROM_HERE, socket_impl_); + socket_impl_ = nullptr; + } + if (device_) { + device_->sockets_.erase(this); + device_ = nullptr; + } +} + AndroidDeviceManager::AndroidWebSocket* AndroidDeviceManager::Device::CreateWebSocket( - const std::string& socket, + const std::string& socket_name, const std::string& url, AndroidWebSocket::Delegate* delegate) { - return new AndroidWebSocket(this, socket, url, delegate); + return new AndroidWebSocket(this, socket_name, url, delegate); } diff --git a/chrome/browser/devtools/device/devtools_android_bridge.cc b/chrome/browser/devtools/device/devtools_android_bridge.cc index 3b05ecb05fe2..747598140176 100644 --- a/chrome/browser/devtools/device/devtools_android_bridge.cc +++ b/chrome/browser/devtools/device/devtools_android_bridge.cc @@ -308,6 +308,7 @@ class DevToolsAndroidBridge::AgentHostDelegate bool socket_opened_; bool is_web_view_; std::vector pending_messages_; + scoped_refptr device_; scoped_ptr web_socket_; content::DevToolsAgentHost* agent_host_; content::DevToolsExternalAgentProxy* proxy_; @@ -361,12 +362,19 @@ void DevToolsAndroidBridge::AgentHostDelegate::Attach( proxy_ = proxy; content::RecordAction(base::UserMetricsAction(is_web_view_ ? "DevTools_InspectAndroidWebView" : "DevTools_InspectAndroidPage")); + + // Retain the device so it's not released until AgentHost is detached. + device_ = bridge_->FindDevice(browser_id_.first); + if (!device_.get()) + return; + web_socket_.reset( - bridge_->CreateWebSocket(browser_id_, debug_url_, this)); + device_->CreateWebSocket(browser_id_.second, debug_url_, this)); } void DevToolsAndroidBridge::AgentHostDelegate::Detach() { web_socket_.reset(); + device_ = nullptr; } void DevToolsAndroidBridge::AgentHostDelegate::SendMessageToBackend( @@ -594,12 +602,13 @@ void DevToolsAndroidBridge::SendJsonRequest( const BrowserId& browser_id, const std::string& request, const JsonRequestCallback& callback) { - DeviceMap::iterator it = device_map_.find(browser_id.first); - if (it == device_map_.end()) { + scoped_refptr device( + FindDevice(browser_id.first)); + if (!device.get()) { callback.Run(net::ERR_FAILED, std::string()); return; } - it->second->SendJsonRequest(browser_id.second, request, callback); + device->SendJsonRequest(browser_id.second, request, callback); } void DevToolsAndroidBridge::SendProtocolCommand( @@ -611,13 +620,14 @@ void DevToolsAndroidBridge::SendProtocolCommand( DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); if (debug_url.empty()) return; - DeviceMap::iterator it = device_map_.find(browser_id.first); - if (it == device_map_.end()) { + scoped_refptr device( + FindDevice(browser_id.first)); + if (!device.get()) { callback.Run(); return; } DevToolsProtocol::Command command(1, method, params); - new ProtocolCommand(it->second, browser_id.second, debug_url, + new ProtocolCommand(device, browser_id.second, debug_url, command.Serialize(), callback); } @@ -632,16 +642,10 @@ DevToolsAndroidBridge::GetBrowserAgentHost( browser->IsWebView()); } -AndroidDeviceManager::AndroidWebSocket* -DevToolsAndroidBridge::CreateWebSocket( - const BrowserId& browser_id, - const std::string& url, - AndroidDeviceManager::AndroidWebSocket::Delegate* delegate) { - DeviceMap::iterator it = device_map_.find(browser_id.first); - if (it == device_map_.end()) - return NULL; - - return it->second->CreateWebSocket(browser_id.second, url, delegate); +scoped_refptr DevToolsAndroidBridge::FindDevice( + const std::string& serial) { + DeviceMap::iterator it = device_map_.find(serial); + return it == device_map_.end() ? nullptr : it->second; } void DevToolsAndroidBridge::RespondToOpenOnUIThread( @@ -760,7 +764,7 @@ DevToolsAndroidBridge::DevToolsAndroidBridge(Profile* profile) : profile_(profile), device_manager_(AndroidDeviceManager::Create()), task_scheduler_(base::Bind(&DevToolsAndroidBridge::ScheduleTaskDefault)), - port_forwarding_controller_(new PortForwardingController(profile)) { + port_forwarding_controller_(new PortForwardingController(profile, this)) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); pref_change_registrar_.Init(profile_->GetPrefs()); pref_change_registrar_.Add(prefs::kDevToolsDiscoverUsbDevicesEnabled, @@ -881,7 +885,7 @@ void DevToolsAndroidBridge::ReceivedDeviceList( (*it)->DeviceListChanged(remote_devices); ForwardingStatus status = - port_forwarding_controller_->DeviceListChanged(complete_devices); + port_forwarding_controller_->DeviceListChanged(remote_devices); PortForwardingListeners forwarding_listeners(port_forwarding_listeners_); for (PortForwardingListeners::iterator it = forwarding_listeners.begin(); it != forwarding_listeners.end(); ++it) { diff --git a/chrome/browser/devtools/device/devtools_android_bridge.h b/chrome/browser/devtools/device/devtools_android_bridge.h index 2310600c56e3..a64dc1f5bbdd 100644 --- a/chrome/browser/devtools/device/devtools_android_bridge.h +++ b/chrome/browser/devtools/device/devtools_android_bridge.h @@ -226,17 +226,13 @@ class DevToolsAndroidBridge scoped_refptr GetBrowserAgentHost( scoped_refptr browser); - - typedef std::pair, - scoped_refptr> CompleteDevice; - typedef std::vector CompleteDevices; - typedef base::Callback DeviceListCallback; - private: friend struct content::BrowserThread::DeleteOnThread< content::BrowserThread::UI>; friend class base::DeleteHelper; + friend class PortForwardingController; + class AgentHostDelegate; class DiscoveryRequest; class RemotePageTarget; @@ -247,8 +243,12 @@ class DevToolsAndroidBridge void StopDeviceListPolling(); bool NeedsDeviceListPolling(); - void RequestDeviceList(const DeviceListCallback& callback); + typedef std::pair, + scoped_refptr> CompleteDevice; + typedef std::vector CompleteDevices; + typedef base::Callback DeviceListCallback; + void RequestDeviceList(const DeviceListCallback& callback); void ReceivedDeviceList(const CompleteDevices& complete_devices); void StartDeviceCountPolling(); @@ -270,10 +270,8 @@ class DevToolsAndroidBridge base::DictionaryValue* params, const base::Closure callback); - AndroidDeviceManager::AndroidWebSocket* CreateWebSocket( - const BrowserId& browser_id, - const std::string& url, - AndroidDeviceManager::AndroidWebSocket::Delegate* delegate); + scoped_refptr FindDevice( + const std::string& serial); void PageCreatedOnUIThread(scoped_refptr browser, const RemotePageCallback& callback, diff --git a/chrome/browser/devtools/device/port_forwarding_controller.cc b/chrome/browser/devtools/device/port_forwarding_controller.cc index d19e9fffb36d..eb82daa84674 100644 --- a/chrome/browser/devtools/device/port_forwarding_controller.cc +++ b/chrome/browser/devtools/device/port_forwarding_controller.cc @@ -254,8 +254,7 @@ FindBestBrowserForTethering( class PortForwardingController::Connection : public AndroidDeviceManager::AndroidWebSocket::Delegate { public: - Connection(Registry* registry, - scoped_refptr device, + Connection(PortForwardingController* controller, scoped_refptr browser, const ForwardingMap& forwarding_map); ~Connection() override; @@ -273,9 +272,7 @@ class PortForwardingController::Connection content::BrowserThread::UI>; friend class base::DeleteHelper; - typedef std::map ForwardingMap; - typedef base::Callback CommandCallback; typedef std::map CommandCallbackMap; @@ -298,8 +295,7 @@ class PortForwardingController::Connection void OnFrameRead(const std::string& message) override; void OnSocketClosed() override; - PortForwardingController::Registry* registry_; - scoped_refptr device_; + PortForwardingController* controller_; scoped_refptr browser_; scoped_ptr web_socket_; int command_id_; @@ -313,29 +309,30 @@ class PortForwardingController::Connection }; PortForwardingController::Connection::Connection( - Registry* registry, - scoped_refptr device, + PortForwardingController* controller, scoped_refptr browser, const ForwardingMap& forwarding_map) - : registry_(registry), - device_(device), + : controller_(controller), browser_(browser), command_id_(0), connected_(false), forwarding_map_(forwarding_map), weak_factory_(this) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - (*registry_)[device_->serial()] = this; + controller_->registry_[browser->serial()] = this; + scoped_refptr device( + controller_->bridge_->FindDevice(browser->serial())); + DCHECK(device.get()); web_socket_.reset( - device_->CreateWebSocket(browser->socket(), - kDevToolsRemoteBrowserTarget, this)); + device->CreateWebSocket(browser->socket(), + kDevToolsRemoteBrowserTarget, this)); } PortForwardingController::Connection::~Connection() { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(registry_->find(device_->serial()) != registry_->end()); - registry_->erase(device_->serial()); + DCHECK(controller_->registry_.find(browser_->serial()) != + controller_->registry_.end()); + controller_->registry_.erase(browser_->serial()); } void PortForwardingController::Connection::UpdateForwardingMap( @@ -515,7 +512,10 @@ void PortForwardingController::Connection::OnFrameRead( base::Bind(&Connection::UpdateSocketCountOnHandlerThread, weak_factory_.GetWeakPtr(), port); - device_->OpenSocket( + scoped_refptr device( + controller_->bridge_->FindDevice(browser_->serial())); + DCHECK(device.get()); + device->OpenSocket( connection_id.c_str(), base::Bind(&SocketTunnel::StartTunnel, destination_host, @@ -523,8 +523,11 @@ void PortForwardingController::Connection::OnFrameRead( callback)); } -PortForwardingController::PortForwardingController(Profile* profile) +PortForwardingController::PortForwardingController( + Profile* profile, + DevToolsAndroidBridge* bridge) : profile_(profile), + bridge_(bridge), pref_service_(profile->GetPrefs()) { pref_change_registrar_.Init(pref_service_); base::Closure callback = base::Bind( @@ -538,23 +541,20 @@ PortForwardingController::~PortForwardingController() {} PortForwardingController::ForwardingStatus PortForwardingController::DeviceListChanged( - const DevToolsAndroidBridge::CompleteDevices& complete_devices) { + const DevToolsAndroidBridge::RemoteDevices& devices) { ForwardingStatus status; if (forwarding_map_.empty()) return status; - for (const auto& pair : complete_devices) { - scoped_refptr device(pair.first); - scoped_refptr remote_device( - pair.second); - if (!remote_device->is_connected()) + for (const auto& device : devices) { + if (!device->is_connected()) continue; - Registry::iterator rit = registry_.find(remote_device->serial()); + Registry::iterator rit = registry_.find(device->serial()); if (rit == registry_.end()) { scoped_refptr browser( - FindBestBrowserForTethering(remote_device->browsers())); + FindBestBrowserForTethering(device->browsers())); if (browser.get()) - new Connection(®istry_, device, browser, forwarding_map_); + new Connection(this, browser, forwarding_map_); } else { status.push_back(std::make_pair(rit->second->browser(), rit->second->GetPortStatusMap())); diff --git a/chrome/browser/devtools/device/port_forwarding_controller.h b/chrome/browser/devtools/device/port_forwarding_controller.h index ab75a48218a8..19736fcdd101 100644 --- a/chrome/browser/devtools/device/port_forwarding_controller.h +++ b/chrome/browser/devtools/device/port_forwarding_controller.h @@ -22,12 +22,13 @@ class PortForwardingController { typedef DevToolsAndroidBridge::BrowserStatus BrowserStatus; typedef DevToolsAndroidBridge::ForwardingStatus ForwardingStatus; - explicit PortForwardingController(Profile* profile); + explicit PortForwardingController(Profile* profile, + DevToolsAndroidBridge* bridge); virtual ~PortForwardingController(); ForwardingStatus DeviceListChanged( - const DevToolsAndroidBridge::CompleteDevices& complete_devices); + const DevToolsAndroidBridge::RemoteDevices& devices); private: class Connection; @@ -38,6 +39,7 @@ class PortForwardingController { void UpdateConnections(); Profile* profile_; + DevToolsAndroidBridge* bridge_; PrefService* pref_service_; PrefChangeRegistrar pref_change_registrar_; Registry registry_; -- 2.11.4.GIT