Notify WebSocket connection failure, chromium side

Notify WebSocket connection failure, chromium side

In order to set |wasClean| property of CloseEvent and show the failure message
correctly, The blink-side WebSocket need to know whether the connection
is dropped because of a server request.

Blink side change is: https://codereview.chromium.org/36033003/
BUG=310405
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232921

[Adam Rice, 2013-10-23 03:18:05]

https://codereview.chromium.org/34753008/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/34753008/diff/70001/content/browser/renderer_...
content/browser/renderer_host/websocket_host.cc:187: // TODO(yhirano): Use
|fail| appropriately.
We should just delete the WebSocketChannel if |fail| is true. But we also need
to de-register it from WebSocketDispatcherHost. No existing method on
WebSocketDispatcherHost does the right thing, so I think we will need a new one.
WebSocketHost does not keep a pointer back to WebSocketDispatcherHost; I am not
sure whether it is a good idea to add one, or if it is risky. Alternatively,
adding a new method to WebSocketChannel like ClientFail() or similar, and then
adding a new event type to WebSocketEventInterface (OnClientFail ?) would work.

https://codereview.chromium.org/34753008/diff/70001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/34753008/diff/70001/content/common/websocket_...
content/common/websocket_messages.h:103: // |fail| is true when the user agent
is required to fail the connection.
I think this comment needs more explanation. How about:

If |fail| is true on a message from the browser, then the WebSocket connection
was not closed cleanly. If |fail| is true on a message from the renderer, then
the connection should be closed immediately without a closing handshake and the
renderer cannot accept any new messages about this connection.

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:295: // TODO(yhirano): Set |fail|
appropriately.
The OnDropChannel-calling code changes a bit in
https://codereview.chromium.org/26544003/ so I think we should leave this as a
TODO until that has landed (and the other ones).

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
File net/websockets/websocket_event_interface.h (right):

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
net/websockets/websocket_event_interface.h:54: // |fail| is true if the user
agent is required to fail the connection.
I have just realised we may need more information than just a boolean parameter.
From
http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#conc...
:

"""
2. If the user agent was required to fail the WebSocket connection or the
WebSocket connection is closed with prejudice, fire a simple event named error
at the WebSocket object. [WSP]

3. Create a trusted event that uses the CloseEvent interface, with the event
type close, which does not bubble, is not cancelable, has no default action,
whose wasClean attribute is initialized to true if the connection closed cleanly
and false otherwise
"""

I see three questions:

1. Was the user agent required to fail the WebSocket connection?
2. Was the WebSocket connection closed with prejudice?
3. Did the connection close cleanly?

I am not sure whether case 1. only applies to requirements from the WHATWG spec
or also includes requirements from RFC6455.

I have not been able to find a definition for "with prejudice" as mentioned in
case 2.

If case 1 or 2 are true, then case 3 is definitely true. However, I think case 3
can be true without cases 1 or 2 being true.

[tyoshino (SeeGerritForStatus), 2013-10-23 05:21:33]

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
File net/websockets/websocket_event_interface.h (right):

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
net/websockets/websocket_event_interface.h:54: // |fail| is true if the user
agent is required to fail the connection.
On 2013/10/23 03:18:06, Adam Rice wrote:
> I see three questions:
> 
> 1. Was the user agent required to fail the WebSocket connection?
> 2. Was the WebSocket connection closed with prejudice?
> 3. Did the connection close cleanly?
> 
> I am not sure whether case 1. only applies to requirements from the WHATWG
spec
> or also includes requirements from RFC6455.

I think yes. But basically we should focus on keeping the new stack behave
consistently to the old stack.

> 
> I have not been able to find a definition for "with prejudice" as mentioned in
> case 2.
> 

An error event should be fired where "must close the ... with prejudice" is
used.

In other words, those sentences are defining what "with prejudice" means.

> If case 1 or 2 are true, then case 3 is definitely true. However, I think case
3
> can be true without cases 1 or 2 being true.

[yhirano, 2013-10-23 05:42:23]

To avoid conflict with other CLs currently reviewed, I minimize the change and
add some TODOs.

