[WebSocket] Send a close frame when the renderer process is gone.

[WebSocket] Send a close frame when the renderer process is gone.

Renderer processes may die without clearing its |WebSocketBridge|s.
It is helpful to send a close frame with code = 1001 in such a case.

BUG=392534
R=ricea@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285557

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285884

[Adam Rice, 2014-07-14 05:41:50]

If you have time, could you add a browser test for this case? As far as I know,
we can't test it with a layout test.

It will be quite a bit of work, so if you don't have time, don't worry about it,
just leave a note on the bug that we need a test for this behaviour.

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.cc:7: #include
<hash_map>
I think the lint warning that tells you to include this is wrong. The
"base/hash_map.h" include file will pull in the correct platform-specific
include.

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.cc:186: // In order to
avoid changing a container while iterating, we copy hosts.
s/a container/the container/
s/copy hosts/copy the hosts/

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.cc:191: // Note that
some GoAway could fail. In that case hosts[i] will be
s/some GoAway/some calls to GoAway/

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/websocket_host.h (right):

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
content/browser/renderer_host/websocket_host.h:43: // The renderer process goes
away.
s/goes away/is going away/

[yhirano, 2014-07-14 09:27:20]

Added a test. PATL.

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.cc:7: #include
<hash_map>
On 2014/07/14 05:41:50, Adam Rice wrote:
> I think the lint warning that tells you to include this is wrong. The
> "base/hash_map.h" include file will pull in the correct platform-specific
> include.
Thanks, done

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.cc:186: // In order to
avoid changing a container while iterating, we copy hosts.
On 2014/07/14 05:41:49, Adam Rice wrote:
> s/a container/the container/
> s/copy hosts/copy the hosts/

Done.

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.cc:191: // Note that
some GoAway could fail. In that case hosts[i] will be
On 2014/07/14 05:41:49, Adam Rice wrote:
> s/some GoAway/some calls to GoAway/

Done.

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/websocket_host.h (right):

https://codereview.chromium.org/390773002/diff/20001/content/browser/renderer...
content/browser/renderer_host/websocket_host.h:43: // The renderer process goes
away.
On 2014/07/14 05:41:50, Adam Rice wrote:
> s/goes away/is going away/

Done.

[Adam Rice, 2014-07-14 10:17:20]

https://codereview.chromium.org/390773002/diff/40001/chrome/browser/net/webso...
File chrome/browser/net/websocket_browsertest.cc (right):

https://codereview.chromium.org/390773002/diff/40001/chrome/browser/net/webso...
chrome/browser/net/websocket_browsertest.cc:93: // Launch a secure WebSocket
server.
Remove the word "secure".

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/conne...
File net/data/websocket/connect.html (right):

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/conne...
net/data/websocket/connect.html:6: var href = window.location.href;
See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pywebs...
for a more elegant way to do this. (Optional).

[Adam Rice, 2014-07-14 10:31:31]

https://codereview.chromium.org/390773002/diff/40001/chrome/browser/net/webso...
File chrome/browser/net/websocket_browsertest.cc (right):

https://codereview.chromium.org/390773002/diff/40001/chrome/browser/net/webso...
chrome/browser/net/websocket_browsertest.cc:110: NavigateToHTTP("connect.html");
The name "connect.html" is too generic. How about "counted-connection.html" so
it is easy to associate with "count-connection.html" used by the same test.

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/count...
File net/data/websocket/count-connection_wsh.py (right):

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/count...
net/data/websocket/count-connection_wsh.py:1: # Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Update copyright message.

Also, you need to add a short description of these files to the README file in
the same directory.

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/count...
net/data/websocket/count-connection_wsh.py:5: import struct
Not used.

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/count...
net/data/websocket/count-connection_wsh.py:10: from mod_pywebsocket import
handshake
Neither mod_pywebsocket import is used.

[yhirano, 2014-07-14 11:24:36]

https://codereview.chromium.org/390773002/diff/40001/chrome/browser/net/webso...
File chrome/browser/net/websocket_browsertest.cc (right):

