Bug 35721 – WebSocket closing handshake

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Fumitoshi Ukai]

WebSocket API Editor's Draft 4 March 2010 /draft-hixie-thewebsocketprotocol-76 introduces closing handshake (CLOSING state, etc)

[Fumitoshi Ukai]

Created attachment 56682 [details]
Patch

[Fumitoshi Ukai]

Created attachment 56848 [details]
Patch

[Kent Hansen]

(In reply to comment #2)
> Created an attachment (id=56848) [details]
> Patch

There are no new tests for CLOSING. Isn't it possible to reliably observe this state from a test?
What about the fail() case, how is that codepath tested?

[Fumitoshi Ukai]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=56848) [details] [details]
> > Patch
> 
> There are no new tests for CLOSING. Isn't it possible to reliably observe this state from a test?
> What about the fail() case, how is that codepath tested?

I'll add test for CLOSING that client has been sent closing frame (0xFF 0x00).
The fail case would be exposed by CloseEvent, which would be added by https://bugs.webkit.org/show_bug.cgi?id=35573
Could you take a look of that patch, please?
Thanks.

[Fumitoshi Ukai]

Created attachment 62800 [details]
Patch

[Alexey Proskuryakov]

<rdar://problem/8467487>

[Yuta Kitamura]

I think we really need to resurrect this thread and CloseEvent thread and move forward. Maybe I should rebase the patch to trunk and re-upload it for review...

[Alexey Proskuryakov]

Is there a reason to be modifying an implementation of v76 protocol? My understanding is that the next protocol is still in early stages of discussion, perhaps we should wait for that?

[Yuta Kitamura]

* CloseEvent and closing handshake is part of hixie-76 (= hybi-00) protocol. Our implementation is not compliant with hixie-76.

* I think these features are likely to be included in the next generation WebSocket protocol / API. We probably need to make progress on this patch and CloseEvent patch sooner or later.

* I would like to start to implement the new protocol as early as possible, as some flaws of the current protocol are already pointed out in the hybi list. We can also provide feedback to the protocol by implementing the protocol early.

[Alexey Proskuryakov]

Compliance with hixie-76 doesn't matter any more - our implementation is what Web developers target. We shouldn't disrupt it without a very good reason, and compliance with an obsolete spec is not such a reason.

[Yuta Kitamura]

So you are saying hixie-76 (= hybi-00) is deprecate and hybi-03 is too early to implement? This means that no protocol specification is effective for us and prevents us from doing anything. Your argument does not make much sense to me.

CloseEvent and closing handshake is unlikely to be removed from the future specification, and these features will be an important basis to implement the future protocol.

Furthermore, CloseEvent and closing handshake are mainly included in the WebSocket API specification rather than the WebSocket protocol specification. I think WebSocket API has been valid since its last update and is not deprecated...

[Alexey Proskuryakov]

> This means that no protocol specification is effective for us and prevents us from doing anything.

That cannot prevent us from doing anything.

I think that close events are fairly likely to be changed even from API perspective, and since WebSockets are no longer an experimental feature, the risk of adding the event seems to outweigh the benefit. Specifically, there is almost no known benefit - the only rationale given in bug 35573 was that the event was added to the spec.

[Adam Barth]

> WebSockets are no longer an experimental feature

What makes WebSockets no longer an experimental feature?

[Joe Mason]

