DevTools: Removed refcounting from AndroidWebSocket

DevTools: Removed refcounting from AndroidWebSocket

Issue 387067 can be resolved by having port forwarding socket dependent on all other references to AndroidDeviceManager::Device.

This requires for lifetime of AWS to be manageable in the first place.

BUG=387067
Committed: https://crrev.com/346b47f09a946c42fbcbdab789cee14682738169
Cr-Commit-Position: refs/heads/master@{#296927}

[vkuzkokov, 2014-08-08 12:29:10]

ptal

[dgozman, 2014-08-08 12:32:09]

Please explain motivation behind this in the description. Having a BUG with
explanation would be even better.

[vkuzkokov, 2014-08-08 12:58:53]

On 2014/08/08 12:32:09, dgozman wrote:
> Please explain motivation behind this in the description. Having a BUG with
> explanation would be even better.

Added description.

[dgozman, 2014-08-08 14:17:28]

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_device_manager.h (left):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_device_manager.h:81:
DISALLOW_COPY_AND_ASSIGN(AndroidWebSocket);
Don't remove this.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (left):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:33: virtual void
ClearDelegate() OVERRIDE;
Why is this method removed?

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:36: class Connection {
Please, add a comment.

This is a WebSocketImpl counterpart which is constructed on UI thread and after
that leaves entirely on handler thread. It uses weak pointer to report back to
web socket on UI thread.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:50: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:64: scoped_ptr<Connection>
connection_;
You should use Connection* here, since you manage it's lifetime explicitly.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:67: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:119:
scoped_ptr<net::StreamSocket> socket(socket_raw);
Please add a comment:

This method needs a separate static implementation, because it should destroy
socket_raw if the owning web socket is already destroyed.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:120: if (WebSocketImpl*
web_socket = weak_web_socket.get())
if (weak_web_socket)
  weak_web_socket->Connected(result, socket.release());

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:209:
scoped_ptr<net::StreamSocket> socket(socket_.release());
This means you need all the |if (!socket_)| checks.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/port_forwarding_controller.cc (left):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/port_forwarding_controller.cc:336: registry_ =
NULL;
All the |if (registry_)| checks should be removed, since regsitry_ is never
NULL.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/port_forwarding_controller.cc (right):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/port_forwarding_controller.cc:435: void
PortForwardingController::Connection::UpdateSocketCountOnHandlerThread(
// static

[vkuzkokov, 2014-08-08 15:59:44]

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_device_manager.h (left):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_device_manager.h:81:
DISALLOW_COPY_AND_ASSIGN(AndroidWebSocket);
On 2014/08/08 14:17:28, dgozman wrote:
> Don't remove this.

Class with pure virtual methods can't be copied or assigned anyway.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (left):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:33: virtual void
ClearDelegate() OVERRIDE;
On 2014/08/08 14:17:28, dgozman wrote:
> Why is this method removed?

It was used to signal AndroidWebSocket not to call delegate's methods anymore.
Now AWS just deleted instead.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:36: class Connection {
On 2014/08/08 14:17:28, dgozman wrote:
> Please, add a comment.
> 
> This is a WebSocketImpl counterpart which is constructed on UI thread and
after
> that leaves entirely on handler thread. It uses weak pointer to report back to
> web socket on UI thread.

Done.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:50: };
On 2014/08/08 14:17:28, dgozman wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:64: scoped_ptr<Connection>
connection_;
On 2014/08/08 14:17:28, dgozman wrote:
> You should use Connection* here, since you manage it's lifetime explicitly.

Done.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:67: };
On 2014/08/08 14:17:28, dgozman wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:119:
scoped_ptr<net::StreamSocket> socket(socket_raw);
On 2014/08/08 14:17:28, dgozman wrote:
> Please add a comment:
> 
> This method needs a separate static implementation, because it should destroy
> socket_raw if the owning web socket is already destroyed.

Done.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:120: if (WebSocketImpl*
web_socket = weak_web_socket.get())
On 2014/08/08 14:17:28, dgozman wrote:
> if (weak_web_socket)
>   weak_web_socket->Connected(result, socket.release());

Done.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:209:
scoped_ptr<net::StreamSocket> socket(socket_.release());
On 2014/08/08 14:17:28, dgozman wrote:
> This means you need all the |if (!socket_)| checks.

Added checks to public methods (StartListening and SendFrame). Private methods
are then guaranteed to be called only while socket is alive.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/port_forwarding_controller.cc (left):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/port_forwarding_controller.cc:336: registry_ =
NULL;
On 2014/08/08 14:17:28, dgozman wrote:
> All the |if (registry_)| checks should be removed, since regsitry_ is never
> NULL.

Done.

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/device/port_forwarding_controller.cc (right):

https://codereview.chromium.org/449883002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/device/port_forwarding_controller.cc:435: void
PortForwardingController::Connection::UpdateSocketCountOnHandlerThread(
On 2014/08/08 14:17:28, dgozman wrote:
> // static

Done.

[dgozman, 2014-08-08 16:51:43]

lgtm, but please wait for Pavel

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:147: if (!socket_)
I think, this is impossible. Let's DCHECK(socket_).

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:217: socket_.reset(NULL);
socket_.reset();

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:354:
web_socket_.reset(NULL);
reset()

[vkuzkokov, 2014-08-11 10:18:23]

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:147: if (!socket_)
On 2014/08/08 16:51:43, dgozman wrote:
> I think, this is impossible. Let's DCHECK(socket_).

Done.

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:217: socket_.reset(NULL);
On 2014/08/08 16:51:43, dgozman wrote:
> socket_.reset();

Done.

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/449883002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:354:
web_socket_.reset(NULL);
On 2014/08/08 16:51:43, dgozman wrote:
> reset()

Done.

[pfeldman, 2014-08-11 11:16:33]

I don't see how this makes code simpler.

[pfeldman, 2014-08-11 11:41:38]

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:39: class Connection {
Lets inherit from NonThreadSafe?

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:44: void SendFrame(const
std::string& message);
Blink line below.

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:125: // The sole purpose of
a static method here is to avoid leaking StreamSocket
Doesn't base::Owned solve this for you?

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:133: void
WebSocketImpl::Connected(int result, net::StreamSocket* socket_raw) {
Please DCHECK the threads in all methods (here and below)

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:218:
base::Bind(&WebSocketImpl::OnSocketClosed, weak_web_socket_));
weak_web_socket_ could be pointing to the garbage in memory.

[vkuzkokov, 2014-08-11 14:40:03]

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:39: class Connection {
On 2014/08/11 11:41:38, pfeldman wrote:
> Lets inherit from NonThreadSafe?

NonThreadSafe is not recommended in this case. Added ThreadChecker instead.

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:44: void SendFrame(const
std::string& message);
On 2014/08/11 11:41:38, pfeldman wrote:
> Blink line below.

Done.

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:125: // The sole purpose of
a static method here is to avoid leaking StreamSocket
On 2014/08/11 11:41:38, pfeldman wrote:
> Doesn't base::Owned solve this for you?

It doesn't: we don't want StreamSocket destroyed after we exit this method.

base::Passed does help, however. It must be added on the other side
(AndroidDeviceManager::SocketCallback) to work, though.

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:133: void
WebSocketImpl::Connected(int result, net::StreamSocket* socket_raw) {
On 2014/08/11 11:41:38, pfeldman wrote:
> Please DCHECK the threads in all methods (here and below)

Done.

https://codereview.chromium.org/449883002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:218:
base::Bind(&WebSocketImpl::OnSocketClosed, weak_web_socket_));
On 2014/08/11 11:41:38, pfeldman wrote:
> weak_web_socket_ could be pointing to the garbage in memory.

That's why we use weak pointer.

[vkuzkokov, 2014-08-12 08:53:25]

Renamed classes and made them more separate.

[pfeldman, 2014-08-13 09:09:35]

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:22: // Counterpart of
AndroidWebSocketImpl existing on handler thread.
There is no mention of AndroidWebSocketImpl in this class anymore. It is just a
web socket wrapper around the socket that should work on the caller thread.

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:138:
scoped_ptr<net::StreamSocket> socket) {
Poor indent

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:186:
delegate_message_loop_->PostTask(FROM_HERE,
Threading should only be handled in the AndroidWebSocketImpl

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:229:
delegate_message_loop_->PostTask(FROM_HERE,
ditto

[vkuzkokov, 2014-08-13 10:00:06]

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:22: // Counterpart of
AndroidWebSocketImpl existing on handler thread.
On 2014/08/13 09:09:35, pfeldman wrote:
> There is no mention of AndroidWebSocketImpl in this class anymore. It is just
a
> web socket wrapper around the socket that should work on the caller thread.

Done.

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:138:
scoped_ptr<net::StreamSocket> socket) {
On 2014/08/13 09:09:35, pfeldman wrote:
> Poor indent

Done.

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:186:
delegate_message_loop_->PostTask(FROM_HERE,
On 2014/08/13 09:09:35, pfeldman wrote:
> Threading should only be handled in the AndroidWebSocketImpl

Rather it shouldn't be handled in WebSocketImpl. We can't call methods of AWSI
on handler thread as we have no control over its lifetime there. This requires a
third object.

https://codereview.chromium.org/449883002/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:229:
delegate_message_loop_->PostTask(FROM_HERE,
On 2014/08/13 09:09:35, pfeldman wrote:
> ditto

Ditto.

[vkuzkokov, 2014-08-13 10:17:53]

Changed DelegateWrapper ownership

[pfeldman, 2014-08-13 11:23:48]

lgtm, thanks

[vkuzkokov, 2014-08-13 11:28:10]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-08-13 11:28:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/449883002/160001

[vkuzkokov, 2014-08-13 11:33:26]

The CQ bit was unchecked by vkuzkokov@chromium.org

[vkuzkokov, 2014-08-13 11:40:22]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-08-13 11:40:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/449883002/180001

[vkuzkokov, 2014-08-13 11:56:29]

The CQ bit was unchecked by vkuzkokov@chromium.org

[vkuzkokov, 2014-08-13 12:03:06]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-08-13 12:03:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/449883002/200001

[commit-bot: I haz the power, 2014-08-13 13:36:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vkuzkokov, 2014-08-13 13:48:26]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-08-13 13:49:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/449883002/220001

[dgozman, 2014-08-13 15:34:34]

https://codereview.chromium.org/449883002/diff/220001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:44: class DelegateWrapper
I still think that this class needs a comment about its purpose.

https://codereview.chromium.org/449883002/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:139: void
WebSocketImpl::SendFrame(const std::string& message) {
Could you please group methods by classes?

[commit-bot: I haz the power, 2014-08-13 15:42:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-13 15:46:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-13 15:46:03]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vkuzkokov, 2014-08-13 15:50:25]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-08-13 15:52:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/449883002/220001

[vkuzkokov, 2014-08-13 15:53:56]

https://codereview.chromium.org/449883002/diff/220001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/449883002/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:44: class DelegateWrapper
On 2014/08/13 15:34:33, dgozman wrote:
> I still think that this class needs a comment about its purpose.

It's better explained by implementation.

https://codereview.chromium.org/449883002/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:139: void
WebSocketImpl::SendFrame(const std::string& message) {
On 2014/08/13 15:34:33, dgozman wrote:
> Could you please group methods by classes?

In another CL - this way we have a clearer diff here.

[commit-bot: I haz the power, 2014-08-13 15:57:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-13 16:08:13]

Committed patchset #12 (220001) as 289306

[vkuzkokov, 2014-08-25 14:35:05]

A revert of this CL (patchset #12) has been created in
https://codereview.chromium.org/505783002/ by vkuzkokov@chromium.org.

The reason for reverting is: There is a crash when port forwarding is turned off
while there is at least one connected device.

BUG=405681.

[vkuzkokov, 2014-08-25 15:36:03]

On 2014/08/25 14:35:05, vkuzkokov wrote:
> A revert of this CL (patchset #12) has been created in
> https://codereview.chromium.org/505783002/ by mailto:vkuzkokov@chromium.org.
> 
> The reason for reverting is: There is a crash when port forwarding is turned
off
> while there is at least one connected device.
> 
> BUG=405681.

Added bug 407137 for second crash

[vkuzkokov, 2014-09-23 12:56:27]

Patchset #13 (id:240001) has been deleted

[vkuzkokov, 2014-09-23 12:59:36]

Patchset #13 (id:260001) has been deleted

[vkuzkokov, 2014-09-23 13:45:56]

Patchset #13 (id:280001) has been deleted

[vkuzkokov, 2014-09-23 13:46:07]

Patchset #13 (id:300001) has been deleted

[vkuzkokov, 2014-09-23 13:50:22]

https://codereview.chromium.org/449883002/diff/220001/chrome/browser/devtools...
File chrome/browser/devtools/device/port_forwarding_controller.cc (right):

https://codereview.chromium.org/449883002/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/device/port_forwarding_controller.cc:596:
STLDeleteValues(&registry_);
FYI, this is the place where crash happened:
delete called registry_->erase which invalidated iterator inside
STLDeleteValues.

[vkuzkokov, 2014-09-23 13:51:21]

Reopened. PTAL

[dgozman, 2014-09-26 09:58:40]

https://codereview.chromium.org/449883002/diff/320001/chrome/browser/devtools...
File chrome/browser/devtools/device/port_forwarding_controller.cc (right):

https://codereview.chromium.org/449883002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/port_forwarding_controller.cc:579: while
(!registry_.empty()) {
Let's make a copy vector of registry, and delete each element from there.

[dgozman, 2014-09-26 10:57:20]

lgtm

Let's separate WebSocketImpl from AndroidWebSocket in a follow-up patch, put it
in web_socket.h/cc files with its own delegate.

[vkuzkokov, 2014-09-26 11:36:13]

The CQ bit was checked by vkuzkokov@chromium.org

[vkuzkokov, 2014-09-26 11:36:49]

The CQ bit was unchecked by vkuzkokov@chromium.org

[vkuzkokov, 2014-09-26 11:37:07]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-09-26 11:37:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/449883002/340001

[commit-bot: I haz the power, 2014-09-26 12:57:21]

Committed patchset #14 (id:340001) as ff7b0104d55c2d954378ef002464239869942817

[commit-bot: I haz the power, 2014-09-26 12:58:04]

Patchset 14 (id:??) landed as
https://crrev.com/346b47f09a946c42fbcbdab789cee14682738169
Cr-Commit-Position: refs/heads/master@{#296927}