https://codereview.chromium.org/390773002/diff/40001/chrome/browser/net/webso...
chrome/browser/net/websocket_browsertest.cc:93: // Launch a secure WebSocket
server.
On 2014/07/14 10:17:20, Adam Rice wrote:
> Remove the word "secure".

Done.

https://codereview.chromium.org/390773002/diff/40001/chrome/browser/net/webso...
chrome/browser/net/websocket_browsertest.cc:110: NavigateToHTTP("connect.html");
On 2014/07/14 10:31:30, Adam Rice wrote:
> The name "connect.html" is too generic. How about "counted-connection.html" so
> it is easy to associate with "count-connection.html" used by the same test.

Done.

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/conne...
File net/data/websocket/connect.html (right):

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/conne...
net/data/websocket/connect.html:6: var href = window.location.href;
On 2014/07/14 10:17:20, Adam Rice wrote:
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pywebs...
> for a more elegant way to do this. (Optional).

Done.

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/count...
File net/data/websocket/count-connection_wsh.py (right):

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/count...
net/data/websocket/count-connection_wsh.py:1: # Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/07/14 10:31:30, Adam Rice wrote:
> Update copyright message.
> 
> Also, you need to add a short description of these files to the README file in
> the same directory.

Done.

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/count...
net/data/websocket/count-connection_wsh.py:5: import struct
On 2014/07/14 10:31:31, Adam Rice wrote:
> Not used.

Done.

https://codereview.chromium.org/390773002/diff/40001/net/data/websocket/count...
net/data/websocket/count-connection_wsh.py:10: from mod_pywebsocket import
handshake
On 2014/07/14 10:31:30, Adam Rice wrote:
> Neither mod_pywebsocket import is used.

Done.

[Adam Rice, 2014-07-14 12:12:15]

https://codereview.chromium.org/390773002/diff/80001/net/data/websocket/conne...
File net/data/websocket/connect_check.html (right):

https://codereview.chromium.org/390773002/diff/80001/net/data/websocket/conne...
net/data/websocket/connect_check.html:6: var scheme =
location.protocol.replace('http', 'ws');
Clever! But I think the URL will end up like
ws:127.0.0.1:3456/echo-with-no-extension, ie. missing "//".

https://codereview.chromium.org/390773002/diff/80001/net/data/websocket/count...
File net/data/websocket/count-connection_wsh.py (right):

https://codereview.chromium.org/390773002/diff/80001/net/data/websocket/count...
net/data/websocket/count-connection_wsh.py:1: # Copyright (c) 2014 The Chromium
Authors. All rights reserved.
The "(c)" isn't used any more.

[yhirano, 2014-07-15 01:50:03]

https://codereview.chromium.org/390773002/diff/80001/net/data/websocket/conne...
File net/data/websocket/connect_check.html (right):

https://codereview.chromium.org/390773002/diff/80001/net/data/websocket/conne...
net/data/websocket/connect_check.html:6: var scheme =
location.protocol.replace('http', 'ws');
On 2014/07/14 12:12:14, Adam Rice wrote:
> Clever! But I think the URL will end up like
> ws:127.0.0.1:3456/echo-with-no-extension, ie. missing "//".

Oops, sorry. Fixed.

https://codereview.chromium.org/390773002/diff/80001/net/data/websocket/count...
File net/data/websocket/count-connection_wsh.py (right):

https://codereview.chromium.org/390773002/diff/80001/net/data/websocket/count...
net/data/websocket/count-connection_wsh.py:1: # Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/07/14 12:12:14, Adam Rice wrote:
> The "(c)" isn't used any more.

Done.

[Adam Rice, 2014-07-15 06:03:30]

lgtm

[yhirano, 2014-07-15 07:23:17]

+jgraettinger for net/data.
+eroman for chrome/browser/net.

[Johnny, 2014-07-15 18:00:54]

net/data lgtm

https://codereview.chromium.org/390773002/diff/100001/net/data/websocket/README
File net/data/websocket/README (right):

https://codereview.chromium.org/390773002/diff/100001/net/data/websocket/READ...
net/data/websocket/README:22: - counted-connection.html : A page that creates a
WebSocket connection
Would be good to decide on a underscore_filename vs hyphen-filename convention
here.

