Last Comment Bug 640003 - WebSockets - upgrade to ietf-07
: WebSockets - upgrade to ietf-07
Status: RESOLVED FIXED
[secr:dchan]
: dev-doc-needed
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: All All
: -- enhancement with 7 votes (vote)
: mozilla6
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
: 648272 (view as bug list)
Depends on: 542401 640213 643291 653530 654197 658546 659324
Blocks: 714325 537787 574897 610249 616275 636960 679092
  Show dependency treegraph
 
Reported: 2011-03-08 14:40 PST by Patrick McManus [:mcmanus]
Modified: 2012-08-15 08:23 PDT (History)
49 users (show)
dveditz: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
udpate to websockets ietf-06 .v1 (238.86 KB, patch)
2011-03-08 14:43 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
update to websockets ietf-06 v2 (218.58 KB, patch)
2011-03-09 10:08 PST, Patrick McManus [:mcmanus]
wfernandom2004: feedback+
Details | Diff | Splinter Review
upgrade to dev snapshot of pywebsockets (134.59 KB, patch)
2011-03-18 13:47 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
update websocket mochitests v1 (10.76 KB, patch)
2011-03-18 13:53 PDT, Patrick McManus [:mcmanus]
bugs: review+
Details | Diff | Splinter Review
open by proxy v1 (39.03 KB, patch)
2011-03-18 13:56 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
update to websockets ietf-06 v3 (218.31 KB, patch)
2011-03-18 14:03 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
upgrade to dev snapshot of pywebsockets v2 (134.58 KB, patch)
2011-03-20 08:55 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review
update to websockets ietf-06 v4 (218.80 KB, patch)
2011-03-20 08:56 PDT, Patrick McManus [:mcmanus]
bugs: review+
Details | Diff | Splinter Review
open by proxy v2 (38.98 KB, patch)
2011-04-16 15:17 PDT, Patrick McManus [:mcmanus]
cbiesinger: review-
Details | Diff | Splinter Review
update to websockets ietf-06 v5 (221.12 KB, patch)
2011-04-16 15:21 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
update to websockets ietf-06 v6 (224.15 KB, patch)
2011-04-19 14:26 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
update to websockets ietf-06 v7 (224.05 KB, patch)
2011-04-20 09:22 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
update to websockets ietf-07 v1 (230.72 KB, patch)
2011-04-29 07:38 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
update to websockets ietf-07 v2 (233.26 KB, patch)
2011-05-02 08:12 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
update to websockets ietf-07 v3 (233.61 KB, patch)
2011-05-03 13:04 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
open by proxy v3 (35.14 KB, patch)
2011-05-10 12:26 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
update to websockets ietf-07 v4 (233.61 KB, patch)
2011-05-10 12:29 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
open by proxy v4 [landed d17947eb66f0] (35.82 KB, patch)
2011-05-13 06:53 PDT, Patrick McManus [:mcmanus]
mcmanus: review+
mcmanus: superreview+
Details | Diff | Splinter Review
update to websockets ietf-07 v5 (233.56 KB, patch)
2011-05-13 10:15 PDT, Patrick McManus [:mcmanus]
cbiesinger: review-
Details | Diff | Splinter Review
fix incomplete message code v1 (9.10 KB, patch)
2011-05-16 14:52 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
upgrade snapshot of pywebsockets (-07) v3 (160.12 KB, patch)
2011-05-17 19:29 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review
updates websocket tests for pywebsockets -07 (5.32 KB, patch)
2011-05-17 19:33 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
unused mowner v1 (1.14 KB, patch)
2011-05-17 19:36 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review
release listener on main thread v1 (1.21 KB, patch)
2011-05-17 19:40 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review
updates websocket tests for pywebsockets -07 v2 (6.05 KB, patch)
2011-05-18 14:04 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review
large transmit bug v1 (968 bytes, text/plain)
2011-05-18 14:06 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details
update to websockets ieth-07 v6 (235.23 KB, patch)
2011-05-20 09:07 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review
update to websockets ietf07 v7 (235.80 KB, patch)
2011-05-20 16:20 PDT, Patrick McManus [:mcmanus]
mcmanus: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2011-03-08 14:40:40 PST
It looks like we will be able to coordinate and ship the websocket protocol based on either ietf -07 or -08. I expect those to be similar to the current revision, -06: 
http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-06

I have updated the gecko implementation to -06 and will attach it here as a WIP patch. We cannot check it in quite yet, as it breaks the websocket mochitests
because we use pywebsocket as the mochitest backend and that is still at version -04. 

My code has been tested against libwebsockets and I'm working on jetty now. pywebsocket will probably be updated soon (there is -06 related activity in the code.google.com repo as of this week) so it is probably best to wait for that rather than spedning time changing the test infrastructure over to a non python back end. I believe microsoft will have a prototype server available soon as well at the >= 06 leve.

As for the patch itself - it does a few major things:

* update from -76 to ietf-06.. with its framing, masking, fragmentation, etc..

* a configurable ping/pong timer to keep nat's alive and bound the discovery time of unreachable peers

* implementation of the compression extension

* the monolithic content/websocket implementation is broken into three parts
 * content (in content)
 * the networking protocol (in netwerk/websocket)
 * the http based handshake (in network/http)

* The -06 handshake is now http compliant (-76 famously just kind of looked like http), and is now implemented using an nsIHttpChannel. This allows us to remove all kinds of code, get a more flexible implementation (deal with redirects, strange http parsing rules, etc..), use the http connection manager, and so on. 

It also means I had to teach the HTTP stack about HTTP upgrade. There is a new idl for that wherein you basically get a OnTransportAvailable() callback with the low level socket and streams for your new protocol once the HTTP Upgrade transaction has yielded a 101 - up until that point the connection belongs to the HTTP stack.

It should fix 636960 (logging problem) and 547897 (CORS cookie problem).

An interactive message protocol like this absolutely must have nagle disabled - so it depends on 542401
Comment 1 Patrick McManus [:mcmanus] 2011-03-08 14:43:24 PST
Created attachment 517882 [details] [diff] [review]
udpate to websockets ietf-06 .v1
Comment 2 Olli Pettay [:smaug] 2011-03-09 03:27:40 PST
I'd be happy to review the content/ part once the patch is ready.

Could you perhaps split the patch somehow?
For example maybe nsIHttpUpgradeChannel could be implemented in a different bug 
and then this bug could depend on that one? That might make reviewing easier.
Comment 3 Wellington Fernando de Macedo 2011-03-09 06:28:16 PST
I can feedback it once the patch is ready.
Comment 4 Patrick McManus [:mcmanus] 2011-03-09 10:08:03 PST
Created attachment 518096 [details] [diff] [review]
update to websockets ietf-06 v2

