Closed Bug 640003 Opened 13 years ago Closed 13 years ago

WebSockets - upgrade to ietf-07

Categories

(Core :: Networking: WebSockets, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [secr:dchan])

Attachments

(5 files, 23 obsolete files)

10.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
35.82 KB, patch
mcmanus
: review+
mcmanus
: superreview+
Details | Diff | Splinter Review
160.12 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
6.05 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
235.80 KB, patch
mcmanus
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
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
Assignee: nobody → mcmanus
Blocks: 636960, 547897
Depends on: 542401
Attached patch udpate to websockets ietf-06 .v1 (obsolete) — Splinter Review
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.
I can feedback it once the patch is ready.
Depends on: 640213
Attached patch update to websockets ietf-06 v2 (obsolete) — Splinter Review
splits out the http upgrade stuff, optimizes the read path a little bit, and fixes an interop bug seen with ssl and deflate.
Attachment #517882 - Attachment is obsolete: true
Attachment #518096 - Flags: feedback?(wfernandom2004)
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?
(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.
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.
nsWebSocketHandler should use nsIProxiedProtocolHandler instead of nsIProtocolHandler, shouldn't it?
> 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.
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.
>> 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.
Attachment #518096 - Flags: feedback?(wfernandom2004) → feedback+
Blocks: 616275
Blocks: 610249
Blocks: 574897
No longer blocks: 547897
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.
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.
Attached patch open by proxy v1 (obsolete) — Splinter Review
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.
Attached patch update to websockets ietf-06 v3 (obsolete) — Splinter Review
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.
Attachment #518096 - Attachment is obsolete: true
Depends on: 643291
> 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.
re-enable deflate-stream in pywebsockets server now that it is confirmed correct
Attachment #520297 - Attachment is obsolete: true
Attached patch update to websockets ietf-06 v4 (obsolete) — Splinter Review
fix problem with deflate-stream
Attachment #520305 - Attachment is obsolete: true
Attachment #520528 - Flags: review?(Olli.Pettay)
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.
Attachment #520528 - Flags: review?(jduell.mcbugs)
Attachment #520301 - Flags: review?(jduell.mcbugs)
Attachment #520299 - Flags: review?(Olli.Pettay)
Attachment #520527 - Flags: review?(jduell.mcbugs)
Sorry for the slight delay. I'll try to review this today or probably tomorrow.
Attachment #520299 - Flags: review?(Olli.Pettay) → review+
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)
Attachment #520528 - Flags: review?(Olli.Pettay) → review+
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
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://).
(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.
*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.
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.
So in other words, the time when WebSockets will be enabled depends on the
stability of the API and protocol specifications.
Attached patch open by proxy v2 (obsolete) — Splinter Review
update for bitrot
Attachment #520301 - Attachment is obsolete: true
Attachment #520301 - Flags: review?(jduell.mcbugs)
Attachment #526524 - Flags: review?(jduell.mcbugs)
Attached patch update to websockets ietf-06 v5 (obsolete) — Splinter Review
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.
Attachment #520528 - Attachment is obsolete: true
Attachment #520528 - Flags: review?(jduell.mcbugs)
Attachment #526525 - Flags: review?(jduell.mcbugs)
(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.
Attached patch update to websockets ietf-06 v6 (obsolete) — Splinter Review
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
Attachment #526525 - Attachment is obsolete: true
Attachment #526525 - Flags: review?(jduell.mcbugs)
Attachment #527106 - Flags: review?(jduell.mcbugs)
Attached patch update to websockets ietf-06 v7 (obsolete) — Splinter Review
fix silly problem with yesterday's logging update
Attachment #527106 - Attachment is obsolete: true
Attachment #527106 - Flags: review?(jduell.mcbugs)
Attachment #527295 - Flags: review?(jduell.mcbugs)
Attachment #520527 - Flags: review?(jduell.mcbugs) → review?(cbiesinger)
Attachment #526524 - Flags: review?(jduell.mcbugs) → review?(cbiesinger)
Attachment #527295 - Flags: review?(jduell.mcbugs) → review?(cbiesinger)
Adding doc needed keyword; we will be using this bug to track readiness to document WebSockets going forward.
Keywords: dev-doc-needed
Blocks: 537787
Depends on: 653530
Attached patch update to websockets ietf-07 v1 (obsolete) — Splinter Review
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.
Attachment #527295 - Attachment is obsolete: true
Attachment #527295 - Flags: review?(cbiesinger)
Attachment #529094 - Flags: review?(cbiesinger)
Any update?
(sorry, didn't comment in the right bug)
Attached patch update to websockets ietf-07 v2 (obsolete) — Splinter Review
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.
Attachment #529094 - Attachment is obsolete: true
Attachment #529094 - Flags: review?(cbiesinger)
Attachment #529480 - Flags: review?(cbiesinger)
Depends on: 654197
Attached patch update to websockets ietf-07 v3 (obsolete) — Splinter Review
Fix a problem one of my testers found with subprotocol negotation and the initialization form that takes an array of protocols.
Attachment #529480 - Attachment is obsolete: true
Attachment #529480 - Flags: review?(cbiesinger)
Attachment #529811 - Flags: review?(cbiesinger)
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 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?
Attachment #520527 - Flags: review?(cbiesinger) → review+
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?
Attachment #526524 - Flags: review?(cbiesinger) → review-
> 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.
> > +    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.
Attached patch open by proxy v3 (obsolete) — Splinter Review
update patch based on review comment 42
Attachment #526524 - Attachment is obsolete: true
Attachment #531411 - Flags: superreview?(bzbarsky)
Attachment #531411 - Flags: review?(cbiesinger)
Attached patch update to websockets ietf-07 v4 (obsolete) — Splinter Review
Update this patch due to bitrot in "open by proxy" patch which was updated based on reviewer comments.
Attachment #529811 - Attachment is obsolete: true
Attachment #529811 - Flags: review?(cbiesinger)
Attachment #531418 - Flags: review?(cbiesinger)
Comment on attachment 531411 [details] [diff] [review]
open by proxy v3

Review of attachment 531411 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531411 - Flags: review?(cbiesinger) → review+
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.
Attachment #531411 - Flags: superreview?(bzbarsky) → superreview+
update with sr comments from comment 48

carry forward r=biesi sr=bz
Attachment #531411 - Attachment is obsolete: true
Attachment #532198 - Flags: superreview+
Attachment #532198 - Flags: review+
Attached patch update to websockets ietf-07 v5 (obsolete) — Splinter Review
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.
Attachment #531418 - Attachment is obsolete: true
Attachment #531418 - Flags: review?(cbiesinger)
Attachment #532269 - Flags: review?(cbiesinger)
Attachment #532198 - Attachment description: open by proxy v4 → open by proxy v4 [landed d17947eb66f0]
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
(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.
Attached patch fix incomplete message code v1 (obsolete) — Splinter Review
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.
Attachment #532748 - Flags: review?(cbiesinger)
> @@ +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.
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.
Attachment #520527 - Attachment is obsolete: true
Attachment #533150 - Flags: review?(cbiesinger)
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.
Attachment #533151 - Flags: review?(cbiesinger)
Attached patch unused mowner v1 (obsolete) — Splinter Review
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.
Attachment #533153 - Flags: review?(cbiesinger)
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.
Attachment #533154 - Flags: review?(cbiesinger)
fix a counter that was out of sync with the tests
Attachment #533151 - Attachment is obsolete: true
Attachment #533151 - Flags: review?(cbiesinger)
Attachment #533411 - Flags: review?(cbiesinger)
Attached file large transmit bug v1 (obsolete) —
This is cumulative to the main update to websockets ietf-07 v5.. it is a trivial one line s/=+/+=/ change
Attachment #533412 - Flags: review?(cbiesinger)
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 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.
Attachment #533412 - Flags: review?(cbiesinger) → review+
Attachment #533411 - Flags: review?(cbiesinger) → review+
Attachment #533154 - Flags: review?(cbiesinger) → review+
Attachment #533153 - Flags: review?(cbiesinger) → review+
Attachment #533150 - Flags: review?(cbiesinger) → review+
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()
Attachment #532269 - Flags: review?(cbiesinger) → review-
Depends on: 658546
Attached patch update to websockets ieth-07 v6 (obsolete) — Splinter Review
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.
Attachment #532269 - Attachment is obsolete: true
Attachment #532748 - Attachment is obsolete: true
Attachment #533153 - Attachment is obsolete: true
Attachment #533154 - Attachment is obsolete: true
Attachment #533412 - Attachment is obsolete: true
Attachment #532748 - Flags: review?(cbiesinger)
Attachment #533992 - Flags: superreview?(bzbarsky)
Attachment #533992 - Flags: review?(cbiesinger)
>+    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 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 :)
Attachment #533992 - Flags: review?(cbiesinger) → review+
Attachment #533411 - Attachment is patch: true
> 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.
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.
Attachment #533992 - Attachment is obsolete: true
Attachment #533992 - Flags: superreview?(bzbarsky)
Attachment #534147 - Flags: superreview?(bzbarsky)
Attachment #534147 - Flags: review+
Comment on attachment 534147 [details] [diff] [review]
update to websockets ietf07 v7

r=me
Attachment #534147 - Flags: superreview?(bzbarsky) → superreview+
(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.
(In reply to comment #71)

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

yes, pending tree logistics. soon.
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.
Are you expecting the API to change in a backwards incompatible way?
Also, Chrome has shipped non-prefixed WebSocket for ages (which is *very* 
unfortunate since they shipped also the old protocol)
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).
It sounds like we need a bug on prefixing the JS API, tracking fx6, right?
Depends on: 659324
(In reply to comment #78)
> It sounds like we need a bug on prefixing the JS API, tracking fx6, right?

Yes. Done. bug 659324
Jonas, where has this discussion about changing the API happening?
Target Milestone: --- → mozilla6
Summary: WebSockets - upgrade to ietf-06+ → WebSockets - upgrade to ietf-07
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.
I get this error "ReferenceError: WebSocket is not defined" in Firefox 6.0 in a websocket based page.
(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.
(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?
(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.
(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?
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.
> 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..
(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?
> 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?
(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).
I am able to do the handshake correctly. But the socket gets disconnected thereafter. Will investigate where the problem is.
(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
(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 ).
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.
No issue for me. Work as expected using a WS-IETF-draft-07-complient server.
(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?
(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)
Blocks: 679092
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
"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/
Whiteboard: [sr:dchan] → [secr:dchan]
No longer blocks: 714257
Flags: sec-review+
You need to log in before you can comment on or make changes to this bug.