https://codereview.chromium.org/34753008/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/34753008/diff/70001/content/browser/renderer_...
content/browser/renderer_host/websocket_host.cc:187: // TODO(yhirano): Use
|fail| appropriately.
On 2013/10/23 03:18:06, Adam Rice wrote:
> We should just delete the WebSocketChannel if |fail| is true. But we also need
> to de-register it from WebSocketDispatcherHost. No existing method on
> WebSocketDispatcherHost does the right thing, so I think we will need a new
one.
> WebSocketHost does not keep a pointer back to WebSocketDispatcherHost; I am
not
> sure whether it is a good idea to add one, or if it is risky. Alternatively,
> adding a new method to WebSocketChannel like ClientFail() or similar, and then
> adding a new event type to WebSocketEventInterface (OnClientFail ?) would
work.
Will do in a future CL.

https://codereview.chromium.org/34753008/diff/70001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/34753008/diff/70001/content/common/websocket_...
content/common/websocket_messages.h:103: // |fail| is true when the user agent
is required to fail the connection.
On 2013/10/23 03:18:06, Adam Rice wrote:
> I think this comment needs more explanation. How about:
> 
> If |fail| is true on a message from the browser, then the WebSocket connection
> was not closed cleanly. If |fail| is true on a message from the renderer, then
> the connection should be closed immediately without a closing handshake and
the
> renderer cannot accept any new messages about this connection.

Thanks, done.

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:295: // TODO(yhirano): Set |fail|
appropriately.
On 2013/10/23 03:18:06, Adam Rice wrote:
> The OnDropChannel-calling code changes a bit in
> https://codereview.chromium.org/26544003/ so I think we should leave this as a
> TODO until that has landed (and the other ones).

OK, I will implement this in future CLs.

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
File net/websockets/websocket_event_interface.h (right):

https://codereview.chromium.org/34753008/diff/70001/net/websockets/websocket_...
net/websockets/websocket_event_interface.h:54: // |fail| is true if the user
agent is required to fail the connection.
On 2013/10/23 03:18:06, Adam Rice wrote:
> I have just realised we may need more information than just a boolean
parameter.
> From
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#conc...
> :
> 
> """
> 2. If the user agent was required to fail the WebSocket connection or the
> WebSocket connection is closed with prejudice, fire a simple event named error
> at the WebSocket object. [WSP]
> 
> 3. Create a trusted event that uses the CloseEvent interface, with the event
> type close, which does not bubble, is not cancelable, has no default action,
> whose wasClean attribute is initialized to true if the connection closed
cleanly
> and false otherwise
> """
> 
> I see three questions:
> 
> 1. Was the user agent required to fail the WebSocket connection?
> 2. Was the WebSocket connection closed with prejudice?
> 3. Did the connection close cleanly?
> 
> I am not sure whether case 1. only applies to requirements from the WHATWG
spec
> or also includes requirements from RFC6455.
> 
> I have not been able to find a definition for "with prejudice" as mentioned in
> case 2.
> 
> If case 1 or 2 are true, then case 3 is definitely true. However, I think case
3
> can be true without cases 1 or 2 being true.
You are right.
IIUC "close with prejudice" occurs in the renderer and we don't have to care
about it here.

I added "NotifyFailure" message to websocket_message.h.

[Adam Rice, 2013-10-23 07:15:30]

https://codereview.chromium.org/34753008/diff/220001/content/common/websocket...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/34753008/diff/220001/content/common/websocket...
content/common/websocket_messages.h:66: // |message| will be shown in the
inspector and won't be passed to the
If we had a way to pass the message directly to the process running the
inspector it would be preferable. But then I suppose we'd lose file & line
information.

Could you put in a TODO for "Find a way to not expose this information to the
renderer process"? Then we can worry about it when we are less busy.

[yhirano, 2013-10-23 08:10:42]

https://codereview.chromium.org/34753008/diff/220001/content/common/websocket...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/34753008/diff/220001/content/common/websocket...
content/common/websocket_messages.h:66: // |message| will be shown in the
inspector and won't be passed to the
On 2013/10/23 07:15:30, Adam Rice wrote:
> If we had a way to pass the message directly to the process running the
> inspector it would be preferable. But then I suppose we'd lose file & line
> information.
> 
> Could you put in a TODO for "Find a way to not expose this information to the
> renderer process"? Then we can worry about it when we are less busy.

Done.

[Adam Rice, 2013-10-23 08:18:01]

lgtm

[tyoshino (SeeGerritForStatus), 2013-10-23 08:28:52]

