730742 - node.js mishandles Connection: header during websocket handshake

Status: RESOLVED FIXED

(Tech Evangelism Graveyard :: English US, defect)

Description

[Knight00931]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120226 Firefox/13.0a1
Build ID: 20120226031015

Steps to reproduce:

Just go to Twimbow (http://www.twimbow.com/dashboard.php), it's a twitter web client.
Error console says Firefox can't establish a connection to the server with ws://twimbow.com ...



Actual results:

Error console shows the following message many times.

Timestamp: 2012/2/27 上午 09:22:37
Error: Firefox can't establish a connection to the server at ws://twimbow.com:8125/rand/178840519c8ebf4aeb55910df24319c409db1133b3fb820bcc2e73f26345fbc5c5d81de8f0ea5eb162e8e563fb770da20e34d865545584c62f9309f6755abd5e11bf6aa15bdbe911080d257a8e1b88e21e8a47c681af86d6210264b3b8d171f926bd388f8dd51c3384c96aa4862808097666f7ab5fc3799f/410d551e8d2fba2423ed59110472b242/asd.
Source File: http://www.twimbow.com/js/TwitterUsersLoader.js
Line: 634


Expected results:

No error message like Firefox stable 10.0 serious.
p.s I've tried Firefox 10 and Nightly 20120226 build with fresh profile.
Thanks.

Comment 1

[Jason Duell]

This is a server bug--but we might want to change our behavior to make these bugs less likely.  

Our opening handshake for the HTTP connect includes

  Connection: keep-alive, Upgrade

while Chrome uses 

  Connection: Upgrade

RFC 2068 section 14.10 is quite clear that it's permissible to send multiple comma-delineated tokens in the Connection header.  And the websocket spec--RFC 6455--says that "The request MUST contain a |Connection| header field whose value MUST include the "Upgrade" token" (section 4.1): note the use of "include", not "be equal to".  But twimbow's server is simply failing to reply at all to our HTTP handshake, and we eventually time out the connection. (If I change the Connection header to just "Upgrade", we connect fine).

So not our fault.  But that said, is there any benefit to sending keep-alive?  I'm not even clear what it means in an HTTP/1.1 context, where connections are persistent by default.   Patrick, did you include keep-alive intentionally?

Also, we could potentially trim down our websocket handshake requests: chrome is sending only Connection, Host, Origin, Sec-WebSocket-{Key|Version}, Upgrade, and if needed, Cookie and Sec-WebSocket-Protocol.  I.e. only the headers required by the WS spec.   We also send Accept, Accept-{Encoding|Language}, Pragma: No-Cache, and User-Agent (I'm a bit surprised Chrome isn't sending User-Agent: I guess they're optimistic that servers won't need to work around UA differences).  This means (factoring out the GET and Cookie header, which obviously vary by request) that chrome has a baseline of 171 bytes of headers vs 448 for us.   As long as cookies and the request URI aren't too long, it's still one packet, so this isn't a big deal.  But if we want to trim, now would be a good time, while WS are still rolling out.

Reassigning to evangelism--I've emailed twimbow with a link to this bug (hi twimbow!).

Comment 2

[Jason Duell]

Hmm, I guess Afrikaans isn't the right language for our evangelism here :)

Comment 3

[Knight00931]

Thanks for the reply, so just wait for twimbow.com confirms this and then modify connection behavior in Nightly?

Comment 4

[Patrick McManus [:mcmanus]]

We're not going to change firefox connection behavior.

The whole point of basing the websockets handshake on http is to reuse that code and infrastructure, which is what is going on here... this is a bog standard http request, and if it fails to bootstrap websockets it might still result in a valid HTTP connection for other HTTP purposes.

This is a server side bug - it should be fixed on the server side.

Comment 5

[Patrick McManus [:mcmanus]]

jason - thanks for digging into it!

Comment 6

[Jason Duell]

Twimbow got back to me, and this issue will be fixed on their end soon.

Comment 7

[Jason Duell]

Hmm, this may be an issue generally with all node.js servers.  Reopening while I verify.

Comment 8

[Jason Duell]

It is a node.js issue, and they're on it:

https://github.com/joyent/http-parser/issues/99

Comment 9

[Knight00931]

Sorry to bother you again, I know that I shouldn't reply here.
But the last changes were 2 weeks ago, have them fixed it?Thanks.
https://github.com/joyent/http-parser/pull/100

Comment 10

[Knight00931]

Seems there are no more news about thier http-parser.
sigh.