splits out the http upgrade stuff, optimizes the read path a little bit, and fixes an interop bug seen with ssl and deflate.
Comment 5 Wellington Fernando de Macedo 2011-03-12 10:17:02 PST
I still need to read this new version of the protocol. One question, does this one fix that security issue that disabled the -76 version?
Comment 6 Patrick McManus [:mcmanus] 2011-03-12 13:07:58 PST
(In reply to comment #5)
> I still need to read this new version of the protocol. One question, does this
> one fix that security issue that disabled the -76 version?

yes. -06 (and -05 and iirc -04) xor all the bytes of the message payloads from the client towards the server using a key randomly selected for, and included with, each message. That means the byte stream is undpredictable, which satisfies the secuirty concern, but doesn't make anything secret.
Comment 7 Wellington Fernando de Macedo 2011-03-12 13:11:31 PST
I've just finished to read the spec.

I need some more time. But I already have some comments.

I've noticed you are using an HTTP channel to establish the connection, and then you take the socket transport to do things. However HTTP and WS have some incompatibilities that your code doesn't handle:
1: When there is a proxy and the connection isn't secure http uses the GET method. However WS uses the CONNECT one. 
2: I don't understand why you are implementing the nsIChannelEventSink interface since WS doesn't support redirection.
3: The HTTP proxy resolution order is (HTTPS || HTTP), SOCKS. However, the WS one is: SOCKS, HTTPS and HTTP. The original code solves that here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsWebSocket.cpp#2245
4: Using one http channel you are sending unnecessary headers like Authorization.

Also, perhaps now you can merge nsWebSocketEstablishedConnection into the nsWebSocket class.
Comment 8 Wellington Fernando de Macedo 2011-03-12 13:18:57 PST
nsWebSocketHandler should use nsIProxiedProtocolHandler instead of nsIProtocolHandler, shouldn't it?
Comment 9 Wellington Fernando de Macedo 2011-03-12 13:52:17 PST
> 1: When there is a proxy and the connection isn't secure http uses the GET
> method. However WS uses the CONNECT one. 
Also, here, just sending CONNECT isn't enough. If a http proxy is used and it requires a digest authentication then your channel needs to change nsIHttpAuthenticableChannel::GetProxyMethodIsConnect to return true. See: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpDigestAuth.cpp#121

Tomorrow I'll continue.
Comment 10 Patrick McManus [:mcmanus] 2011-03-12 19:18:25 PST
Wellington, thanks.

(In reply to comment #7)
> I've just finished to read the spec.
> 
> I need some more time. But I already have some comments.
> 
> I've noticed you are using an HTTP channel to establish the connection, and
> then you take the socket transport to do things. However HTTP and WS have some
> incompatibilities that your code doesn't handle:

things have changed on this front - until the 101 and the response associated with it is received the transaction *is* HTTP. There are no incompatibilities. If there are - that's a bug in the spec and we should bring it to the WG, but the WG is firm that this is all HTTP until it is all framed as WS and HTTP is no longer relevant. That is a clean HTTP bootstrapped upgrade.

> 2: I don't understand why you are implementing the nsIChannelEventSink
> interface since WS doesn't support redirection.

See above - a http redirect of the handshake is absolutely a supported use case and something I have tested.

I appreciate the notes on proxies, I'll review and get back with questions as necessary.

> 4: Using one http channel you are sending unnecessary headers like
> Authorization.
> 

Please elaborate - I'm not sure in specific what you're speaking about here.
Comment 11 Wellington Fernando de Macedo 2011-03-13 14:01:05 PDT
>> 4: Using one http channel you are sending unnecessary headers like
>> Authorization.
> Please elaborate - I'm not sure in specific what you're speaking about here.
I meant, for instance, suppose one has opened www.domain.com and has basic credentials set in its current session; if a WS connection is opened to www.domain.com then that credentials are sent together with the WS handshake (by the Authorization header).

> things have changed on this front - until the 101 and the response associated with it is received the transaction *is* HTTP.
Ok. So my 4th commentary wasn't an issue.

feedback=+ with the notes on proxies.
Comment 12 Patrick McManus [:mcmanus] 2011-03-18 13:47:41 PDT
Created attachment 520297 [details] [diff] [review]
upgrade to dev snapshot of pywebsockets

code from http://pywebsocket.googlecode.com/svn/branches/hybi-06 as of march 17 2011 .. updates old -76 version of same code

a few very minor local changes applied (noted in this patch)

we have a deflate stream interop problem with this code (which I believe to be in pywebsocket, but am not certain) - so that extension is disabled in the server.
Comment 13 Patrick McManus [:mcmanus] 2011-03-18 13:53:09 PDT
Created attachment 520299 [details] [diff] [review]
update websocket mochitests v1

Some bits of our mochitests had hardcoded -76 bits in them.. where I could I removed the version dependency, and everywhere I updated them to -06 compatibility. This also removes the setting of the "security override" pref in the tests as the -06 implementation drops that.
Comment 14 Patrick McManus [:mcmanus] 2011-03-18 13:56:49 PDT
Created attachment 520301 [details] [diff] [review]
open by proxy v1

As Wellington notes in comment 7, websockets has somewhat different rules regarding proxies than does the http code - and yet the http code should be used to bootstrap the websocket connection.

This patch implements a new ioservice interface that more or less lets you open any channel (in this case an http channel) but still supply the flags and a separate uri for doing the proxy resolution.

I broke it out of the main websockets patch for reviewing convenience.
Comment 15 Patrick McManus [:mcmanus] 2011-03-18 14:03:02 PDT
Created attachment 520305 [details] [diff] [review]
update to websockets ietf-06 v3

changes from -02
* an open timer of 20 seconds (prefable)
* dont generate error events in js unless in OPEN state
* use the new proxy interface from open by proxy v1 patch
* remove some of the test changes - moved to the "update websocket mochitests" patch for tidyness.
* some other syntactic dressups

this is all based on testing with libwebsockets and pywebsockets.. jetty and the ms offering are still on tap.
Comment 16 Patrick McManus [:mcmanus] 2011-03-20 08:53:55 PDT
> we have a deflate stream interop problem with this code (which I believe to be
> in pywebsocket, but am not certain) - so that extension is disabled in the
> server.

The interop bug was in necko (and also in libwebsockets which we had interoped with), not pywebsocket. I have repaired that defect and re-enabled the deflate testing with pywebsocket in the mochitests.

I have now shown interop with libwebsockets (modulo the compress issue), ms wcf, the pywebsockets -06 dev branch, and jetty. I also have a positive report about APE. Those seem to be the major players right now.
Comment 17 Patrick McManus [:mcmanus] 2011-03-20 08:55:37 PDT
Created attachment 520527 [details] [diff] [review]
upgrade to dev snapshot of pywebsockets v2

re-enable deflate-stream in pywebsockets server now that it is confirmed correct
Comment 18 Patrick McManus [:mcmanus] 2011-03-20 08:56:32 PDT
Created attachment 520528 [details] [diff] [review]
update to websockets ietf-06 v4

fix problem with deflate-stream
Comment 19 Patrick McManus [:mcmanus] 2011-03-20 08:58:54 PDT
Comment on attachment 520528 [details] [diff] [review]
update to websockets ietf-06 v4

I picked Olli and Jason to review the content and network portions of this.. feel free to reassign as apropos. I have a hard time figuring out who to choose sometimes.
Comment 20 Olli Pettay [:smaug] 2011-03-23 12:52:51 PDT
Sorry for the slight delay. I'll try to review this today or probably tomorrow.
Comment 21 Olli Pettay [:smaug] 2011-03-24 11:14:16 PDT
Comment on attachment 520528 [details] [diff] [review]
update to websockets ietf-06 v4

>+NS_IMPL_THREADSAFE_ISUPPORTS3(nsWebSocketEstablishedConnection,
>+                               nsIInterfaceRequestor,
>+                               nsIWebSocketListener,
>+                               nsIRequest)
Nit, align interfaces under nsWebSocketEstablishedConnection


>+nsWebSocketEstablishedConnection::OnStop(nsISupports *aContext,
>+                                         nsresult aStatusCode)
> {
>-  if (mHeaders[kProxyAuthenticatePos].IsEmpty()) {
>-    return NS_ERROR_NOT_AVAILABLE;
>+  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>+  if (!mOwner)
>+    return NS_OK;
>+  
>+  if (NS_FAILED(aStatusCode)) {
>+    ConsoleError();
>+    if (mOwner && mOwner->mReadyState != nsIWebSocket::CONNECTING) {
>+      nsresult rv =
>+        mOwner->CreateAndDispatchSimpleEvent(NS_LITERAL_STRING("error"));
>+      if (NS_FAILED(rv))
>+        NS_WARNING("Failed to dispatch the error event");
>+    }
>   }
>-  value.Assign(mHeaders[kProxyAuthenticatePos]);
>+
>+  mClosedCleanly = NS_SUCCEEDED(aStatusCode);
>+  mStatus = CONN_CLOSED;
>+  mOwner->SetReadyState(nsIWebSocket::CLOSED);
I think nothing guarantees that mOwner is not null here.
error event listener may have called something which nulls out
mOwner.



>-  NS_ASSERTION((aNewReadyState == nsIWebSocket::OPEN) ||
>+  NS_ABORT_IF_FALSE((aNewReadyState == nsIWebSocket::OPEN) ||
>                (aNewReadyState == nsIWebSocket::CLOSING) ||
>                (aNewReadyState == nsIWebSocket::CLOSED),
>                "unexpected readyState");
Nit, align expressions somehow



> 
>   if (aNewReadyState == nsIWebSocket::OPEN) {
>-    NS_ASSERTION(mReadyState == nsIWebSocket::CONNECTING,
>+    NS_ABORT_IF_FALSE(mReadyState == nsIWebSocket::CONNECTING,
>                  "unexpected readyState transition");
ditto

>   if (aNewReadyState == nsIWebSocket::CLOSING) {
>-    NS_ASSERTION((mReadyState == nsIWebSocket::CONNECTING) ||
>+    NS_ABORT_IF_FALSE((mReadyState == nsIWebSocket::CONNECTING) ||
>                  (mReadyState == nsIWebSocket::OPEN),
>                  "unexpected readyState transition");
And here


>+  static void ReleaseGlobals() {}
What is this?

>   nsCString mOrigin;
>+  nsString  mUTF16Origin;
>+  
Do we use both so often that we need
to have the same data in 2 formats?


content/ part is quite a bit easier to review, since it is mainly just code removal.
r=me, but I'd like to re-review an updated patch. (just because of the size of the patch)
I assume jduell will review the whole necko part (which means also reviewing the protocol)
Comment 22 Patrick McManus [:mcmanus] 2011-04-06 08:08:43 PDT
the whatwg editor's draft of websockets changed slightly last week after a very long period of stability. Close() takes some new parameters to support receipt of data carried in the IETF-06 protocol (reason code and message). There is now also a new tracking bug about what code to use when the protocol isn't what drives the close (e.g. nav away)

http://www.w3.org/Bugs/Public/show_bug.cgi?id=12433
Comment 23 Cork 2011-04-07 09:28:14 PDT
*** Bug 648272 has been marked as a duplicate of this bug. ***
Comment 24 Joel Martin 2011-04-07 09:38:11 PDT
Does the restructuring make this issue go away:
https://bugzilla.mozilla.org/show_bug.cgi?id=594502 (ability to manually accept
self-signed certs when using wss://).
Comment 25 Patrick McManus [:mcmanus] 2011-04-07 09:45:09 PDT
(In reply to comment #24)
> Does the restructuring make this issue go away:
> https://bugzilla.mozilla.org/show_bug.cgi?id=594502 (ability to manually accept
> self-signed certs when using wss://).

it should, as the handshake uses the existing nshttpchannel implementation - but I will test to confirm.
Comment 26 Mathieu Pellerin 2011-04-12 20:36:47 PDT
*crossing fingers* Is the plan to re-implement the websockets in time for Firefox 5? I bunch of us are currently writing a web app surrounding this real-time communciation feature, would really appreciate a sense of when the devs feel this will reach the mass of firefox users.
Comment 27 Olli Pettay [:smaug] 2011-04-12 20:45:33 PDT
Since the patches haven't landed yet, WebSockets won't be enabled for
Fx5. If the protocol becomes stable enough
(Patrick guessed -07 or -08 in comment 0) soon, we could hope to
get this all done for Fx6.
Comment 28 Olli Pettay [:smaug] 2011-04-12 20:49:13 PDT
So in other words, the time when WebSockets will be enabled depends on the
stability of the API and protocol specifications.
Comment 29 Patrick McManus [:mcmanus] 2011-04-16 15:17:41 PDT
Created attachment 526524 [details] [diff] [review]
open by proxy v2

update for bitrot
Comment 30 Patrick McManus [:mcmanus] 2011-04-16 15:21:03 PDT
Created attachment 526525 [details] [diff] [review]
update to websockets ietf-06 v5

Add a pref for following http redirects during handshake (default off) as discussed during all-hands necko meeting with jonas.. also update for bitrot.

Carrying forward an r+ from Olli on the content bits as those have not changed in this update.
Comment 31 Patrick McManus [:mcmanus] 2011-04-16 15:28:38 PDT
(In reply to comment #24)
> Does the restructuring make this issue go away:
> https://bugzilla.mozilla.org/show_bug.cgi?id=594502 (ability to manually accept
> self-signed certs when using wss://).

Sorry it took me a week to follow up on this.

Unfortunately that situation will remain the same - if you already have accepted a cert from a previous transaction (presumably an html base page) the wss:// will be fine, but as I believe with any other non base page, you don't have the option of accepting an invalid cert if this is the first time it comes up. The same situation happens for instance with the inclusion of an image over an https:// connection that you don't have a trust entry for - there is no way to process the exception.
Comment 32 Patrick McManus [:mcmanus] 2011-04-19 14:26:57 PDT
Created attachment 527106 [details] [diff] [review]
update to websockets ietf-06 v6

Updates since v5:

* bulked up logging
* a memory leak on a timeout error path
* make sure nsIURIs are always proxy released to the main thread
Comment 33 Patrick McManus [:mcmanus] 2011-04-20 09:22:12 PDT
Created attachment 527295 [details] [diff] [review]
update to websockets ietf-06 v7

fix silly problem with yesterday's logging update
Comment 34 Eric Shepherd [:sheppy] 2011-04-22 11:55:27 PDT
Adding doc needed keyword; we will be using this bug to track readiness to document WebSockets going forward.
Comment 35 Patrick McManus [:mcmanus] 2011-04-29 07:38:53 PDT
Created attachment 529094 [details] [diff] [review]
update to websockets ietf-07 v1

Update the patch to implement ietf-07

This draft in IETF WG Last Call.

Changes at specification level including the location of the masking key, the introduction of a masking bit and the possibility of receiving masked frames in the client, total rearrangement of opcodes..

Other changes in the code include better support for binary frames through the idl even though there is no JS API for them yet and significant commenting and readability enhancements.

As I write this - this patch breaks our mochitests because the pywebsocket server we have embeded for those tests does not yet implement -07. I suspect it will do so relatively soon, so there is no point in routing around it at this time. I've tried to validate the changes using WebSockets-Node and libwebsockets to do some manual testing.
Comment 36 Paul Rouget [:paul] 2011-04-29 08:37:52 PDT
Any update?
Comment 37 Paul Rouget [:paul] 2011-04-29 10:44:37 PDT
(sorry, didn't comment in the right bug)
Comment 38 Patrick McManus [:mcmanus] 2011-05-02 08:12:03 PDT
Created attachment 529480 [details] [diff] [review]
update to websockets ietf-07 v2

The original websockets idl is missing the negotiated protocol as a property, as per http://dev.w3.org/html5/websockets/#dom-websocket-protocol. Patch updated to expose that.
Comment 39 Patrick McManus [:mcmanus] 2011-05-03 13:04:56 PDT
Created attachment 529811 [details] [diff] [review]
update to websockets ietf-07 v3

Fix a problem one of my testers found with subprotocol negotation and the initialization form that takes an array of protocols.
Comment 40 Joel Martin 2011-05-04 11:00:32 PDT
I have tested builds from Patrick with the new protocol rev with noVNC and websockify (https://github.com/kanaka/noVNC, https://github.com/kanaka/websockify) and it appears to be working well.

One thing I should note though is that the latency of WebSockets in firefox continues to be quite a bit higher (more than 10X) than in Chrome 10.

In websockify I have a latency test (tests/latency.html and test/latency.py). The test page sends the server frames containing a timestamp, sequence number, and random data of the given size. The server decodes the frame and sends the payload right back. The client then checks the sequence number (to make sure no frames get out of order) and calculates a timestamp delta, and then sends a new frame.

I'm getting the following latency results (min/mean/max in milliseconds) after sending about 1500 messages:

firefox 4.0.1 (native Hixie-76 manually enabled):
       4 byte payload - 6.0/10.5/47.0 ms
    2000 byte payload - 6.0/10.9/72.0 ms

firefox 4.0.1 (Flash web-socket-js polyfill/shim Hixie-76):
       4 byte payload - 2.0/11.6/68.0 ms
    2000 byte payload - 5.0/14.4/71.0 ms

firefox 4.0.1 (Flash web-socket-js polyfill/shim HyBi-07):
       4 byte payload - 1.0/14.1/52.0 ms
    2000 byte payload - 6.0/18.0/75.0 ms

firefox 6.0a1 (HyBi-07):
       4 byte payload - 6.0/9.6/27.0 ms
    2000 byte payload - 7.0/29.6/63.0 ms

Chrome 10 (Hixie-76):
       4 byte payload - 0.0/0.6/5.0 ms
    2000 byte payload - 1.0/1.8/25.0 ms

The web-socket-js with Hixie-76 support is here (and included with websockify): https://github.com/gimite/web-socket-js

I hacked a of web-socket-js to add HyBi-07 support here: https://github.com/kanaka/web-socket-js

Note that for larger frames, my hacked web-socket-js with HyBi-07 is getting better latency results than the 6.0a1 build and that's including that Javascript to Flash ExternalInterface overhead in the measure.
Comment 41 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-06 14:20:28 PDT
Comment on attachment 520527 [details] [diff] [review]
upgrade to dev snapshot of pywebsockets v2

Review of attachment 520527 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/pywebsocket/README
@@ +1,5 @@
> +This is a development version of mod_pywebsocket 0.6, from http://code.google.com/p/pywebsocket/
> +
> +It has the following patches installed against the subversion repo fo
> + 1] follow symlinks to wsh files for mochitest
> + 2] a bug where hybi06 does not call do_extra_handshake

This patch might be useful upstream too?

@@ +4,5 @@
> + 1] follow symlinks to wsh files for mochitest
> + 2] a bug where hybi06 does not call do_extra_handshake
> + 3] set the ws_protocol field to the requested subprotocol as
> +    mochitest expects
> + 4] disable deflate stream as it seems broken. perhaps it uses rfc1950

Did you ask the upstream mailing list about this?
Comment 42 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-06 14:34:32 PDT
Comment on attachment 526524 [details] [diff] [review]
open by proxy v2

Review of attachment 526524 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsIIOService3.idl
@@ +38,5 @@
> +
> +#include "nsIIOService2.idl"
> +
> +/**
> + * nsIIOService2 extends nsIIOService with support for automatic

This comment seems wrong. But why bother with a new interface anyway? Just change nsIIOService2 (which should now get merged into nsIIOService)

@@ +55,5 @@
> +     * @param aProxyFlags flags from nsIProtocolProxyService to use
> +     *        when resolving proxies for this new channel
> +     * @return reference to the new nsIChannel object
> +     */
> +    nsIChannel newChannelFromURIWithProxyFlags(in nsIURI aURI,

I'm not really convinced that this method is useful enough to expose it here. Can't callers query the proxy service themselves and then get the protocol handler and call newProxiedChannel directly? Or is there actually more than one caller for it?

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +101,5 @@
>      return clone;
>  }
> +
> +PRBool
> +nsHttpConnectionInfo::ForceConnect()

Hmm, this function name misled me into thinking that it did a forceful connect, whereas in reality, it is asking whether to force connect. Maybe name it ShouldForceConnectMethod?

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +230,5 @@
>  
>      // make sure we eliminate any proxy specific headers from 
>      // the request if we are talking HTTPS via a SSL tunnel.
> +    PRBool pruneProxyHeaders = 
> +        mConnInfo->ForceConnect() ||

Please also use cinfo here so that it's obvious that this line is checking the same object as the next one.

It almost seems like conninfo should have a method for checking whether to use connect, combining the ALWAYS_CONNECT check with the SSL check. What do you think?
Comment 43 Patrick McManus [:mcmanus] 2011-05-06 16:32:47 PDT
> This patch might be useful upstream too?

I've talked with takeshi and the changes should be (mostly) upstream when we see -07 on the server.. the compression issues are sorted out too.
Comment 44 Patrick McManus [:mcmanus] 2011-05-10 11:48:56 PDT
> > +    nsIChannel newChannelFromURIWithProxyFlags(in nsIURI aURI,
> 
> I'm not really convinced that this method is useful enough to expose it
> here. Can't callers query the proxy service themselves and then get the
> protocol handler and call newProxiedChannel directly? Or is there actually
> more than one caller for it?

honestly I find that a bit more awkward - ioservice::newChannelFromURI() already seems to be the preferred way to set things up and that includes finding a proxy if its relevant. This change just lets the caller set some details about the proxy selection that is already going on instead of assuming that the flags ought to be 0.

Websockets will need it because it specfies some peculiar rules about proxy selection. That doesn't seem out of place for other upgraded protocols to want the same thing.
Comment 45 Patrick McManus [:mcmanus] 2011-05-10 12:26:38 PDT
Created attachment 531411 [details] [diff] [review]
open by proxy v3

update patch based on review comment 42
Comment 46 Patrick McManus [:mcmanus] 2011-05-10 12:29:32 PDT
Created attachment 531418 [details] [diff] [review]
update to websockets ietf-07 v4

Update this patch due to bitrot in "open by proxy" patch which was updated based on reviewer comments.
Comment 47 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-10 13:33:15 PDT
Comment on attachment 531411 [details] [diff] [review]
open by proxy v3

Review of attachment 531411 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 48 Boris Zbarsky [:bz] 2011-05-12 19:17:24 PDT
Comment on attachment 531411 [details] [diff] [review]
open by proxy v3

Please fix the overall comment describing what nIIOService2 interface is about (e.g. by deleting all the text starting with "with support".

There's a missing blank line after the new constant in nsIProtocolProxyService.idl.

You need to change the iid of nsIProxyInfo.

sr=me with those nits fixed.
Comment 49 Patrick McManus [:mcmanus] 2011-05-13 06:53:07 PDT
Created attachment 532198 [details] [diff] [review]
open by proxy v4 [landed d17947eb66f0]

update with sr comments from comment 48

carry forward r=biesi sr=bz
Comment 50 Patrick McManus [:mcmanus] 2011-05-13 10:15:38 PDT
Created attachment 532269 [details] [diff] [review]
update to websockets ietf-07 v5

small change to update bitrot against sr induced changes in 640213 interface. If you've started reviewing v4 of this I would just continue and then check the small interdiff.
Comment 51 Patrick McManus [:mcmanus] 2011-05-13 10:59:30 PDT
open by proxy changes landed as http://hg.mozilla.org/mozilla-central/rev/d17947eb66f0
Comment 52 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-13 18:11:54 PDT
Comment on attachment 532269 [details] [diff] [review]
update to websockets ietf-07 v5

Review of attachment 532269 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsWebSocket.cpp
@@ +298,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCString utf8Origin = NS_ConvertUTF16toUTF8(mOwner->mUTF16Origin);

NS_ConvertUTF16toUTF8 utf8Origin(mOwner->mUTF16Origin);

@@ +359,5 @@
>  }
>  
> +nsresult
> +nsWebSocketEstablishedConnection::Close()
> +// when this is called the browser side wants no more aprt of it

aprt->part

move the comment above the nsresult line please

@@ +481,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsWebSocketEstablishedConnection::
> +OnBinaryMessageAvailable(nsISupports *aContext,

I prefer linewrapping like:
nsWebSocketEstablishedConnection::OnBinaryMessageAvailable(
    nsISupports *aContext,
    const nsACString & aMsg,
...

@@ +989,5 @@
>      return NS_ERROR_DOM_SYNTAX_ERR;
>    }
>  
> +  ToLowerCase(origin);
> +  mUTF16Origin = NS_ConvertUTF8toUTF16(origin);

CopyUTF8toUTF16(origin, mUTF16Origin);

@@ +1166,5 @@
>  
>  NS_IMETHODIMP
> +nsWebSocket::GetProtocol(nsAString& aProtocol)
> +{
> +  aProtocol = NS_ConvertUTF8toUTF16(mProtocol);

CopyUTF8toUTF16(mProtocol, aProtocol);

@@ +1354,2 @@
>  {
> +  *aValue = !!(mOwner);

I find the !! notation kinda unreadable and prefer mOwner != nsnull, but this is not my module

@@ +1363,4 @@
>    return NS_OK;
>  }
>  
> +// probably means window went away

or that the user pressed the stop button

::: modules/libpref/src/init/all.js
@@ +797,5 @@
> +// event is sent to the javascript websockets application
> +pref("network.websocket.timeout.ping.response", 10);
> +
> +// Defines whether or not to try and negotiate the stream-deflate compression
> +// extesnion with the websocket server

extesnion -> extension

::: netwerk/build/nsNetModule.cpp
@@ +282,5 @@
>  #endif
>  
> +#ifdef NECKO_PROTOCOL_websocket
> +#include "nsWebSocketHandler.h"
> +namespace mozilla { namespace net {

As mentioned in another review, please put those two on separate lines

::: netwerk/protocol/websocket/nsIWebSocketProtocol.idl
@@ +70,5 @@
> +     * @param aContext user defined context
> +     * @param aMsg the message data
> +     */
> +    void onMessageAvailable(in nsISupports aContext,
> +                            in ACString aMsg);

ACString -> AUTF8String, right?

@@ +107,5 @@
> +[scriptable, uuid(dc01db59-a513-4c90-824b-085cce06c0aa)]
> +interface nsIWebSocketProtocol : nsISupports
> +{
> +    /**
> +     * The original URI used to construct the channel. This is used in

channel -> protocol?

Is this attribute useful for websockets?

@@ +141,5 @@
> +     */
> +    attribute ACString protocol;
> +
> +    /**
> +     * Asynchronously open this channel.  Data is fed to the specified stream

stream -> socket (several times in this comment)

@@ +186,5 @@
> +     * Use to send text message down the channel to WebSocket peer.
> +     *
> +     * @param aMsg the utf8 string to send
> +     */
> +    void sendMsg(in ACString aMsg);

ACString -> AUTF8String

::: netwerk/protocol/websocket/nsWebSocketHandler.cpp
@@ +73,5 @@
> +#include "zlib.h"
> +
> +extern PRThread *gSocketThread;
> +
> +namespace mozilla { namespace net {

two lines

@@ +95,5 @@
> +#define LOG(args) PR_LOG(webSocketLog, PR_LOG_DEBUG, args)
> +
> +// Use this fake ptr so the Fin message stays in sequence in the
> +// main transmit queue
> +#define kFinMessage ((nsCString *)0x01)

use a reinterpret_cast please

@@ +292,5 @@
> +    
> +    ~nsWSAdmissionManager()
> +    {
> +        for (PRUint32 i = 0; i < mData.Length(); i++)
> +            delete (nsOpenConn *)mData[i];

use C++-style casts, but why do you need a cast at all here?

@@ +295,5 @@
> +        for (PRUint32 i = 0; i < mData.Length(); i++)
> +            delete (nsOpenConn *)mData[i];
> +    }
> +
> +    PRBool conditionallyConnect(nsCString &aStr, nsWebSocketHandler *ws)

uppercase start of method names in C++, please

@@ +322,5 @@
> +        NS_ABORT_IF_FALSE(NS_IsMainThread(), "not main thread");
> +        PRInt32 index = IndexOf(aStr);
> +        NS_ABORT_IF_FALSE(index >= 0, "completed connection not in open list");
> +        
> +        nsOpenConn *olddata = (nsOpenConn *) mData[index];

same cast question

@@ +329,5 @@
> +        
> +        // are there more of the same address pending dispatch?
> +        index = IndexOf(aStr);
> +        if (index >= 0) {
> +            ((nsOpenConn *)mData[index])->mHandler->BeginOpen();

again

@@ +341,5 @@
> +
> +    PRInt32 IndexOf(nsCString &aStr)
> +    {
> +        for (PRUint32 i = 0; i < mData.Length(); i++)
> +            if (aStr == ((nsOpenConn *)mData[i])->mAddress)

again

@@ +347,5 @@
> +        return -1;
> +    }
> +};
> +
> +// similar to nsDeflateCoverter except for the mandatory FLUSH calls

Coverter -> Converter

@@ +446,5 @@
> +    nsresult PushData()
> +    {
> +        PRUint32 bytesToWrite = kBufferLen - mZlib.avail_out;
> +        if (bytesToWrite > 0) {
> +            mStream->ShareData((char*)mBuffer, bytesToWrite);

C++ cast, please

@@ +511,5 @@
> +    
> +    if (!sWebSocketAdmissions)
> +        sWebSocketAdmissions = new nsWSAdmissionManager();
> +
> +    mFramePtr = mBuffer = (PRUint8 *)moz_xmalloc(mBufferSize);

C++ cast

@@ +574,5 @@
> +
> +    nsCOMPtr<nsIChannel> localChannel = do_QueryInterface(mChannel, &rv);
> +    if (NS_FAILED(rv)) {
> +        LOG(("WebSocketHandler::BeginOpen cannot async open\n"));
> +        AbortSession(NS_ERROR_CONNECTION_REFUSED);

I think you want to return after this?

@@ +580,5 @@
> +
> +    rv = localChannel->AsyncOpen(this, mHttpChannel);
> +    if (NS_FAILED(rv)) {
> +        LOG(("WebSocketHandler::BeginOpen cannot async open\n"));
> +        AbortSession(NS_ERROR_CONNECTION_REFUSED);

same

@@ +592,5 @@
> +    return rv;
> +}
> +
> +void
> +nsWebSocketHandler::UpdateReadBuffer(PRUint8 *buffer, PRUint32 count)

My apologies, I ran out of time halfway through this file :( I'll do my best to continue tomorrow
Comment 53 :Ms2ger (⌚ UTC+1/+2) 2011-05-14 10:18:18 PDT
(In reply to comment #51)
> open by proxy changes landed as
> http://hg.mozilla.org/mozilla-central/rev/d17947eb66f0

Awful commit message.

(In reply to comment #52)
> ::: content/base/src/nsWebSocket.cpp
> @@ +1354,2 @@
> >  {
> > +  *aValue = !!(mOwner);
> 
> I find the !! notation kinda unreadable and prefer mOwner != nsnull, but
> this is not my module

FWIW, the Style Guide doesn't want you to compare pointers to nsnull. The parens are unnecessary, though.
Comment 54 Patrick McManus [:mcmanus] 2011-05-16 14:52:46 PDT
Created attachment 532748 [details] [diff] [review]
fix incomplete message code v1

This patch depends on the "update to webssockets ietf-07" patch and fixes a bug in there dealing with messages that span multiple read calls (particularly fragments). 

Thanks to michael @ http://websocketstest.com/ for finding it.

biesi, Normally I would just integrate it into the base patch, but I know you are part way through the review so I didn't want to churn that on you - so I have left it as a cumulative patch. When we update based on review comments I will integrate them together.
Comment 55 Patrick McManus [:mcmanus] 2011-05-17 09:10:27 PDT
> @@ +107,5 @@
> > +[scriptable, uuid(dc01db59-a513-4c90-824b-085cce06c0aa)]
> > +interface nsIWebSocketProtocol : nsISupports
> > +{
> > +    /**
> > +     * The original URI used to construct the channel. This is used in
> 

> Is this attribute useful for websockets?

yes, websockets have the potential to be redirected at the HTTP ugprade stage. Right now we control that via a pref (and it is disabled), but it is probably something that will have a mechanism to opt into.. so I think having that information through the interface is useful.

I have updated the other comments in that IDL to remove the obsolete references to redirects - if they are pref'd on they are in implementation issue, not one that the user of the interface needs to do anything about by handing onredirect events for example.


> > +    nsCOMPtr<nsIChannel> localChannel = do_QueryInterface(mChannel, &rv);
> > +    if (NS_FAILED(rv)) {
> > +        LOG(("WebSocketHandler::BeginOpen cannot async open\n"));
> > +        AbortSession(NS_ERROR_CONNECTION_REFUSED);
> 
> I think you want to return after this?
> [..]
> same
> 

yes, thank you! good catch.
Comment 56 Patrick McManus [:mcmanus] 2011-05-17 19:29:13 PDT
Created attachment 533150 [details] [diff] [review]
upgrade snapshot of pywebsockets (-07) v3

Google, especially Takeshi Yoshino , has updated pywebsockets to support ietf -07. That means we can use our existing mochi tests. This patch brings in the pywebsockets patch - removing most of the local patches we had used for -06.
Comment 57 Patrick McManus [:mcmanus] 2011-05-17 19:33:03 PDT
Created attachment 533151 [details] [diff] [review]
updates websocket tests for pywebsockets -07

updating pywebsockets to -07 means our backend tests need to deal with a new subprotcol selection api. 

this patch also removes a couple override-security-block pref references in the same set of tests which has now been removed.
Comment 58 Patrick McManus [:mcmanus] 2011-05-17 19:36:34 PDT
Created attachment 533153 [details] [diff] [review]
unused mowner v1

This is a trivial patch to remove an unused member variable - it is cumulative to "update to websockets ietf-07 v5", I just didn't want to roll it in there and interrupt an in-progress code review.
Comment 59 Patrick McManus [:mcmanus] 2011-05-17 19:40:59 PDT
Created attachment 533154 [details] [diff] [review]
release listener on main thread v1

It is possible for the nswebsockethandler to drop the last reference to the content nsWebSocket - I have seen this (rarely) in the mochitests. This simple change proxy releases that onto the main thread where it is safe to delete.

this code is cumulative to "update to websockets ietf-07 v5" - but is being posted separately because of the review in progress on that patch. Pending reviews on all the extras (incomplete messages, unused mowner, and this one) as well as the base patch I will combine them when applying the review comments.
Comment 60 Patrick McManus [:mcmanus] 2011-05-18 14:04:47 PDT
Created attachment 533411 [details] [diff] [review]
updates websocket tests for pywebsockets -07 v2

fix a counter that was out of sync with the tests
Comment 61 Patrick McManus [:mcmanus] 2011-05-18 14:06:37 PDT
Created attachment 533412 [details]
large transmit bug v1

This is cumulative to the main update to websockets ietf-07 v5.. it is a trivial one line s/=+/+=/ change
Comment 62 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-18 17:54:33 PDT
Comment on attachment 531418 [details] [diff] [review]
update to websockets ietf-07 v4

Review of attachment 531418 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/nsWebSocketHandler.cpp
@@ +681,5 @@
> +            if (avail < framingLength)
> +                break;
> +            // copy this in case it is unaligned
> +            PRUint16 tempLen;
> +            memcpy (&tempLen, mFramePtr + 2, 2);

I'd do mFramePtr[2] << 8 | mFramePtr[3] instead of the memcpy (with unsigned casts as necessary)

@@ +820,5 @@
> +                mServerClosed = 1;
> +                
> +                if (payloadLength >= 2) {
> +                    PRUint16 code;
> +                    memcpy (&code, payload, 2);

No space before ( please

@@ +1031,5 @@
> +        payload = mOutHeader + 6;
> +        
> +        // The close reason code sits in the first 2 bytes of payload
> +        if (NS_SUCCEEDED(mStopOnClose))
> +            *((PRUint16 *)payload) = PR_htons(1000); /* ok */

These numbers really should be constants

@@ +1104,5 @@
> +        }
> +        payload = mOutHeader + mHdrOutToSend;
> +    }
> +    
> +    NS_ABORT_IF_FALSE(payload, "payload offset not found");

note to self: continue reviewing here
Comment 63 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-18 18:39:14 PDT
Comment on attachment 532269 [details] [diff] [review]
update to websockets ietf-07 v5

Review of attachment 532269 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/nsWebSocketHandler.cpp
@@ +1186,5 @@
> +{
> +    LOG(("WebSocketHandler::StopSession() %p [%x]\n", this, reason));
> +
> +    // normally this should be called on socket thread, but it is ok to call it
> +    // from OnnStartRequest before the socket thread machine has gotten underway

Onn -> On

@@ +1220,5 @@
> +        PRUint32 total = 0;
> +        nsresult rv;
> +        do {
> +            total += count;
> +            rv = mSocketIn->Read((char *)buffer, 512, &count);

I'd just make buffer a char[] and get rid of the cast

@@ +1235,5 @@
> +
> +    if (mTransport) {
> +        mTransport->SetSecurityCallbacks(nsnull);
> +        mTransport->SetEventSink(nsnull, nsnull);
> +        mTransport->Close(NS_OK);

Closing with a success status is unusual... Maybe use NS_BASE_STREAM_CLOSED instead? That's what socket transport does anyway when given a success code.

@@ +1432,5 @@
> +    secKeyString.Assign(b64);
> +    PR_Free(b64);
> +    mHttpChannel->SetRequestHeader(
> +        NS_LITERAL_CSTRING("Sec-WebSocket-Key"), secKeyString, PR_FALSE);
> +    LOG(("WebSocketHandler::AsyncOpen() client key %s\n", secKeyString.get()));

my laptop is running out of battery and I have no charger with me :( today is not a good day for me.

I'll continue once I find power and time, but that may be tomorrow.
Comment 64 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-19 17:58:47 PDT
Comment on attachment 532269 [details] [diff] [review]
update to websockets ietf-07 v5

Review of attachment 532269 [details] [diff] [review]:
-----------------------------------------------------------------

All done!

::: netwerk/protocol/websocket/nsWebSocketHandler.cpp
@@ +1458,5 @@
> +nsWebSocketHandler::ApplyForAdmission()
> +{
> +    LOG(("WebSocketHandler::ApplyForAdmission() %p\n", this));
> +
> +    // Websockets has a policy of 1 session at atime being allowed in the

atime -> a time

@@ +1528,5 @@
> +    LOG(("WebSocketHandler::GetInterface() %p\n", this));
> +
> +    if (iid.Equals(NS_GET_IID(nsIChannelEventSink))) {
> +        QueryInterface(iid, result);
> +        return NS_OK;

You really should make this return QueryInterface(...);

@@ +1544,5 @@
> +nsWebSocketHandler::AsyncOnChannelRedirect(nsIChannel *oldChannel,
> +                                           nsIChannel *newChannel,
> +                                           PRUint32 flags,
> +                                           nsIAsyncVerifyRedirectCallback
> +                                           *callback)

I prefer this formatting:
nsWebSocketHandler::AsyncOnChannelRedirect(
    nsIChannel *oldChannel,
    nsIChannel *newChannel,
    PRUint32 flags,
    nsIAsyncVerifyRedirectCallback* callback)

@@ +1861,5 @@
> +
> +    nsCOMPtr<nsIURI> localURI;
> +    nsCOMPtr<nsIChannel> localChannel;
> +    
> +    localURI = new nsStandardURL();

This line is not necessary, given the next line, right?

@@ +1869,5 @@
> +    else
> +        rv = localURI->SetScheme(NS_LITERAL_CSTRING("http"));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    
> +    nsCOMPtr<nsIIOService>  ioService;

Just one space, please

@@ +1949,5 @@
> +    return mSocketThread->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);
> +}
> +
> +NS_IMETHODIMP
> +nsWebSocketHandler::SendBinaryMsg(const nsACString &aMsg, PRInt32 dataLen)

Why require people to pass dataLen when that can be gotten from the string itself?

@@ +2135,5 @@
> +            else {
> +                LOG(("WebsocketHandler::OnStartRequest "
> +                     "subprotocol [%s] not found - %s returned",
> +                     mProtocol.get(), respProtocol.get()));
> +                mProtocol = NS_LITERAL_CSTRING("");

mProtocol.Truncate()
Comment 65 Patrick McManus [:mcmanus] 2011-05-20 09:07:53 PDT
Created attachment 533992 [details] [diff] [review]
update to websockets ieth-07 v6

Biesi - Thanks for all the reviews. This is a big patch!

This version of the patch incorporates your four review comment blocks, plus it rolls in the fixes you have seen that were other attachments to this bug while you were reviewing the base.

the only other change is in nsWebSocketEstablishedConnection::OnStop() - where I no longer generate an error event just because a connection is closed uncleanly. (that's what the unclean parameter to the close event is for).

Boris - I think the sr just needs to look primarily at the nsIWebSocketProtocol interface, which is what is used to shuffle data between content and network, basically. the nsIWebSocket.idl has been updated to just include the protocol attribute that is defined by the w3c document but had been omitted to this point for reasons unknown to me.

Thanks! I think we can get this landed for ff6.
Comment 66 Boris Zbarsky [:bz] 2011-05-20 12:30:24 PDT
>+    void onBinaryMessageAvailable(in nsISupports aContext,
>+                                  in ACString aMsg,
>+                                  in long dataLen);

Why is that third argument needed?  aMsg already knows its own length.  Please take that argument out.

For an nsIWebSocketListener, please document that if onStart() is called onStop() will be as well.

The current comments on nsIWebSocketListener make me think that onStop() will be called in all cases but onServerClose will _sometimes_ be called after onStop()?  How is the listener supposed to know when it's "done" and can clean up state, if there is no reliable "no more from me after this" notification?  Should onStop() come after onServerClose()?  Or does "no additional messages" mean no more calls to onMessageAvailable() and onBinaryMessageAvailable() but posible calls to onStop() and onAcknowledge()?  This whole part is very confusing...  I suspect the issue is just one of documentation, and the real behavior is that onStop() comes last, but please fix the documentation to make thing clear?

>+     * thought additional data may still be sent.

s/thought/though/

> Attempts to
>+     * set it to null must throw.

originalURI is readonly.  So there's no way to even try setting it.  Take that sentence out or clearly explain what it actually means?

>+     * The URI corresponding to the protocol.  Its value is immutable.

In the "this is a readonly property" sense or in the "The uri this hands out implements nsIMutable and is immutable" sense?  Or something else?

>+     * If asyncOpen returns successfully, the channel is responsible for

s/channel/protocol/ or something?  Same elsewhere in this documentation.

Is there a reason this thing is a "protocol" as opposed to a "connection"?

I assume onStop() can come after close() or before if the server closed?  Worth documenting clearly.
Comment 67 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-20 12:32:21 PDT
Comment on attachment 533992 [details] [diff] [review]
update to websockets ieth-07 v6

netwerk/protocol/websocket/nsWebSocketHandler.cpp
1600	    nsIAsyncVerifyRedirectCallback
1601	    *callback)

please merge these two lines now :)
Comment 68 Patrick McManus [:mcmanus] 2011-05-20 16:14:39 PDT
> I suspect the issue
> is just one of documentation, and the real behavior is that onStop() comes
> last, but please fix the documentation to make thing clear?

you're right  - the onStop() is last. I have tried to clear it up in the forthcoming patch update.
Comment 69 Patrick McManus [:mcmanus] 2011-05-20 16:20:52 PDT
Created attachment 534147 [details] [diff] [review]
update to websockets ietf07 v7

Udpates
 * for biesi comment 67
 * for bz comment 66
 * code to ensure onAcknowledge doesn't get called after onStop
 * code to through ALREADY_OPENED in AsyncOpen() as the doc said it would!

Also, new news: microsoft labs updated their server foundation classes to match our protocol level - and we performed a basic interop test fine.
Comment 70 Boris Zbarsky [:bz] 2011-05-20 18:08:59 PDT
Comment on attachment 534147 [details] [diff] [review]
update to websockets ietf07 v7

r=me
Comment 71 Robert Sayre 2011-05-21 11:22:04 PDT
(In reply to comment #69)
> Also, new news: microsoft labs updated their server foundation classes to
> match our protocol level - and we performed a basic interop test fine.

Excellent. Is this ready to land? I see all the patches are reviewed.
Comment 72 Patrick McManus [:mcmanus] 2011-05-21 11:35:41 PDT
(In reply to comment #71)

> Excellent. Is this ready to land? I see all the patches are reviewed.

yes, pending tree logistics. soon.
Comment 74 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-23 15:49:12 PDT
We really *really* should prefix the JS API here. There are a lot of questions that the API spec editor still hasn't responded to, some of which would change the API in pretty big ways.
Comment 75 Olli Pettay [:smaug] 2011-05-24 02:22:19 PDT
Are you expecting the API to change in a backwards incompatible way?
Comment 76 Olli Pettay [:smaug] 2011-05-24 02:24:44 PDT
Also, Chrome has shipped non-prefixed WebSocket for ages (which is *very* 
unfortunate since they shipped also the old protocol)
Comment 77 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-24 02:26:46 PDT
Yes. For example it's likely that we'll remove the arguments to the constructor and instead add a .open method which takes a URI. That model could work better both with redirects as well as allowing the consumer to chose format for binary data (ArrayBuffer vs. Blob).
Comment 78 Boris Zbarsky [:bz] 2011-05-24 07:50:19 PDT
It sounds like we need a bug on prefixing the JS API, tracking fx6, right?
Comment 79 Asa Dotzler [:asa] 2011-05-24 08:03:28 PDT
(In reply to comment #78)
> It sounds like we need a bug on prefixing the JS API, tracking fx6, right?

Yes. Done. bug 659324
Comment 80 Olli Pettay [:smaug] 2011-05-24 12:14:59 PDT
Jonas, where has this discussion about changing the API happening?
Comment 81 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-24 15:35:03 PDT
At least part of it happens here:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12102
Comment 82 Eric Shepherd [:sheppy] 2011-07-11 10:57:29 PDT
The docs for WebSockets are roughly finished, except that we don't yet have the infrastructure in place for hosting live demos for people to try.
Comment 83 Lenin Raj Rajasekaran 2011-07-22 00:19:48 PDT
I get this error "ReferenceError: WebSocket is not defined" in Firefox 6.0 in a websocket based page.
Comment 84 Patrick McManus [:mcmanus] 2011-07-22 05:15:36 PDT
(In reply to comment #83)
> I get this error "ReferenceError: WebSocket is not defined" in Firefox 6.0
> in a websocket based page.

FF6(beta) has the websockets JS API prefixed beacuse the API is in flux.

Use MozWebSocket instead or
 
if (MozWebSocket)
   WebSocket = MozWebSocket;
 
The prefix has not been applied to FF7(aurora) yet, but I anticipate that it will be.
Comment 85 Lenin Raj Rajasekaran 2011-07-22 08:33:50 PDT
(In reply to comment #84)
> (In reply to comment #83)
> > I get this error "ReferenceError: WebSocket is not defined" in Firefox 6.0
> > in a websocket based page.
> 
> FF6(beta) has the websockets JS API prefixed beacuse the API is in flux.
> 
> Use MozWebSocket instead or
>  
> if (MozWebSocket)
>    WebSocket = MozWebSocket;
>  
> The prefix has not been applied to FF7(aurora) yet, but I anticipate that it
> will be.

Thanks. Now I'm using FF7(Aurora) this client code (http://phpwebsocket.googlecode.com/svn/trunk/%20phpwebsocket/client.html)
I always get "Disconnected - status 3" while in Google Chrome(12.0.742.122), I get "Welcome - status 1"

Why is it that I am not able to connect from Firefox?
Comment 86 Patrick McManus [:mcmanus] 2011-07-22 08:39:35 PDT
(In reply to comment #85)
> (In reply to comment #84)
> > (In reply to comment #83)
> > > I get this error "ReferenceError: WebSocket is not defined" in Firefox 6.0
> > > in a websocket based page.
> > 
> > FF6(beta) has the websockets JS API prefixed beacuse the API is in flux.
> > 
> > Use MozWebSocket instead or
> >  
> > if (MozWebSocket)
> >    WebSocket = MozWebSocket;
> >  
> > The prefix has not been applied to FF7(aurora) yet, but I anticipate that it
> > will be.
> 
> Thanks. Now I'm using FF7(Aurora) this client code
> (http://phpwebsocket.googlecode.com/svn/trunk/%20phpwebsocket/client.html)
> I always get "Disconnected - status 3" while in Google Chrome(12.0.742.122),
> I get "Welcome - status 1"
> 
> Why is it that I am not able to connect from Firefox?

What version of websockets does your localhost server implement? FF7(aurora) and FF8(nightly) require version 8.
Comment 87 Lenin Raj Rajasekaran 2011-07-22 09:04:20 PDT
(In reply to comment #86)
> (In reply to comment #85)
> > (In reply to comment #84)
> > > (In reply to comment #83)
> > > > I get this error "ReferenceError: WebSocket is not defined" in Firefox 6.0
> > > > in a websocket based page.
> > > 
> > > FF6(beta) has the websockets JS API prefixed beacuse the API is in flux.
> > > 
> > > Use MozWebSocket instead or
> > >  
> > > if (MozWebSocket)
> > >    WebSocket = MozWebSocket;
> > >  
> > > The prefix has not been applied to FF7(aurora) yet, but I anticipate that it
> > > will be.
> > 
> > Thanks. Now I'm using FF7(Aurora) this client code
> > (http://phpwebsocket.googlecode.com/svn/trunk/%20phpwebsocket/client.html)
> > I always get "Disconnected - status 3" while in Google Chrome(12.0.742.122),
> > I get "Welcome - status 1"
> > 
> > Why is it that I am not able to connect from Firefox?
> 
> What version of websockets does your localhost server implement? FF7(aurora)
> and FF8(nightly) require version 8.

Not sure about the websocket version. But the protocol is "draft 76".
From this link(http://code.google.com/p/phpwebsocket/source/browse/trunk/%20phpwebsocket/server.php), I see the change log as "update for new version of WebSocket. protocol - draft 76. no backward compatibility with draft 75".

Is this information okay or how can I determine the websocket version being implemented in my server?
Comment 88 Boris Zbarsky [:bz] 2011-07-22 09:16:35 PDT
This bug was about updating from protocol draft 76 to a newer version that doesn't have the security problems 76 does.  Chrome still implements the old insecure protocol, apparently.
Comment 89 Patrick McManus [:mcmanus] 2011-07-22 09:23:39 PDT
> Not sure about the websocket version. But the protocol is "draft 76".

Right, we don't support that old protocol version. It has security problems, among other things.

(76 is older than draft-0 through draft-10.. a weirdness in the handoff between standards committees at some point).

We intend to support one protocol version when unprefixed, and that's probably going to be 8. That's defined by draft's 8, 9, and 10. (more or less an implementation of any of them will interop - the all use the same protocol number).

It's true FF6(beta) will support version 7, but that's part of the reason the prefix is still there. Without the prefix, we'll keep it as stable as can be done.

As far as servers - 

Websocket-Node (node.js) is pretty cool. I believe Brian has implementations for both version 8 and version 7 available.

pywebsockets (python) is up to version 7 (and backwards compatible support for everyting < 7) and I've seen activity in its bugbase about doing the version 8 implementation.

libwebsockets (c) is up to version 7 and has backwards compat for < 7. I don't know about their plans.

there are surely others..
Comment 90 Lenin Raj Rajasekaran 2011-07-22 19:22:25 PDT
(In reply to comment #89)

> We intend to support one protocol version when unprefixed, and that's
> probably going to be 8. That's defined by draft's 8, 9, and 10. (more or
> less an implementation of any of them will interop - the all use the same
> protocol number).
> 
> It's true FF6(beta) will support version 7, but that's part of the reason
> the prefix is still there. Without the prefix, we'll keep it as stable as
> can be done.

I am updating my server code for make it supported in Aurora(FF7). I see that the latest version of websocket is draft-10 (http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-10). From this page (https://developer.mozilla.org/en/WebSockets), I understand that the draft-07 is the one that is supported.
Should I target 07 or 10?
Comment 91 Patrick McManus [:mcmanus] 2011-07-22 19:43:54 PDT
> Should I target 07 or 10?

firefox 7(aurora), firefox 8(nightly) and hopefully future versions support draft-10 (which is protocol version 8 if you look at the contents of the draft 10 document, the same protocol version supoprted by drafts 9 and the shortlived draft 8).

Which server do you author?
Comment 92 Lenin Raj Rajasekaran 2011-07-22 19:50:56 PDT
(In reply to comment #91)

> Which server do you author?

In PHP/Apache. (code.google.com/p/phpwebsocket/). Will try to update tonight my time(IST).
Comment 93 Lenin Raj Rajasekaran 2011-07-23 04:58:53 PDT
I am able to do the handshake correctly. But the socket gets disconnected thereafter. Will investigate where the problem is.
Comment 94 Andrés G. Aragoneses 2011-08-15 08:04:33 PDT
(In reply to Lenin Raj Rajasekaran from comment #93)
> I am able to do the handshake correctly. But the socket gets disconnected
> thereafter. Will investigate where the problem is.

I'm having the same problem. Already sniffed traffic but it seems to be FF6 closing the connection. How can I debug Firefox to know what is it expecting from the server? Already tried NSPR_LOG_MODULES=all:4 but I don't get any useful info from that.

TIA
Comment 95 Andrés G. Aragoneses 2011-08-15 08:54:16 PDT
(In reply to Andrés G. Aragoneses from comment #94)
> Already tried NSPR_LOG_MODULES=all:4 but I don't get any
> useful info from that.

As adviced on #developers and #devrel, I got a debug build from ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-release-linux-debug/1313099199/ and run with NSPR_LOG_MODULES=nsHttp:4,nsWebSocket:4 , but still no useful info to know why FF is closing the connection ( http://pastebin.com/6Fc6mbb2 ).
Comment 96 Patrick McManus [:mcmanus] 2011-08-15 11:44:22 PDT
http://pastebin.com/6Fc6mbb2 isn't the entire log... can you attach it to a new bug (this bug is closed) please and cc: me?

Handshake processing would involve lines near "WebSocketHandler::OnStartRequest" and/or "WebSocketHandler::OnTransportAvailable".

please also include in the new bug, if you can, a url I can try and connect to.
Comment 97 Anthony Catel [:paraboul] 2011-08-15 12:15:29 PDT
No issue for me. Work as expected using a WS-IETF-draft-07-complient server.
Comment 98 Lenin Raj Rajasekaran 2011-08-15 12:19:43 PDT
(In reply to Anthony Catel [:paraboul] from comment #97)
> No issue for me. Work as expected using a WS-IETF-draft-07-complient server.

What language your server side code is using? Can you share it if it is PHP?
Comment 99 Anthony Catel [:paraboul] 2011-08-15 13:03:04 PDT
(In reply to Lenin Raj Rajasekaran from comment #98)
> (In reply to Anthony Catel [:paraboul] from comment #97)
> > No issue for me. Work as expected using a WS-IETF-draft-07-complient server.
> 
> What language your server side code is using? Can you share it if it is PHP?

It's C (APE server : http://www.ape-project.org/ | Sources : https://github.com/APE-Project/APE_Server/tree/master/src)
Comment 100 David Chan [:dchan] 2011-08-16 15:09:33 PDT
The implementation review of WebSockets draft 7 for Fx6 was completed. There was nothing else of note outside of the reviews already conducted by bz, mcmanus and smaug
Comment 101 mexuk 2011-08-30 13:39:47 PDT
"What language your server side code is using? Can you share it if it is PHP?"

Check this:
http://code.google.com/p/php-websocket-server/

Note You need to log in before you can comment on or make changes to this bug.