[yhirano, 2014-07-16 02:11:46]

https://codereview.chromium.org/390773002/diff/100001/net/data/websocket/README
File net/data/websocket/README (right):

https://codereview.chromium.org/390773002/diff/100001/net/data/websocket/READ...
net/data/websocket/README:22: - counted-connection.html : A page that creates a
WebSocket connection
On 2014/07/15 18:00:53, Johnny wrote:
> Would be good to decide on a underscore_filename vs hyphen-filename convention
> here.

Done.

[yhirano, 2014-07-18 09:29:05]

eroman: ping

[yhirano, 2014-07-23 08:45:41]

eroman: ping

[yhirano, 2014-07-24 13:38:54]

+mmenke for chrome/browser/net

[mmenke, 2014-07-24 14:34:32]

Just a couple style nits, LGTM, deferring to other reviewers on actual content
of the tests.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
File chrome/browser/net/websocket_browsertest.cc (right):

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:20: :
ws_server_(net::SpawnedTestServer::TYPE_WS,
This should use 4-space indent.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:30: net::SpawnedTestServer
wss_server_;
data members should go at the end of a section.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:32: void NavigateToHTTP(const
std::string& path) {
Should probably include string header.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:33: // Visit a HTTP page for
testing.
This section should use 2-space indent.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:36:
replacements.SetSchemeStr(scheme);
And the gurl header as well.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:43: // Visit a HTTPS page for
testing.
This section should use 2-space indent.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:73: EXPECT_TRUE(EqualsASCII(result,
"PASS"));
optional:  Suggest using:

EXPECT_EQ(base::ASCIIToUTF16("PASS"), result);

On all of these.  Believe that will print out both strings on a mismatch, or at
least attempt to.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:100: content::WebContents* newTab =
new_tab.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:105: content::TitleWatcher
title_watcher(newTab,
Suggest using another name for this watcher, to make it clearer it's watching
the new tab, and to more clearly distinguish it from the title_watcher below

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:108:
content::WebContentsDestroyedWatcher destroyed_watcher(newTab);
Any reason to declare this here, instead of just before it's actually used (Per
google style guidelines)?  I don't think there's any reason why it should be
destroyed before your call to CloseWebContentsAt, but could be missing
something.

[yhirano, 2014-07-25 01:49:00]

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
File chrome/browser/net/websocket_browsertest.cc (right):

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:20: :
ws_server_(net::SpawnedTestServer::TYPE_WS,
On 2014/07/24 14:34:32, mmenke wrote:
> This should use 4-space indent.

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:30: net::SpawnedTestServer
wss_server_;
On 2014/07/24 14:34:32, mmenke wrote:
> data members should go at the end of a section.

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:32: void NavigateToHTTP(const
std::string& path) {
On 2014/07/24 14:34:32, mmenke wrote:
> Should probably include string header.

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:33: // Visit a HTTP page for
testing.
On 2014/07/24 14:34:32, mmenke wrote:
> This section should use 2-space indent.

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:36:
replacements.SetSchemeStr(scheme);
On 2014/07/24 14:34:32, mmenke wrote:
> And the gurl header as well.

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:43: // Visit a HTTPS page for
testing.
On 2014/07/24 14:34:31, mmenke wrote:
> This section should use 2-space indent.

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:73: EXPECT_TRUE(EqualsASCII(result,
"PASS"));
On 2014/07/24 14:34:32, mmenke wrote:
> optional:  Suggest using:
> 
> EXPECT_EQ(base::ASCIIToUTF16("PASS"), result);
> 
> On all of these.  Believe that will print out both strings on a mismatch, or
at
> least attempt to.

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:100: content::WebContents* newTab =
On 2014/07/24 14:34:32, mmenke wrote:
> new_tab.

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:105: content::TitleWatcher
title_watcher(newTab,
On 2014/07/24 14:34:32, mmenke wrote:
> Suggest using another name for this watcher, to make it clearer it's watching
> the new tab, and to more clearly distinguish it from the title_watcher below

Done.

https://codereview.chromium.org/390773002/diff/210001/chrome/browser/net/webs...
chrome/browser/net/websocket_browsertest.cc:108:
content::WebContentsDestroyedWatcher destroyed_watcher(newTab);
On 2014/07/24 14:34:32, mmenke wrote:
> Any reason to declare this here, instead of just before it's actually used
(Per
> google style guidelines)?  I don't think there's any reason why it should be
> destroyed before your call to CloseWebContentsAt, but could be missing
> something.

Done.

[yhirano, 2014-07-25 01:49:22]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-07-25 02:26:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/390773002/250001

[commit-bot: I haz the power, 2014-07-25 07:17:09]

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

[commit-bot: I haz the power, 2014-07-25 10:32:52]

Change committed as 285557

[Vladislav Kaznacheev, 2014-07-25 11:55:34]

A revert of this CL has been created in
https://codereview.chromium.org/416333004/ by kaznacheev@chromium.org.

The reason for reverting is:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

[Adam Rice, 2014-07-25 12:20:17]

https://codereview.chromium.org/390773002/diff/250001/content/browser/rendere...
File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/390773002/diff/250001/content/browser/rendere...
content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:64: virtual
~WebSocketDispatcherHostTest() {}
We need to add dispatcher_host_ = NULL here to avoid gone_hosts_ being accessed
after its destructor has been called.

An alternative would be to change the order of the member variables, but I think
explicitly freeing dispatcher_host_ in the destructor is safer.

[yhirano1, 2014-07-25 13:16:01]

reopen

[yhirano, 2014-07-28 03:54:06]

ricea@, PTAL.

https://codereview.chromium.org/390773002/diff/250001/content/browser/rendere...
File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/390773002/diff/250001/content/browser/rendere...
content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:64: virtual
~WebSocketDispatcherHostTest() {}
On 2014/07/25 12:20:16, Adam Rice wrote:
> We need to add dispatcher_host_ = NULL here to avoid gone_hosts_ being
accessed
> after its destructor has been called.
> 
> An alternative would be to change the order of the member variables, but I
think
> explicitly freeing dispatcher_host_ in the destructor is safer.

CL15 suppresses adding gone_hosts_ in the destructor.

[Adam Rice, 2014-07-28 04:29:22]

https://codereview.chromium.org/390773002/diff/270001/content/browser/rendere...
File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/390773002/diff/270001/content/browser/rendere...
content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:66:
on_destruction_ = true;
This doesn't seem safe to me. How about changing the MockWebSocketHost owner_
pointer to a WeakPtr, and checking it is non-null before calling
owner_->GoAway() ?

[yhirano, 2014-07-28 05:08:53]

https://codereview.chromium.org/390773002/diff/270001/content/browser/rendere...
File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/390773002/diff/270001/content/browser/rendere...
content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:66:
on_destruction_ = true;
On 2014/07/28 04:29:22, Adam Rice wrote:
> This doesn't seem safe to me. How about changing the MockWebSocketHost owner_
> pointer to a WeakPtr, and checking it is non-null before calling
> owner_->GoAway() ?

Done.

[Adam Rice, 2014-07-28 07:04:48]

https://codereview.chromium.org/390773002/diff/290001/content/browser/rendere...
File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/390773002/diff/290001/content/browser/rendere...
content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:53: public
base::SupportsWeakPtr<WebSocketDispatcherHostTest> {
You don't need to use base::SupportsWeakPtr and base::WeakPtrFactory together.
You only need one or the other.

[yhirano, 2014-07-28 07:12:48]

https://codereview.chromium.org/390773002/diff/290001/content/browser/rendere...
File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/390773002/diff/290001/content/browser/rendere...
content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:53: public
base::SupportsWeakPtr<WebSocketDispatcherHostTest> {
On 2014/07/28 07:04:48, Adam Rice wrote:
> You don't need to use base::SupportsWeakPtr and base::WeakPtrFactory together.
> You only need one or the other.

Thanks, done.

[Adam Rice, 2014-07-28 07:15:08]

lgtm

[yhirano, 2014-07-28 07:22:10]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-07-28 07:22:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/390773002/300001

[commit-bot: I haz the power, 2014-07-28 10:01:12]

Change committed as 285884