(In reply to comment #12)
> I think that close events are fairly likely to be changed even from API perspective, and since WebSockets are no longer an experimental feature, the risk of adding the event seems to outweigh the benefit. Specifically, there is almost no known benefit - the only rationale given in bug 35573 was that the event was added to the spec.

Why do you think that close events will be changed?  Apart from the opening handshake the spec seems stable now.  Any further changes are likely to be driven by implementor's experiences - ie. us.

I agree that any behaviour change to the -76 protocol is a mistake.  We should do this development for the -03 protocol, in a separate branch.  (Or do them in the main branch but use a feature define to toggle between them so that users of the nightlies can play around with the new protocol.)

[Alexey Proskuryakov]

Last I checked, the semantics of the close event were very far from what TCP/IP had, so the folks in IETF may well change it.

[Yuta Kitamura]

Discussion about close frame and closing handshake has been settled down in the IETF hybi list, and hybi draft 06 now includes closing handshake that is almost identical to hixie-76's one.

http://www.ietf.org/id/draft-ietf-hybi-thewebsocketprotocol-06.txt
(See "4.5.1. Close")

The main purpose of introducing closing handshake is (IMO) to avoid TCP RST hazard. When an endpoint receives a close frame, it is guaranteed to receive no more bytes and can safely close the underlying socket. If an endpoint tries to close a socket which has unread data, a TCP RST packet may be sent to the other endpoint and it may cause undesireble result.

So, I think it's good time to get closing handshake implemented and make our implementation conformant to hixie-76 at least.

If there is no objection, I'd like to rebase this patch to make it apply to trunk...

[Alexey Proskuryakov]

OK, let's work on it.

[Yuta Kitamura]

Created attachment 86572 [details]
Merge to trunk

[Alexey Proskuryakov]

Looks like there are objections being made to WebSocket closing handshake, after all. I started researching what people are saying, and found this: <https://www.infosecisland.com/blogview/12681-The-WebSocket-Protocol-Past-Travails-To-Be-Avoided.html>.

[Takeshi Yoshino]

(In reply to comment #19)
<https://www.infosecisland.com/blogview/12681-The-WebSocket-Protocol-Past-Travails-To-Be-Avoided.html>.

He's just arguing wording in the current spec. Not objecting to having closing handshake and its usefulness.

Purposes of closing handshake are
a) tell intermediaries (which understands WebSocket) that the connection is abandoned so it's ok to release resources for it such as memory, TCP session without depending on TCP FIN/RST
b) prevent RST hazard (An endpoint may close socket without draining received data. This causes sending out RST (since TCP stack couldn't deliver the received data to the upper layer) on some platform. This RST may impede the other peer from handling received data). This can be done by relying on TCP's FIN i.e. shutdown(SHUT_WR) and 0 from read() call on some platform, but they're not always available. We're going to address this by making sure both peers send out close frame to tell the other peer there will be no more data coming, so it's safe to call close on their socket.

For a), replying close frame is not necessary. For b), it's necessary. The current spec is adopting b).

As far as I know, no one is objecting to have a). Gabriel of Microsoft was doubting usefulness of b) but not objecting actively now. Several people on the list are demanding b)-feature to perform safe closure including Jamie Lokier one of the people who initially proposed closing handshake. In reality, when user agent doesn't care RST hazard (e.g. the browser is going down) and want to throw away resource for the connection immediately without waiting for close frame in reply, the user agent may just abandon the connection. It may be categorized as abnormal closure but not protocol violation.

There's not consensus call made, but I'm almost sure the closing handshake will be kept as is on the spec now.

[Yuta Kitamura]

Could anybody review this? The discussion about closing semantics has been settled down for long (in IETF hybi mailing list), and no objections were raised as far as I observed. The article in comment #19 argues about just editorial issues, not protocol semantics.

It's good time to move towards implementing the new protocol.

[Kent Tamura]

Comment on attachment 86572 [details]
Merge to trunk

View in context: https://bugs.webkit.org/attachment.cgi?id=86572&action=review

I understand the patch is harmless and we should go ahead.

Can you split the patch into multiple parts?
e.g.
  1. Adding CLOSING to WebSocket.idl, WebSocket.h, and SocketStreamHandleBase.h, and update test results.
  2. Adding ThreadableWebSocketChannelClientWrapper.cpp, and move existing inline function definitions in ThreadableWebSocketChannelClientWrapper.h to .cpp.
  3. Remaining

> LayoutTests/http/tests/websocket/tests/script-tests/close-frame.js:1
> +description("Make sure client sends closing frame.");

You can merge this file into close-frame.html.

> LayoutTests/http/tests/websocket/tests/script-tests/close-frame.js:12
> +if (window.layoutTestController)
> +    layoutTestController.waitUntilDone();
> +
> +function endTest()
> +{
> +    isSuccessfullyParsed();
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();
> +}

You can simplify this code by jsTestIsAsync=true and finishJSTest().

> Source/WebCore/websockets/WebSocket.cpp:182
> +    if (m_state == CONNECTING) {
> +      m_state = CLOSING;
> +      m_channel->fail();
> +      return;
> +    }

Wrong indentation.

> Source/WebCore/websockets/WebSocket.cpp:188
> -        m_channel->close();
> +      m_channel->close();

ditto.

[Yuta Kitamura]

Comment on attachment 86572 [details]
Merge to trunk

Sure, I'm going to split the patch...

[Yuta Kitamura]

Part 1: Add CLOSING state (bug 60878)

[Yuta Kitamura]

Part 2: Clean up ThreadableWebSocketChannelClientWrapper (bug 60945) (already committed)

[Yuta Kitamura]

(I realized that I could still divide the remaining patch into smaller pieces, so I'm posting more patches than Kent's suggestion.)

Part 3: Use ScriptExecutionContext::Task in ThreadableWebSocketChannelClientWrapper (bug 61034).

[Kent Tamura]