https://codereview.chromium.org/34753008/diff/380001/content/child/websocket_...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/34753008/diff/380001/content/child/websocket_...
content/child/websocket_bridge.cc:228: // This method is for closing handshake
and hence |was_clean| shall be true.
NewWebSocketChannelImpl::disconnect() also uses this and it can be for kinda
failure?

[yhirano, 2013-10-23 08:32:55]

https://codereview.chromium.org/34753008/diff/380001/content/child/websocket_...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/34753008/diff/380001/content/child/websocket_...
content/child/websocket_bridge.cc:228: // This method is for closing handshake
and hence |was_clean| shall be true.
On 2013/10/23 08:28:53, tyoshino wrote:
> NewWebSocketChannelImpl::disconnect() also uses this and it can be for kinda
> failure?

You are right, I will fix NewWebSocketChannelImpl not to call close.

[yhirano, 2013-10-23 09:12:59]

https://codereview.chromium.org/34753008/diff/380001/content/child/websocket_...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/34753008/diff/380001/content/child/websocket_...
content/child/websocket_bridge.cc:228: // This method is for closing handshake
and hence |was_clean| shall be true.
On 2013/10/23 08:32:55, yhirano wrote:
> On 2013/10/23 08:28:53, tyoshino wrote:
> > NewWebSocketChannelImpl::disconnect() also uses this and it can be for kinda
> > failure?
> 
> You are right, I will fix NewWebSocketChannelImpl not to call close.
Done at the Blink-side change.
https://codereview.chromium.org/36033003/

[tyoshino (SeeGerritForStatus), 2013-10-25 05:59:20]

lgtm

[yhirano, 2013-10-31 18:06:17]

+jam, tsepez for OWNER review

[Tom Sepez, 2013-10-31 19:07:52]

LGTM from a security perspective with nits about comments. Thanks.

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
content/common/websocket_messages.h:62: // Notify the renderer that the browser
is required to fail the connection.
nit: is required by whom? or why is it required?

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
content/common/websocket_messages.h:63: // When the renderer process is received
this message, it will drop the
nit: When the renderer process receives this message,

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
content/common/websocket_messages.h:66: // |message| will be shown in the
inspector and won't be passed to the
nit: why not just send the WebSocket_DropChannel then?

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
content/common/websocket_messages.h:117: // closing handshake and the renderer
cannot accept any new messages about this
nit: on this connection.

[jam, 2013-10-31 20:44:16]

On 2013/10/31 18:06:17, yhirano wrote:
> +jam, tsepez for OWNER review

lgtm

You and others working on websocket stuff should be owners for these files, feel
free to send me a cl to do that

[yhirano, 2013-11-01 05:58:35]

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
content/common/websocket_messages.h:62: // Notify the renderer that the browser
is required to fail the connection.
On 2013/10/31 19:07:53, Tom Sepez wrote:
> nit: is required by whom? or why is it required?

By the spec. RFC6455 says:

Certain algorithms and specifications require an endpoint to _Failthe WebSocket
Connection_.

I added a link to the spec.

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
content/common/websocket_messages.h:63: // When the renderer process is received
this message, it will drop the
On 2013/10/31 19:07:53, Tom Sepez wrote:
> nit: When the renderer process receives this message,

Thanks, done.

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
content/common/websocket_messages.h:66: // |message| will be shown in the
inspector and won't be passed to the
On 2013/10/31 19:07:53, Tom Sepez wrote:
> nit: why not just send the WebSocket_DropChannel then?

The renderer process need to distinguish failure to fire an error event.

https://codereview.chromium.org/34753008/diff/550001/content/common/websocket...
content/common/websocket_messages.h:117: // closing handshake and the renderer
cannot accept any new messages about this
On 2013/10/31 19:07:53, Tom Sepez wrote:
> nit: on this connection.

Done.

[commit-bot: I haz the power, 2013-11-01 14:42:46]

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

[commit-bot: I haz the power, 2013-11-01 15:21:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-01 23:35:42]

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

[commit-bot: I haz the power, 2013-11-01 23:59:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-02 07:27:14]

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

[commit-bot: I haz the power, 2013-11-02 11:22:06]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-04 03:13:34]

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

[commit-bot: I haz the power, 2013-11-04 07:07:55]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-05 00:05:21]

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

[commit-bot: I haz the power, 2013-11-05 00:42:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-05 01:36:05]

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

[commit-bot: I haz the power, 2013-11-05 03:01:39]

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

[commit-bot: I haz the power, 2013-11-05 05:59:40]

Change committed as 232921