Active WebSocket should send closing handshake to server on page navigation or page close

Active WebSocket should send closing handshake to server on page navigation or page close

Specification URL: http://dev.w3.org/html5/websockets/#garbage-collection

BUG=275280
R=tyoshino

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156381

[vivekg__, 2013-08-19 11:04:31]

This is a WIP patch. As discussed over the bug, it still is not working for page
close scenario. Investigating the same.

Need to add the appropriate test scenario to test the closing handshake.

[tyoshino (SeeGerritForStatus), 2013-08-20 03:07:06]

https://codereview.chromium.org/23201008/diff/1/LayoutTests/http/tests/websoc...
File LayoutTests/http/tests/websocket/tests/hybi/close-on-unload_wsh.py (left):

https://codereview.chromium.org/23201008/diff/1/LayoutTests/http/tests/websoc...
LayoutTests/http/tests/websocket/tests/hybi/close-on-unload_wsh.py:51:
socketName = socketName + ': receive next message'
looks ok to remove this. instead of this line, could you please get the received
close code using request.ws_close_code and append it to socketName so that we
can check if the right code was sent?

https://codereview.chromium.org/23201008/diff/1/Source/modules/websockets/Web...
File Source/modules/websockets/WebSocket.cpp (right):

https://codereview.chromium.org/23201008/diff/1/Source/modules/websockets/Web...
Source/modules/websockets/WebSocket.cpp:508:
m_channel->close(WebSocketChannel::CloseEventCodeGoingAway, "");
let me correct. please use String() for consistency.

[vivekg__, 2013-08-20 03:28:10]

Thank you @tyoshino for the review. I have uploaded a new patch with the
comments addressed.

[tyoshino (SeeGerritForStatus), 2013-08-20 05:02:18]

Looks good. Please update expectation files, and then I think we can land this
first and work on the tab close case later separately.

[vivekg__, 2013-08-20 05:04:20]

On 2013/08/20 05:02:18, tyoshino wrote:
> Looks good. Please update expectation files, and then I think we can land this
> first and work on the tab close case later separately.

The tests are all passing now! So I guess we wouldn't need to update the
expectation file.

[tyoshino (SeeGerritForStatus), 2013-08-20 05:18:00]

On 2013/08/20 05:04:20, vivekg_ wrote:
> On 2013/08/20 05:02:18, tyoshino wrote:
> > Looks good. Please update expectation files, and then I think we can land
this
> > first and work on the tab close case later separately.
> 
> The tests are all passing now! So I guess we wouldn't need to update the
> expectation file.

OK. Looks like this is because iframeReady is invoked when the first message is
received by the childWebSocket.

Let's start using the new code (close code echo back) in a separate CL.
http://crbug.com/276107

Please inlucde a link to
http://dev.w3.org/html5/websockets/#garbage-collection
in the CL description.

Then, LGTM

[vivekg_samsung, 2013-08-20 05:27:27]

On 2013/08/20 05:18:00, tyoshino wrote:
> On 2013/08/20 05:04:20, vivekg_ wrote:
> > On 2013/08/20 05:02:18, tyoshino wrote:
> > > Looks good. Please update expectation files, and then I think we can land
> this
> > > first and work on the tab close case later separately.
> > 
> > The tests are all passing now! So I guess we wouldn't need to update the
> > expectation file.
> 
> OK. Looks like this is because iframeReady is invoked when the first message
is
> received by the childWebSocket.
> 
> Let's start using the new code (close code echo back) in a separate CL.
> http://crbug.com/276107
> 
> Please inlucde a link to
> http://dev.w3.org/html5/websockets/#garbage-collection
> in the CL description.
> 
> Then, LGTM

Sounds great. I have updated the description. I will start working on the other
CL immediately. Thank you!

[commit-bot: I haz the power, 2013-08-20 05:28:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/23201008/6001

[commit-bot: I haz the power, 2013-08-20 05:37:13]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-08-20 05:47:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/23201008/6001

[commit-bot: I haz the power, 2013-08-20 05:52:28]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-08-20 06:21:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/23201008/6001

[commit-bot: I haz the power, 2013-08-20 06:52:46]

Retried try job too often on win_blink_rel for step(s) webkit_lint,
webkit_python_tests, webkit_tests, webkit_unit_tests, weborigin_unittests,
wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-08-20 08:47:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/23201008/6001

[commit-bot: I haz the power, 2013-08-20 09:18:42]

Change committed as 156381