(In reply to comment #26)
> (I realized that I could still divide the remaining patch into smaller pieces, so I'm posting more patches than Kent's suggestion.)

Great!  Smaller is better.

[Yuta Kitamura]

Created attachment 94928 [details]
The final part

[Kent Tamura]

Comment on attachment 94928 [details]
The final part

View in context: https://bugs.webkit.org/attachment.cgi?id=94928&action=review

> Source/WebCore/websockets/WebSocketChannel.cpp:127
> +        m_closingTimer.startOneShot(2 * 2 * 60.0); // 2 * TCP Maximum Segment Lifetime.

So we had better add
  const double TCPMaximumSegmentLifetime = 2 * 60
and use it like startOneShot(2 * TCPMaximumSegmentLifetime);

> Source/WebCore/websockets/WebSocketChannelClient.h:45
> +        virtual void didClose(unsigned long /* unhandledBufferedAmount */, bool /* didReceiveClosingHandshake */) { }

Do not add bool argument.  We should use enum.

[WebKit Review Bot]

Comment on attachment 94928 [details]
The final part

Attachment 94928 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8736212

New failing tests:
http/tests/websocket/tests/client-close.html
http/tests/websocket/tests/server-close.html

[WebKit Review Bot]

Created attachment 94951 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yuta Kitamura]

(In reply to comment #30)
> (From update of attachment 94928 [details])
> Attachment 94928 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8736212
> 
> New failing tests:
> http/tests/websocket/tests/client-close.html
> http/tests/websocket/tests/server-close.html

This seems like a false alarm; the log says the bot failed to start the new pywebsocket server:

[2011-05-26 09:45:34,085] [CRITICAL] root: mod_pywebsocket: [Errno 98] Address already in use
[2011-05-26 09:45:34,086] [CRITICAL] root: mod_pywebsocket: Traceback (most recent call last):
  File "/mnt/git/webkit-chromium-ews/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pywebsocket/mod_pywebsocket/standalone.py", line 464, in _main
    WebSocketRequestHandler)
  File "/mnt/git/webkit-chromium-ews/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pywebsocket/mod_pywebsocket/standalone.py", line 189, in __init__
    self.server_bind()
  File "/usr/lib/python2.6/BaseHTTPServer.py", line 108, in server_bind
    SocketServer.TCPServer.server_bind(self)
  File "/usr/lib/python2.6/SocketServer.py", line 413, in server_bind
    self.socket.bind(self.server_address)
  File "<string>", line 1, in bind
error: [Errno 98] Address already in use

I usually use chromium-linux for local development and tests, and they are passing in my local environment. So I guess they are okay...

[Yuta Kitamura]

Created attachment 94957 [details]
The final part v2

[WebKit Commit Bot]

Comment on attachment 94957 [details]
The final part v2

Clearing flags on attachment: 94957

Committed r87464: <http://trac.webkit.org/changeset/87464>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

New tests added in this change are flaky on Windows and Mac.  See:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fclient-close.html%2Chttp%2Ftests%2Fwebsocket%2Ftests%2Fserver-close.html

and

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20chromium.org&showExpectations=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fclient-close.html%2Chttp%2Ftests%2Fwebsocket%2Ftests%2Fserver-close.html

[Adam Klein]

(In reply to comment #36)
> New tests added in this change are flaky on Windows and Mac.  See:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fclient-close.html%2Chttp%2Ftests%2Fwebsocket%2Ftests%2Fserver-close.html
> 
> and
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20chromium.org&showExpectations=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fclient-close.html%2Chttp%2Ftests%2Fwebsocket%2Ftests%2Fserver-close.html

Since these failures don't look due to test fixture flakiness (all other websocket tests pass in the same run),  these look like real problems with the tests themselves (or the underlying code), so I've rolled your change out.

[Yuta Kitamura]

I think these failures are caused by stale pywebsocket processes in bots.

This patch adds new pywebsocket tests, thus websocket server must be restarted. Normally the websocket server is shut down at the end of test run, but it is known that a stale pywebsocket process may sometimes continue to live; if this happens, the new websocket server process will not be created successfully, because the port is occupied by the stale process.

If there's pywebsocket.ws.log that contains "Address already in use" errors, the bot is probably affected by this issue.

So, if you see failures of client-close.html and/or server-close.html (these are new tests added in this patch), the server restart will probably fix the failures.

[Yuta Kitamura]

I'm trying to re-land the patch and will look at the Chromium bots closely.

[Yuta Kitamura]

Created attachment 95322 [details]
Ready to land

[WebKit Commit Bot]

Comment on attachment 95322 [details]
Ready to land

Clearing flags on attachment: 95322

Committed r87674: <http://trac.webkit.org/changeset/87674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.