User-Agent: Mozilla/5.0 (X11; U; Linux i686; pt-BR; rv:1.9.0.5) Gecko/2008121621 Ubuntu/8.04 (hardy) Firefox/3.0.5 Build Identifier: Web sockets enable Web applications to maintain bidirectional communications with server-side processes. Reproducible: Always Steps to Reproduce: 1. 2. 3.

WebSockets API may merge with or replace http://dev.w3.org/2006/webapi/network-api/network-api.html.

It's looking like it'll replace it. However there's also discussions about taking the protocol part of this to IETF which means that it might get updated, so if anyone is considering implementing it please keep this in mind.

Hi, I intend to implement this. I talked to shepazu today. He told me when the websockets' spec move to IETF and to w3c he would email me about it.

It is my initial work. There are some changes in nsDOMClassInfo.cpp and nsXHREventTarget I've done that I don't know if they are good.

Comment on attachment 358882 [details] [diff] [review] initial work >+ { &nsGkAtoms::onopen, { NS_OPEN, EventNameType_All }}, >+ { &nsGkAtoms::onmessage, { NS_MESSAGE, EventNameType_All }}, >+ { &nsGkAtoms::onclosed, { NS_CLOSED, EventNameType_All }}, Why EventNameType_All? Could EventNameType_None be ok? >+static already_AddRefed<nsIDocument> >+GetDocumentFromScriptContext(nsIScriptContext *aScriptContext) nsXMLHttpRequest.cpp has something similar? Perhaps this could be a helper method in nsContentUtils >+#define NS_WEBSOCKET_IMPL_DOMEVENTLISTENER(_eventlistenername, _eventlistener) \ >+ NS_IMETHODIMP \ >+ nsWebSocket::GetOn##_eventlistenername(nsIDOMEventListener * *aEventListener)\ >+ { \ >+ NS_ENSURE_ARG_POINTER(aEventListener); \ >+ if (_eventlistener) \ >+ NS_ADDREF(*aEventListener = _eventlistener); \ >+ else \ >+ *aEventListener = nsnull; \ I think this could be NS_IF_ADDREF(*aEventListener = _eventlistener); >+ //also, it has no default action >+ nsCOMPtr<nsIPrivateDOMEvent> privEvent(do_QueryInterface(event, &rv)); >+ NS_ENSURE_SUCCESS(rv, NS_OK); >+ nsEvent *internalEvent = privEvent->GetInternalNSEvent(); >+ NS_ENSURE_TRUE(internalEvent, NS_OK); >+ internalEvent->flags |= NS_EVENT_FLAG_NO_DEFAULT; Is this really needed? If the event doesn't have default action, it just doesn't have one. No need to basically call .preventDefault() >+ nsCOMPtr<nsIStandardURL> stardardURL(do_CreateInstance( >+ NS_STANDARDURL_CONTRACTID, &rv)); You use rv but don't actually check its value. >@@ -1098,17 +1100,17 @@ static nsDOMClassInfoData sClassInfoData ... > NS_DEFINE_CLASSINFO_DATA(SVGNumberList, nsDOMGenericSH, >- DOM_DEFAULT_SCRIPTABLE_FLAGS) >+ DOM_DEFAULT_SCRIPTABLE_FLAGS) No need for this change >@@ -6073,17 +6093,17 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp ... > // Resolving a standard class won't do any evil, and it's possible > // for caps to get the answer wrong, so disable the security check > // for this case. >- >+ No need for this change >@@ -6930,17 +6950,17 @@ nsNodeSH::PreCreate(nsISupports *nativeO ... > NS_ASSERTION(node->IsNodeOfType(nsINode::eCONTENT) || > node->IsNodeOfType(nsINode::eATTRIBUTE), > "Unexpected node type"); >- >+ ditto. >@@ -7048,28 +7068,28 @@ nsNodeSH::GetProperty(nsIXPConnectWrappe ... > nsCOMPtr<nsIXPConnectJSObjectHolder> holder; > nsresult rv = WrapNative(cx, obj, uri, &NS_GET_IID(nsIURI), vp, > getter_AddRefs(holder)); > return NS_FAILED(rv) ? rv : NS_SUCCESS_I_DID_SOMETHING; >- } >+ } And here > nsresult rv = WrapNative(cx, obj, node->NodePrincipal(), > &NS_GET_IID(nsIPrincipal), vp, > getter_AddRefs(holder)); > return NS_FAILED(rv) ? rv : NS_SUCCESS_I_DID_SOMETHING; >- } >+ } And here >@@ -8346,17 +8378,17 @@ nsHTMLDocumentSH::DocumentOpen(JSContext ... > contentType = "text/plain"; > } > } >- >+ And here > NS_IMETHODIMP > nsHTMLOptionsCollectionSH::NewResolve(nsIXPConnectWrappedNative *wrapper, >- JSContext *cx, JSObject *obj, >- jsval id, PRUint32 flags, >+ JSContext *cx, JSObject *obj, >+ jsval id, PRUint32 flags, > JSObject **objp, PRBool *_retval) And here >- >- *objp = obj; >- >+ >+ *objp = obj; >+ And here > rv = sXPConnect->GetWrappedNativeOfJSObject(cx, JSVAL_TO_OBJECT(argv[0]), > getter_AddRefs(wrapper)); >- >+ And here > if (NS_FAILED(rv)) { > nsDOMClassInfo::ThrowJSException(cx, rv); > return JS_FALSE; > } >- >+ And here > nsCOMPtr<nsIDOMNode> beforeNode; > options->Item(index, getter_AddRefs(beforeNode)); >- >+ and here > nsCOMPtr<nsIDOMHTMLSelectElement> select; > nsoptions->GetSelect(getter_AddRefs(select)); >- >+ and here. The patch would be significantly smaller without those whitespace changes ;) But looks like a great WIP. Somehow combining XHR's and WebSocket's event handling would be great, but could be done in a followup bug.

This is up-to-date patch. I will try to upload once a week my work in order to you can look at it, do comments, etc. Wellington.

this has the work I've done this week.

Hi, This patch is already working. I've uploading a video in youtube demonstrating it, using img-tag+data-uri: http://www.youtube.com/watch?v=CrV9jJQn_MA however... there are things to do yet: * authentication request headers * proxy * ssl * security checks * an static global Disconnect method * handling set-cookie and set-cookie2 headers * resource management (max-connections, bfcache, something else?)

here are my test files. Wellington.

The spec is now split between: The protocol spec http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol The API spec http://dev.w3.org/html5/websockets/ Please don't hesitate to let me know if you have any questions about the spec, either directly or on the whatwg@whatwg.org list. Thanks! I can't wait for this to get into the nightlies!

This patch implements all the current spec (except the set-cookie2 header, which it seems isn't implemented in the code). Talking to Ian some question has been raised: * Should the spec handle 407 and 401 auth responses codes? Like I've told him, IMHO I think full HTTP handling (as for normal http) for these codes should be nice, because, as it is, the protocol is so http oriented. * How about when freezing (bfcache)? This patch doesn't handle it. Also, Ian has asked me if this would be in nightlies. I don't know what to tell him... Please, could somebody answer him this? Wellington.

Yes, once the patch is reviewed and landed this would be included in nightlies.

smaug wrote: >> diff --git a/netwerk/base/src/nsProtocolProxyService.cpp b/netwerk/base/src >> /nsProtocolProxyService.cpp >> --- a/netwerk/base/src/nsProtocolProxyService.cpp >> +++ b/netwerk/base/src/nsProtocolProxyService.cpp >> Especially all the changes to netwerk needs to be reviewed by someone else >> than me. >> Maybe biesi or bz. biesi, could you review these changes in netwerk, please? thanks.

Jason, could you perhaps look at the networking part?

fix missing parenthesis in two conditions in a netwerk file.

About Apache. I've done some tests with cgi and normal php scripts and I couldn't do Apache working with the WebSocket protocol. In this zip there are logs, tcpdumps, the tests codes, and what I've noticed(below) during the tests. normal php scripts: 1. Apache always sends the Date and Server response headers right after the HTTP line, so the protocol needs be changed or doesn't work 2. The script never gets the postMessage messages, neither reading stdin nor through php vars. The messages are simply ignored. Changing the request method to POST doesn't work, either. 3. The PHP output, however, is sent correctly. cgi: 1. Apache always reply with HTTP 200 OK... 2. Apache always sends the Date and Server response headers right after the HTTP line, like when handling php 3. The script never gets the postMessage messages reading stdin. The messages are simply ignored. Changing the request method to POST doesn't work, either. 4. The output, however, is sent correctly.

I don't know enough about the web sockets protocol to know if the issues you're having with apache are problems with apache, or your web sockets impl. Are you planning to make the code work w/apache? If no, do we have some other test strategy? This is a big patch, and I'd like to see it in stable state & with some tests before I wade in.

I did a search on apache.org and got *zero* hits for 'websocket'. So, this begs the question: Does Apache support websockets? Because, if it doesn't, which seems to be the case, then that would be the obvious reason why you're not able to use websockets to communicate with Apache.

It would be nice if some web server could support this impl. So I've done these tests in order to find if Apache would work with websocket. I think the problems are with Apache, because the impl seems to follow the spec. Well, in the zip there are the logs and tcpdumps of my tests. Using them I think it is possible to know if Apache is not working or this impl is wrong :)

(In reply to comment #20) > If no, do we have some other test > strategy? This is a big patch, and I'd like to see it in stable state & with > some tests before I wade in. Unfortunately I don't have another strategy. I think if this protocol is going to become a standard soon there should be web servers supporting it. However there aren't any web servers that implement this protocol yet. So what we can do is implementing standalone apps (running like servers) intended to testing.

It should be possible to spin up another mock server like httpd.js to do this, assuming the protocol's not HTTP-like enough for httpd.js to support it without too much wrangling.

(In reply to comment #24) > It should be possible to spin up another mock server like httpd.js to do this, > assuming the protocol's not HTTP-like enough for httpd.js to support it without > too much wrangling. ok, I will try to do this.

Nice work! So, WebSockets support becomes native in Firefox 3.5? if not, when can we expect the patch to merge with the main dev. tree?

(In reply to comment #26) > Nice work! Indeed! > So, WebSockets support becomes native in Firefox 3.5? No > if not, when can we > expect the patch to merge with the main dev. tree? Once the patch has reviews (I can review non-networking parts) and it has been sorted out that the protocol is easily implementable and usable also with Apache.

(In reply to comment #24) > It should be possible to spin up another mock server like httpd.js to do this, > assuming the protocol's not HTTP-like enough for httpd.js to support it without > too much wrangling. this patch makes httpd.js supporting websocket. Although Apache doesn't support the protocol, I hope this could be useful...

Comment on attachment 380454 [details] [diff] [review] mochitest working I'll need to look at the httpd.js changes...

Comment on attachment 380454 [details] [diff] [review] mochitest working First, the changes you make to httpd.js are a bit of a use-specific hack to allow processing of the websocket request as it's received. This particular mechanism is not specific to web sockets; some HTTP request handlers would also be interested in being able to read requests whose bodies are streamed like this. I'm not a fan of adding websocket-specific methods to HTTP-specific interfaces when they wouldn't be necessary if the server had a little more (generic) chops. Second, I've taken a closer look at the spec, and I'm not quite sure yet that we're best served here by implementing websocket tests within httpd.js, rather than within a custom server. I'm still trying to wrap my head around the spec and what it requires. I might be wrong about this, but I don't know yet. In any case the first note still applies, so r- for now. I filed bug 495877 on implementing incremental processing more generically and marked it as blocking this bug for now.

Please note that the websocket protocol is not HTTP. It sort of looks like HTTP at first, but after that it's no more related to HTTP than FTP, so it's no surprise that httpd.js or apache can't handle it. It might make sense to add websocket support to httpd.js if we think of it as a generic testing server, rather than a http server. Similarly I'd hope and expect someone to develop a module to apache which adds websocket support, but only because people that want websocket are likely to have apache running, since they are both web protocols.

Well, a conforming client's request is a subset of HTTP's syntax. The "header" section is perfectly acceptable HTTP, and handling the non-header portion really just requires the ability to read some data and respond with data conditioned on it, repeatedly. The problem with httpd.js is that there's currently no way to do the "repeatedly" part -- but we can fix that, and I think that would address the needs here. The question, then, is whether more-permissive HTTP parsing (allowing HT instead of SP as a delimiter, accepting multiline headers, may be others as well) of the "header" from the client, by the server, is acceptable or not, and I don't know the answer to that yet. Also, it's not clear yet whether "header" parsing may apply the HTTP field-name/field-value syntax restrictions -- the current spec might allow random null bytes or whatever so long as each line after the first contained ": " and ended with CRLF.

>> It might make sense to add websocket support to httpd.js if we >> think of it as a generic testing server, rather than a http server. Yes, this was my idea. I just tried make changes to allow websocket testing (without changing HTTP requests behavior in httpd.js). Well, actually, I think a specific websocket socket would be better... perhaps it wasn't a good idea changing httpd.js :) Wellington.

Wellington, Another thing we will want in order to take this patch is to have a preference (i.e. an about:config variable) that will enable/disable the whole websockets API, so we can pref it off it needed. Perhaps you have this already--I haven't looked.

(In reply to comment #34) > Wellington, > > Another thing we will want in order to take this patch is to have a preference > (i.e. an about:config variable) that will enable/disable the whole websockets > API, so we can pref it off it needed. Perhaps you have this already--I > haven't looked. Ok. So, I will remove the httpd.js changes, add this pref and take a look in the spec again (because I don't know if it has been changed since this last patch). Next week I'll have an updated patch.

I'm hacking on bug 495877 in my spare time and will probably have a patch sometime within the next week or so, which will be enough to put some real tests of this in the patch without resorting to non-generalizable httpd.js hacks, without having to spin up an entirely different handler for it. On the other hand, on further thought httpd.js will not be enough because what's really desired here is a strict server which *only* accepts inputs that a conformant client will send. If there's a bug client-side which causes it to send bad data that still parses as HTTP, httpd.js will never be able to detect that, so such a bug would be untestable -- hence the need for a server which strictly verifies its inputs.

(In reply to comment #35) > Ok. So, I will remove the httpd.js changes, add this pref and take a look in > the spec again (because I don't know if it has been changed since this last > patch). Next week I'll have an updated patch. Here it is the patch. Wellington.

(In reply to comment #15) > Created an attachment (id=371462) [details] > initial comments >> + nsDeque mInStrings; > Could you use nsTArray<nsCString> here? I think a nsDeque is better, because it is a FIFO structure. > Why you need to unlock explicitly? This is aesthetic. It seems to me better than enclosing with "{}". > Nothing ensures that types[0] points to a valid value. Well, I've copied this stuff from netwerk http's code. What are the valid values? > Couldn't you use NS_NewURI here? I don't know if it would work now, since I've added the protocol handlers for "ws" and "wss". But, before of that, I found NS_NewURI relied on nsIOService, and because of this NS_NewURI didn't work (because of the schemes). Well, in this patch I've let as it was (using nsIStandardURL) :). Wellington.

Should I set the review flag to someone? Who? (Sorry, I forgot to ask this before)

FYI the spec has renamed the postMessage() method to send().

FYI the spec has also been slightly changed so that send() no longer fires an exception if called after the connection has closed (since that could happen at any time).

FYI the disconnect() method also got renamed to close().

Comment on attachment 386295 [details] [diff] [review] last patch merged with head revision >diff --git a/content/base/public/nsDOMEventTargetWrapperCache.h b/content/base/public/nsDOMEventTargetWrapperCache.h >new file mode 100644 >+ * The Original Code is mozilla.org code. >+ * >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. >+ * Portions created by the Initial Developer are Copyright (C) 1998 >+ * the Initial Developer. All Rights Reserved. Please don't. You're writing a new file If you aren't writing a new file, then I want to see an HG Copy in your patch. >+ * Initialize the object for use from C++ code with the principal, script >+ * context, and owner window that should be used. >diff --git a/content/base/src/Makefile.in b/content/base/src/Makefile.in >@@ -167,17 +167,19 @@ CPPSRCS = \ > nsXMLHttpRequest.cpp \ >+ nsWebSocket.cpp \ > nsXMLNameSpaceMap.cpp \ >+ nsDOMEventTargetWrapperCache.cpp \ Adding items like this is odd, is there a reason not to use alpha sort? >diff --git a/content/base/src/nsDOMEventTargetWrapperCache.cpp b/content/base/src/nsDOMEventTargetWrapperCache.cpp >new file mode 100644 Same comment. >+ * The Original Code is mozilla.org code. >+ * >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. >+ * Portions created by the Initial Developer are Copyright (C) 1998 > GK_ATOM(ononline, "ononline") > GK_ATOM(onoffline, "onoffline") >+GK_ATOM(onopen, "onopen") onoffline appears to be out of place, perhaps you could help it out? >+static NS_DEFINE_CID(kSocketTransportServiceCID, NS_SOCKETTRANSPORTSERVICE_CID); >+static NS_DEFINE_CID(kSocketProviderServiceCID, NS_SOCKETPROVIDERSERVICE_CID); >+static NS_DEFINE_CID(kProtocolProxyServiceCID, NS_PROTOCOLPROXYSERVICE_CID); >+static NS_DEFINE_CID(kCookieServiceCID, NS_COOKIESERVICE_CID); Do not use CIDs from other people's modules. use contractid's. >+// prevent more than one instance connecting a time prevent more than one instance _from_ ? connecting _at_ ? a time >+ // This isn't in the spec. I think slow servers can be a problem if Using "I" in comments is problematic. >+ // we always wait the last connection be established in order to establish wait _for_ ? ... connection _to_ ? be established *before establishing* ? >+ // another. So it limits to 10 tries. >+#define ENSURE_SUCCESS_AND_FAIL_IF_FAILED(res, ret) \ >+ nsresult __rv = res; \ >+ if (NS_FAILED(__rv)) { \ >+ NS_ENSURE_SUCCESS_BODY(res, ret) \ >+ return ret; \ I don't see why you would want to have both __rv and res running around, each only used once in your macro. >+ nsCOMPtr<nsIRunnable> event = \ >+ NS_NEW_RUNNABLE_METHOD(nsWebSocketStablishedConnection, this, \ >+ _method); \ I think we're still expected to handle oom locally (NS_DispatchToMainThread tails to something which asserts you wouldn't do that). >+ NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL); \ >+ * CONN_READING_FIRST_CHAR_OF_RESPONSE_HEADER_NAME -> >+ * CONN_READING_RESPONSE_HEADER_NAME | >+ * CONN_CONNECTED_AND_READY | >+ * CONN_READING_RESPONSE_HEADER_NAME -> >+ * CONN_READING_RESPONSE_HEADER_NAME | I don't see a transition from CONN_READING_RESPONSE_HEADER_NAME to CONN_READING_RESPONSE_HEADER_VALUE Is there a reason not to use an enum? :) >+#define CONN_NOT_CONNECTED 1 ... >+#define CONN_CLOSED 14 >+#define END_BYTE_OF_MESSAGE 0xff >+ extra blank line here? >+ It's generally bad for structure size to bury 8 bit creatures between other things >+ PRUint8 mStatus; >+ PRLock* mLockDisconnect; >+ PRLock* mLockOutStrings; >+ PRLock* mLockInStrings; >+ nsDeque mOutStrings; // has nsCString* which need to be send to be *sent* >+ nsDeque mInStrings; // has nsCString* which need to be dispatch as message events which *needs* to be *dispatched* *when/as a* ? message *event* ? ... ? >+ // we can connect only one instance a time, so we take hand of timers we can _only_ connect *to* one instance _at_ a time i don't understand the second half of your sentence at all :) I'll continue from here later: >+nsWebSocketStablishedConnection::PostData

Comment on attachment 386295 [details] [diff] [review] last patch merged with head revision >+ nsCString* buf = new nsCString(); >+ NS_ENSURE_TRUE(buf, NS_ERROR_OUT_OF_MEMORY); FROM HERE ON YOU MUST NOT USE NS_ENSURE_whatever without making sure you delete buf. >+ NS_ENSURE_TRUE(buf->Length() == static_cast<PRUint32>(maxLen), >+ NS_ERROR_OUT_OF_MEMORY); Like here. You leaked it. >+ rv = converter->Convert(aData.BeginReading(), &inLen, start, &outLen); >+ NS_ENSURE_SUCCESS(rv, rv); And here. >+ NS_ENSURE_TRUE(buf->Length() == static_cast<PRUint32>(outLen), >+ NS_ERROR_UNEXPECTED); And here. >+ >+ if (rv == NS_ERROR_UENC_NOMAPPING) { >+ // Yes, NS_ERROR_UENC_NOMAPPING is a success code >+ return NS_ERROR_LOSS_OF_SIGNIFICANT_DATA; And here. >+ nsAutoLock lockOut(mLockOutStrings); >+ mOutStrings.Push(buf); This is where you transfered ownership of buf. Until this point you've been leaking it. >+ lockOut.unlock(); nsAutoLock automatically unlocks when it leaves scope (i.e. HERE). >+ return NS_OK; >+ strRequest.AppendLiteral(" HTTP/1.1\r\n"); >+ // Upgrade: WebSocket >+ strRequest.AppendLiteral("Upgrade: WebSocket\r\n"); >+ // Connection: Upgrade >+ strRequest.AppendLiteral("Connection: Upgrade\r\n"); >+ // Host >+ strRequest.AppendLiteral("Host: "); I think that we'd probably want to consider concatenating these string literals (And yes, I'm aware that for spec comparisons there's a conflict) >+ if (mBuffer.Find(NS_LITERAL_CSTRING("HTTP"), 0) != 0) { Find is a very expensive operation to just do a check against 0. >+ if (mBuffer.CharAt(i) == '\r' || mBuffer.CharAt(i) == '\n') { >+ FailConnection(); >+ return NS_ERROR_ABORT; >+ } else if (mBuffer.CharAt(i) == ':') { drop "else", as the only way you can reach this point is if you didn't return which means you didn't satisfy the |if|. >+ mStatus = CONN_READING_RESPONSE_HEADER_VALUE; >+ return HandleNewInputString(i + 1); >+ } else { again. >+ if (mBuffer.CharAt(i) >= 'A' && mBuffer.CharAt(i) <= 'Z') >+ mBuffer.Replace(i, 1, mBuffer.CharAt(i) + 0x20); // to lower case >+ return HandleNewInputString(i+1); >+ } else if (mBuffer.CharAt(i) == '\n') { (and many other times below) >+ if (cookieServive && doc) { please use: if (!cookieService || !doc) return NS_OK; >+ nsCOMPtr<nsIURI> documentURI = doc->GetDocumentURI(); >+ if (documentURI) { similarly >+ nsAutoLock lockIn(mLockInStrings); >+ { >+ if (mInStrings.GetSize() == 0) >+ return; >+ >+ data = static_cast<nsCString*>(mInStrings.PopFront()); >+ } >+ lockIn.unlock(); We've historically used: nsAutoUnlock lockOff(mLockInStrings); instead of manually poking unlock. >+ nsCOMPtr<nsIDOMDocumentEvent> de = do_QueryInterface(doc); personally I'd prefer event or docEvent. >+ if (!de) { >+ NS_WARNING("failed getting de"); A warning about "de" is totally unhelpful. >+ mLockDisconnect = PR_NewLock(); >+ mLockOutStrings = PR_NewLock(); >+ nsCOMPtr<nsICookieService> cookieServive = do_GetService(kCookieServiceCID); Please don't misspell service >+ NS_WARNING("It has failed getting sourceSpec"); >+ NS_WARNING("It has failed getting targetSpec"); >+ NS_WARNING("It has failed getting bundleService"); >+ NS_WARNING("It has failed getting strBundle"); >+ NS_WARNING("It has failed getting console"); please use "Failed to get ... >+ NS_WARNING("strBundle->FormatStringFromName has failed"); drop 'has' >+ lockDisconnect.unlock(); Either use more {}s to scope, or don't do this. >+ >+ if (gWsConnecting == this) >+ gWsConnecting = nsnull; Here, all you have to do is move the nsAutoLock *inside* the block >+ nsAutoLock lockDisconnect(mLockDisconnect); >+ { ... >+ } And then you don't need this: >+ lockDisconnect.unlock(); >+ CHECK_SUCCESS_AND_FAIL_IF_FAILED(this, rv); >+ why the extra blank line here? >+ >+ nsCOMPtr<nsIProxyInfo> pi; >+ CHECK_SUCCESS_AND_FAIL_IF_FAILED(this, rv); >+ >+ mProxyInfo = pi; >+ lockDisconnect.unlock(); I'm going to stop complaining about your use of nsAutoLock.unlock(), please fix them all, thanks :) >+ nsAutoLock lockDisconnect(mLockDisconnect); >+ { >+ nsAutoLock lockOut(mLockOutStrings); >+ { >+ * It is used for constructing our nsWebSocket from javascript. It expects a URL JavaScript >+ * string parameter and an optional protocol parameter. Also, initializes the It also ... >+ // initializes the principal, the script context and the window owner. initialize <- no 's' >+ // the constructor should throw a SYNTAX_ERROR only if it fails parsing the ... fails to parse ... >+nsWSProtocolHandler::GetScheme(nsACString& aScheme) >+{ >+ aScheme = "ws"; aScheme.AssignLiteral("ws") ? >+nsWSSProtocolHandler::GetScheme(nsACString& aScheme) >+ aScheme = "wss"; >+++ b/content/events/src/nsDOMEvent.h >@@ -125,16 +125,18 @@ public: > eDOMEvents_paste, >+ eDOMEvents_open, >+ eDOMEvents_message, So from memory this is an enum which is effectively vaguely public, which means randomly inserting in various places is totally wrong. That said, I expect the DOM owners to get this right, so, I'm going to flag it and hold them responsible :) > #ifdef MOZ_SVG > eDOMEvents_SVGLoad,

> So from memory this is an enum which is effectively vaguely public, No it is not.

(In reply to comment #45) > (From update of attachment 386295 [details] [diff] [review]) > >+ nsCString* buf = new nsCString(); > >+ NS_ENSURE_TRUE(buf, NS_ERROR_OUT_OF_MEMORY); > > FROM HERE ON YOU MUST NOT USE NS_ENSURE_whatever without making sure you delete > buf. Simply use an nsAutoPtr<nsCString> buf along with its .forget() function and you'll be fine. Timeless, if you're reviewing peoples patches it'd be appreciated if you also help them up. I'd expect that you know how to use nsAutoPtr?

> onoffline appears to be out of place, perhaps you could help it out? I think that you meant to put it before ononline (alpha sort). >> So from memory this is an enum which is effectively vaguely public, >> which means randomly inserting in various places is totally wrong. > No it is not. I didn't understand. I think it is right, isn't it? > I don't see why you would want to have both __rv and res running around, > each only used once in your macro. This macro is implemented like the NS_ENSURE_SUCCESS macro. Both __rv and res are required by NS_ENSURE_SUCCESS_BODY.

I've found this at apache's bugzilla, about Web Sockets: https://issues.apache.org/bugzilla/show_bug.cgi?id=47485

Comment on attachment 388994 [details] [diff] [review] working patch (netwerk linking issue corrected) Wellington: Here are some comments. As I mentioned, I'm not done reviewing the whole patch. + * CONN_READING_RESPONSE_HEADER_VALUE -> + * CONN_READING_RESPONSE_HEADER_VALUE | + * CONN_WAITING_LF_CHAR_OF_RESPONSE_HEADER | + * CONN_CLOSED Correct me if I'm wrong, but one doesn't generally put a non-transition into a description of a state machine (?). So I'd leave out CONN_READING_RESPONSE_HEADER_VALUE here. +nsresult +nsWebSocketStablishedConnection::HandleNewInputString(PRUint32 aStart) +{ + NS_ASSERTION(!NS_IsMainThread(), "wrong thread"); Slightly better wording: "Not running on socket thread". This is in about 4 places. +#define IMPL_RUNNABLE_ON_MAIN_THREAD_METHOD(_method) \ + nsresult \ + nsWebSocketStablishedConnection::_method() \ + { \ .... + if (!mOwner) \ + return NS_OK; \ Wouldn't NS_ERROR_UNEXPECTED be better here? This can only get hit if Init() hasn't been called yet, which seems like a programming error. + // Authorization + rv = AddAuthorizationHeaders(strRequest, PR_FALSE); So here you're adding *either* proxy auth headers, *or* regular auth headers to the request. nsHttpChannel checks for them separately, and will add both if needed (http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#3846). Is there any reason not to do the same for websockets? I see that you call this function to add auth headers if the initial connection fails in OnOutputStreamReady(). But if we've already got proxy auth creds stored, we ought to use them in DoInitialRequest, too, right? +nsresult +nsWebSocketStablishedConnection::AddAuthorizationHeaders(nsAString& aStr, PRBool aIsProxyAuth) +{ + NS_ASSERTION(NS_IsMainThread(), "wrong thread"); + + nsresult rv; + nsCAutoString creds; + + nsCOMPtr<nsIHttpAuthManager> authManager = + do_GetService("@mozilla.org/network/http-auth-manager;1"); + if (!authManager) + return NS_OK; Getting the authManager should always succeed. I'd actually put in an assert here (NS_ENSURE_TRUE? maybe there's a better one to use, but something like that).

> +#define IMPL_RUNNABLE_ON_MAIN_THREAD_METHOD(_method) \ > + nsresult \ > + nsWebSocketStablishedConnection::_method() \ > + { \ > .... > + if (!mOwner) \ > + return NS_OK; \ > > Wouldn't NS_ERROR_UNEXPECTED be better here? This can only get hit if Init() > hasn't been called yet, which seems like a programming error. No. For instance, this use case won't work properly: 1. There is a message to be dispatched (from the server) in the main queue. 2. The user calls disconnect (by JavaScript) -> so, it would do nsWebSocketStablishedConnection::mOwner = nsnull 3. the previous method in the queue is called on the main thread

This is just FYI. As some already shared, WebKit project is also implementing WebSockets including Test Infra. On WebKit Bugzilla, you can find more info on the test infra at https://bugs.webkit.org/show_bug.cgi?id=27490.

Comment on attachment 390817 [details] [diff] [review] patch (comments jduell) +++ b/netwerk/base/src/nsProtocolProxyService.cpp + else if (info.scheme.EqualsLiteral("ws") || + info.scheme.EqualsLiteral("wss")) { Please add a comment referring to the websockets spec explaining why you're putting this after the SOCKS check, unlike all the other protocols. Not really happy about putting ws/wss here when necko doesn't know about it at all otherwise. Can you implement this algorithm in the websocket code instead? For pac, you can use nsIProxyAutoConfig.idl +++ b/netwerk/protocol/http/public/nsIHttpAuthManager.idl + * Get the creds for the given path that has been stored in the auth cache. It feels really weird to have this method on this interface. creds are not part of it anywhere. And indeed there's no hint at all as to what they are.

do you handle the case when there are no creds in the auth cache yet? from a brief glance it looks like you don't, but I might've missed something. you're also not handling NTLM auth correctly (to the proxy or the server)

> Not really happy about putting ws/wss here when necko doesn't know about it at > all otherwise. Can you implement this algorithm in the websocket code instead? > For pac, you can use nsIProxyAutoConfig.idl hmmm, ok, I can implement there. > do you handle the case when there are no creds in the auth cache yet? from a > brief glance it looks like you don't, but I might've missed something. yes, I do. See nsWebSocketStablishedConnection::AddAuthorizationHeaders > +++ b/netwerk/protocol/http/public/nsIHttpAuthManager.idl > + * Get the creds for the given path that has been stored in the auth > cache. > > It feels really weird to have this method on this interface. creds are not > part of it anywhere. And indeed there's no hint at all as to what they are. > The ws protocol sends the Authorizations headers before the server response. Because of that the creds are needed based just on the uri's path. Because content and necko are distinct modules, Jonas and I needed to get the creds somehow from an gecko's interface. Then, we decided, provisionally, add that method. Do you have any suggestion? > you're also not handling NTLM auth correctly (to the proxy or the server) Hmmm, I think you are writing about what nsHttpChannel::PrepareForAuthentication does, doesn't it?

BWTP (http://bwtp.wikidot.com/) has been proposed as an alternative to this HTML5 Websocket proposal. What do think about it, is BWTP going to replace WebSockets? I'm asking you this because I wouldn't like to work on something that soon would become useless.

> Jonas and I Jason and I, sorry.

I doubt BWTP will eventually succeed, given the patch here, WebKit's working on a patch, and the severe excess of complexity of BWTP compared to WebSockets. However, I am not clairvoyant, so I could be wrong (although I dearly hope I am not).

(In reply to comment #57) > > do you handle the case when there are no creds in the auth cache yet? from a > > brief glance it looks like you don't, but I might've missed something. > yes, I do. See nsWebSocketStablishedConnection::AddAuthorizationHeaders Well no, that does nothing if the credentials are not in the auth cache yet. > > +++ b/netwerk/protocol/http/public/nsIHttpAuthManager.idl > > + * Get the creds for the given path that has been stored in the auth > > cache. > > > > It feels really weird to have this method on this interface. creds are not > > part of it anywhere. And indeed there's no hint at all as to what they are. > > > > The ws protocol sends the Authorizations headers before the server response. > Because of that the creds are needed based just on the uri's path. Because > content and necko are distinct modules, Jonas and I needed to get the creds > somehow from an gecko's interface. Then, we decided, provisionally, add that > method. Do you have any suggestion? I'd make an nsIHttpProtocolHandlerInternal interface and put the function there and implement it on nsHttpHandler.cpp instead. I feel that it fits there much better. > > you're also not handling NTLM auth correctly (to the proxy or the server) > Hmmm, I think you are writing about what > nsHttpChannel::PrepareForAuthentication does, doesn't it? More like nsHttpChannel::GetCredentials. For NTLM (and negotiate-auth, which is pretty much the same) you can't just send a predetermined set of credentials, you need to see the WWW-Authenticate header from the server first. Similarly for Digest auth... in reality, your code as written only works for Basic auth.

> More like nsHttpChannel::GetCredentials. For NTLM (and negotiate-auth, which is > pretty much the same) you can't just send a predetermined set of credentials, > you need to see the WWW-Authenticate header from the server first. Similarly > for Digest auth... in reality, your code as written only works for Basic auth. Yes, you are right. But the WebSocket protocol neither handles the 401 http response nor handles the WWW-Authenticate header.

Sure, but the proxy can be configured for Digest or NTLM authentication.

And, why shouldn't the websocket protocol handler handle 401/www-authenticate?

(In reply to comment #64) > And, why shouldn't the websocket protocol handler handle 401/www-authenticate? This is a good question... I need to check if the protocol spec has been changed. But, I've just found this irc log: " # [10:24] <john_fallows> but there was also a note previously with a TODO for status code 401 # [10:24] <john_fallows> now 401 would fail the connection, right? # [10:24] <Hixie> yeah i decided to punt on that for this version " from: http://krijnhoetmer.nl/irc-logs/whatwg/20090228

http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol-35 See near the bottom of page 16, and also in other places. I'm assuming that that's the right version of the spec, but I don't really know where to find the authoritative version, so maybe it's not...

Note that your IRC quote is from February, whereas that spec link is from August.

(In reply to comment #67) > Note that your IRC quote is from February, whereas that spec link is from > August. Hmmm, nice. Ian has changed the spec since the last patch I've uploaded. I will implement this, no problem.

Yeah I added that recently.

Just FYI, we've open sourced an experimental Web Socket extension for Apache HTTP Server: http://code.google.com/p/pywebsocket/ Please feel free to use it if you need to have the server implementation to test with. Thanks, Takuya

> Not really happy about putting ws/wss here when necko doesn't know about it at > all otherwise. Can you implement this algorithm in the websocket code instead? > For pac, you can use nsIProxyAutoConfig.idl I'm close to finishing to add authentication to the patch. Really necko doesn't know about ws at al. However, the changes are small (10 lines) and they don't affect performance. I can implement it in the websocket code, but the changes there will be bigger. Is this change really necessary?

ps: My first phrase should be before the Biesinger's phrase, because it was an unrelated subject, sorry.

This patch handles the 401 and 407 http responses. I've tested the basic and digest authentications. I've been a little bit busy these last days and because of this I haven't had time to implement proxy resolution inside the patch. I will work on this in the meanwhile you review this code. Wellington.

Comment on attachment 402012 [details] [diff] [review] working-path (with authentication) Note that this patch doesn't cleanly apply to the mozilla-central tip (Not important as this is a work-in-progress patch). Also, it looks like most of your files (ex: nsWebSocket.cpp) are using windows-style carriage returns in the diff. Please make sure to use Unix ones (\n, not \r\n). >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ Your tab-width should be 8, and you need to add a line for vim. These are from the Coding style guide: /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ /* vim: set sw=2 ts=8 et tw=80 : */ >+// static >+nsresult >+NS_AddIPConnecting(const PRNetAddr& aIP) >+NS_RemoveIPConnecting(const PRNetAddr& aIP) These are each only used in one place, so they might as well be "static inline". The HTTP auth stuff looks good at a glance, but biesi knows this code much better than I, and I'm going to be away on vacation for about a week. Try to ping biesi on IRC to see if he can give you any feedback in the mean time.

Comment on attachment 402012 [details] [diff] [review] working-path (with authentication) Jonas. I've looked over some of this patch already, but it's fairly big, and there are parts of it (especially the DOM stuff) that I'm fairly clueless about. Whatever you've got time to look at would be great--I'd like to move this websockets stuff along.

This patch does proxy resolution and it is complete. Wellington.

Some changes. Now it is good :) I last comment mine. Since my second or third patches sometimes there is the assertion: "potential deadlock detected". But that never happens at all. I've already checked it. It happens because the main thread uses either the disconnect lock or the two other ones (in/out locks), separately. However the socket threads always does disconnect lock -> other lock. Wellington.

So I had some comments for the first third or so of the patch, but... this patch duplicates a lot of the httpchannel functionality (all the authentication stuff). I know that I pretty much suggested that earlier, but seeing this fairly complex logic duplicated does really not seem ideal. I can't really think of a good solution unfortunately...

See also http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-November/024108.html

(In reply to comment #78) > So I had some comments for the first third or so of the patch, but... > > this patch duplicates a lot of the httpchannel functionality (all the > authentication stuff). I know that I pretty much suggested that earlier, but > seeing this fairly complex logic duplicated does really not seem ideal. I can't > really think of a good solution unfortunately... It seems Chrome doesn't support authentication yet. Also, the spec has been changed to support only the 407 response code. Anyway, because there is the 407 use case, this stuff is supposed to be still necessary. Perhaps the authentication stuff could be in a helper class, inside somewhere in necko/protocol/http/, (although ws isn't http...). There could be a interface method like the below (I don't know which interface): nsresult ProcessAuthentication(nsIHttpChannel, nsIAuthPromptCallback); Then, nsHttpChannel and nsWebSocket could use that method and resume their jobs when they receive feedback from its nsIAuthPromptCallback methods. This is the best I can think right now :) regards

Yeah... if you can do that that'd be great. I don't think you want to pass an nsIHttpChannel there, but a new interface with the required methods should work.

I've created a channel interface for authenticating. The implementation shares the previous nsHttpChannel auth code. Unfortunately some interfaces (like nsIAuthPrompt2.idl) need a nsIChannel interface, and some of them are frozen (or it would need a lot of changes...). Well, I did the best I could :) Wellington.

Is this code already merged on trunk? Using this would give my employers web application a leading edge. ;-)

Not yet. Still waiting for review on the patch.

Why are those protocols URI_LOADABLE_BY_ANYONE, exactly? For that matter, why are they INHERIT_PRINCIPAL, if they don't return data?

Comment on attachment 415025 [details] [diff] [review] patch sharing auth code Hmm.. this patch seems to be heavily broken. It removes all OnChannelRedirect calls along with other major pieces of nsHttpChannel code - you remove more then 6000 lines of code and I don't see it elsewhere! Also nsHttpChannel h and cpp files are contained twice in the patch. However, I would like to see the asynchronous redirect API patch (bug 513086) landed before this huge (and probably not really necessary) http channel refactoring.

(In reply to comment #86) > (From update of attachment 415025 [details] [diff] [review]) > Hmm.. this patch seems to be heavily broken. It removes all OnChannelRedirect > calls along with other major pieces of nsHttpChannel code - you remove more > then 6000 lines of code and I don't see it elsewhere! They aren't removed. If you apply this patch you will see the OnChannelRedirect in the code. > Also nsHttpChannel h and cpp files are contained twice in the patch. It is because I've copied (hg cp) them into new files and I have changed the original nsHttpChannel.* ones... > However, I would like to see the asynchronous redirect API patch (bug 513086) > landed before this huge (and probably not really necessary) http channel > refactoring. Well, anyway, I did this last patch in order to share code. The previous one works either, and don't have these many changes in netwerk.

(In reply to comment #85) > Why are those protocols URI_LOADABLE_BY_ANYONE, exactly? nsIProtocolHandler states that the one of these four flags: URI_LOADABLE_BY_ANYONE, URI_DANGEROUS_TO_LOAD, URI_IS_UI_RESOURCE or URI_IS_LOCAL_FILE must be set. I actually didn't know if the best was URI_LOADABLE_BY_ANYONE or URI_IS_UI_RESOURCE. Well, I chose the first one because IMHO it was the most similar to XHR. > For that matter, why are they INHERIT_PRINCIPAL, if they don't return data? Hmmm... Good question. Well, I think this flag can be removed :) Wellington.

(In reply to comment #87) > (In reply to comment #86) > > Also nsHttpChannel h and cpp files are contained twice in the patch. > > It is because I've copied (hg cp) them into new files and I have changed the > original nsHttpChannel.* ones... Ah, I see. I had to check the patch itself and not just the diffs. So, it should not be that much horrible work to merge.

> I actually didn't know if the best was URI_LOADABLE_BY_ANYONE or > URI_IS_UI_RESOURCE. Why shouldn't it be URI_DANGEROUS_TO_LOAD? Specifically, should <a href="ws://something"> in a webpage return a security error when clicked, or actually do a network access of some sort?

> Why shouldn't it be URI_DANGEROUS_TO_LOAD? Specifically, should <a > href="ws://something"> in a webpage return a security error when clicked, or > actually do a network access of some sort? No, it shouldn't. Really, URI_DANGEROUS_TO_LOAD is the correct flag.

Has work on this stopped? It would be nice to have Web Sockets shipped with Firefox 3.7.

For what it's worth, there's a recent thread on the whatwg/hybi lists about if httponly cookies should be sent. I suspect the answer will be "yes". Don't know if that's what's happening in this patch already, nor do I think we need any particular behavior in the initial landing. But didn't want to loose track of this issue, which is why i'm posting here.

Yes, answer will be yes.

Comment on attachment 415025 [details] [diff] [review] patch sharing auth code Reassigning review to smaug and sr to biesi. We'd like to land ASAP!

nice :) if questions about the code arise please send an email or ping me on irc.

Seems like the work on web Sockets are going on quite nicely.Hope to see WebSocket enabled Firefox soon. Is there a plan that the Websocket may act as a server socket( A socket that listens for connections and returns particular reply), as this will be helpful in many purposes e.g. sharing Firefox cache on LAN.

Here are some (mainly) pretty minor review comments. I'll continue tomorrow from websocket.cpp and httpchannel.cpp (The latter one is really difficult to review because hg seems to have generated pretty terrible patch file) Up-to-date patch would be nice, so that I could test it with Apache for example. And that would also help with httpchannel.cpp reviewing. I need to also re-review websockets draft spec.

Olli, this patch is up-to-date (parent revision is 51f9d293c939).

This patch is the same as the last one, but it has full context for nsHttpChannel.cpp and netwerk/protocol/http/src/nsHttpAuthManagerInternal.cpp sources. I think this can facilitate the review for those files.

Olli, I've just found a problem in nsHttpAuthManagerInternal.cpp. In some places when it was supposed to test if there was a *http proxy* it was only checking if there was a *proxy*. I've already fixed that here, in my local repository. When you finish the review and I apply your comments my patch will have that fix, too. Wellington.

Just a heads-up — the handshake is likely to change in the coming weeks, based on discussion on hybi.

Hixie. Websockets handshake can change? How? It's standard.

Websockets API is currently a W3C draft (is it actually just an Editor's draft?) and the protocol is an IETF draft. So anything in the both drafts may change before they become recommentations/specifications.

+ DOM_CLASSINFO_MAP_BEGIN(WebSocket, nsIWebSocket) + DOM_CLASSINFO_MAP_ENTRY(nsIWebSocket) + DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget) + DOM_CLASSINFO_MAP_END Should have also nsIDOMNSEventTarget

Next will be the actual protocol frame handling. (I still don't understand why the protocol has the sort-of-optional binary-frames. Why they are there if they are semi-optional?)

> (I still don't understand why the protocol has the sort-of-optional > binary-frames. Why they are there if they are semi-optional?) On hybi was said the protocol should have binary content support, mainly for standalone apps. However, for browsers, currently binary content is supposed to be discarded (but the implementation must handle it).

On the other hand server may disconnect if it gets binary-frame. I just don't understand why to have such optional feature in the spec, if no one needs to handle it. IMO, it would make more sense to have it *must implement/handle*, than *may*. The fact the WebSocket API uses only text-frames is a different thing. But anyway, this is a discussion for hybi list. I'll send a message.

If you could update the patch now. I need to look at the netwerk/ part some more, but that is anyway mainly about splitting nsHttpChannel. And that is code which I hope biesi (or someone) could review more carefully. But anyway, perhaps it would make sense to update the patch now.

> But anyway, perhaps it would make sense to update the patch now. ok. > Not sure I like hardcoding wss usage here, but I don't have better > suggestions. Perhaps biesi has. I thought, when I was implementing that, add a nsIURI::SchemeIsSSL() or something like that, but that interface is FROZEN. Maybe we could create another interface and add it to the implementation class, I don't know.

(In reply to comment #112) > I thought, when I was implementing that, add a nsIURI::SchemeIsSSL() or > something like that, but that interface is FROZEN. Maybe we could create > another interface and add it to the implementation class, I don't know. Perhaps there could be some helper method in nsINetUtils or somewhere which checks if the "uri scheme is SSL".

Comment on attachment 426920 [details] [diff] [review] up-to-date patch >+IMPL_RUNNABLE_ON_MAIN_THREAD_METHOD_BEGIN(DoInitialRequest) >+{ >+ nsresult rv; >+ nsString strRequest; >+ >+ // GET resource HTTP/1.1 >+ strRequest.AppendLiteral("GET "); >+ strRequest.Append(NS_ConvertUTF8toUTF16(mOwner->mResource)); >+ strRequest.AppendLiteral(" HTTP/1.1\r\n" >+ "Upgrade: WebSocket\r\n" >+ "Connection: Upgrade\r\n" >+ "Host: "); >+ strRequest.Append(NS_ConvertUTF8toUTF16(mOwner->mHost)); >+ strRequest.AppendLiteral("\r\n"); Just that I don't forget the IRC conversation; Host: should include the port if the port isn't the default one.

nsHttpChannel::OnAuthAvailable and nsHttpChannel::OnAuthCancelled have at least wrong LOG(), but I also need to understand the changes to those methods.

I wonder if there was some way to generate a patch which was easier to review.

This is the right file

For some reason I can't 'hg import' or patch -p1 the latest patch. I get lots of Hunk #X failed errors

(In reply to comment #120) I forgot to say when I uploaded, the parent's revision is d80b2bb478dc

I've tested this one. hg import -p 1 worked for me :) Wellington.

and you forgot again ;) To save you some hassle: applies to rev 1918f1187eb6 (nsHttpChannel.cpp gets changed a lot)

Comment on attachment 428497 [details] [diff] [review] updated patch Just a few notes: - shouldn't nsHttpChannelAuthInternal::Cancel also release the mAuthChannel? if you get network error, the reference will never be released and we leak the whole channel; calling Disconnect in channel destructor is ineffective - looks like you are not releasing mCallback in nsHttpChannelAuthInternal, would be better to throw it away when it is no more needed - on top of previous: as you always pass 'this' as a callback to ProcessAuthentication, wouldn't be better if you communicate OnAuthAvailable and OnAuthCanceled to the authenticable channel via additional method(s) of the new nsIHttpAuthenticableChannel interface? the arguments of nsIAuthPromptCallback methods are useless for it anyway, you pass null, null; you also save your self trouble with mCallback - nit: you probably want to use NS_DECL_NSIHTTPAUTHENTICABLECHANNEL macro to declare your methods

(In reply to comment #125) > - shouldn't nsHttpChannelAuthInternal::Cancel also release the mAuthChannel? > if you get network error, the reference will never be released and we leak the > whole channel; calling Disconnect in channel destructor is ineffective I'm not sure if Cancel should disconnect from the channel or just cancel the current authentication, I will take a look at the previous nsHttpChannel code. But, anyway, mAuthChannel is a weak reference, so the channel wouldn't leak at al.

(In reply to comment #123) > and you forgot again ;) To save you some hassle: applies to rev 1918f1187eb6 > (nsHttpChannel.cpp gets changed a lot) thanks :)

> + // TODO: handle the set-cookie2 header > Do we need a followup bug for this? yes, I think so, because current code looks like do not handle this header.

Does websockets require set-cookie2 support? If so I think we should argue to get the spec changed. We don't support set-cookie2 for http, and I don't see a reason to add it.

(In reply to comment #129) > Does websockets require set-cookie2 support? If so I think we should argue to > get the spec changed. We don't support set-cookie2 for http, and I don't see a > reason to add it. Yes, it does: "-> If the entry's name is "set-cookie" or "set-cookie2" or another cookie-related header name Handle the cookie as defined by the appropriate specification, with ... "

At the very least, websocket should allow the UA to use the same level of support for those headers as it does in its HTTP implementation.

(In reply to comment #131) > At the very least, websocket should allow the UA to use the same level of > support for those headers as it does in its HTTP implementation. Yes. Because of this I let the handle case of this header just as a TODO comment in the code. When http's code have support for this header I think it could be easy to add it to websockets, like it does for set-cookie.

(In reply to comment #126) > But, anyway, mAuthChannel is a weak reference, so the channel wouldn't leak at > al. Didn't notice first, now I understand the code in the destructor as well. Sorry for spam.

Comment on attachment 428799 [details] few comments >Event should be non-bubbling and non-cancelable, so the 3rd parameter should be >PR_FALSE. >The draft specification doesn't actually say that origin should be set at all. >Could you use null string for origin and lasteventid parameters: >nsAutoString nullString; >SetDOMStringToNull(nullString); >and then nullString as parameter. > >I know, using null vs empty string isn't specified in the draft. Ah, my mistake, this is specified in another draft (HTML5). So empty string is the right thing to do.

Comment on attachment 428799 [details] few comments >So bufferedAmount does take account start and end bytes in this case. Note, the draft has changed http://dev.w3.org/html5/websockets/#dom-websocket-bufferedamount So the bytes added by the protocol don't affect to bufferedAmount.

> + nsCOMPtr<nsIDocument> doc = > + nsContentUtils::GetDocumentFromScriptContext(mOwner->mScriptContext); > + rv = mOwner->CheckInnerWindowCorrectness(); > + if (NS_FAILED(rv) || !doc) { > + continue; > + } > Do you actually need doc for anything. I guess it is a leftover from > previous patch. I don't know if CheckInnerWindowCorrectness::CheckInnerWindowCorrectness is wrong, but in my tests when the window became frozen it was necessary check the doc in order to prevent dispatching messages events.

CheckInnerWindowCorrectness::CheckInnerWindowCorrectness -> nsDOMEventTargetHelper::CheckInnerWindowCorrectness/

> (...) when the window became frozen (...) Sorry. I meant when the window is closed but not actually deleted (by the garbage collector).

(In reply to comment #136) > I don't know if CheckInnerWindowCorrectness::CheckInnerWindowCorrectness is > wrong, but in my tests when the window became frozen it was necessary check the > doc in order to prevent dispatching messages events. Oh, right. I had totally forgotten frozen windows. We should close the connection if the window is frozen, I think. Or actually, would it make sense to mark document or window to have open websocket connections, and if it has, then nsDocument::CanSavePresentation could return false. Also, I think the connection should be closed when the window is closed.

parent's revision is 4e1b68ecf126

jduell, feel free to review the patch. These comments are pretty minor.

Comment on attachment 429716 [details] [diff] [review] updated patch From my part this is r+. If new issues are found, I can still re-review. This definitely needs a second review.

> Strange changes to js/src Actually, strange changes... Sorry, my diff tool didn't show me these changes when I checked the patch before uploading.

Has the promised new WS handshake been announced yet? (I'm way behind on my hybi email.) If so, have we implemented it?

Spec should be available by the end of the week.

Comment on attachment 429716 [details] [diff] [review] updated patch biesi will hopefully sr this within a week or so. If not, I will have to give it a go, but it will take longer and I'll be much more likely to miss bugs in the patch.

WebSocket draft is now at: http://www.whatwg.org/specs/web-socket-protocol/ It has the draft proposals for a modified opening handshake and a closing handshake. I've also updated the API: http://dev.w3.org/html5/websockets/ It now has an onerror event handler for reporting bad frames from the server, and the onclose event object has an attribute that reports if it was a clean close or not (suggestions for better names for that attribute are very welcome). This is all draft text of course, please send feedback to the hybi list when you spot whatever mistakes I made. :-)

One thing to fix is the bufferedAmount when a whole message hasn't yet been sent. In that case mOutgoingBufferedAmount should be reduced by the amount of the sent bytes - 1 (the frame start byte).

I think we could or should even take the patch before adding all the latest changes to the draft specs, unless the protocol changes so heavily that the implementation needs significant changes. I'm not at all sure I like the changes to the API and I haven't yet read properly the changes to the protocol.

I have just read new handshake proposal and I think it is way too complicated and in my opinion unnecessary. As far as I understand WebSocket had major design goals: 1. Be different from "raw" socket in order to prevent browsers from becoming world wide swarming medium for worms by not allowing direct connections to existing services like SMTP, POP and others. I believe it would be sufficient to solve this problem the same way as Java applets do by allowing to connect to origin host only. Significant part of todays browsers support Java applets which can use "raw" sockets but we don't see any mass (or at all) exploits of this feature. One additional problem with "raw" sockets is that JavaScript can not handle binary data, but that could be easily solved by few additional functions in WebSocket API. Anyway old WebSocket handshake for me seems adequate to provide this goal. 2. Existing proxy support. New WebSocket handshake doesn't simplify reaching this goal at all. I think new proposal is drifting away from the original goal - allowing bidirectional persistent connections from browsers. All checks that right client is connected to right server using right sub protocol can be achieved by sending relevant keys and IDs in the first frames after handshake as I do in my own application. I believe web developers just want simple way to send arbitrary data via bidirectional persistent connection. Wouldn't it make sense to keep WebSocket handshake as simple as possible and allow to emerge micro protocols like autentification and session management at application layer? Some services by definition don't require any restrictions on their clients. For example a server which provides free realtime stock information or news. Such server would like to push relevant information to clients as fast as possible (realtime) without wasting time calculating MD5 sums to validate their origin or prevent connections by carefully crafted packets using XMLHttpRequest or a form submission as it is said in the new proposal (if it is really possible). In my opinion possibility to connect to WebSocket server from various sources (and not only from a browser) is a positive thing. It is core idea of internet to allow heterogenous nodes to communicate. One can imagine that as WebSocket services will emerge then they will be used not only by web applications but also by desktop applications. Old WebSocket handshake could be trivially implemented by any programming language which supports sockets. Now new WebSocket handshake requires MD5 sum which is also not so hard to implement but it is not trivial and it is not yet in standard libraries of many programing languages. I would suggest to leave old WebSocket handshake as it was.

Please discuss the protocol on the hybi or whatwg lists, rather than in a bug database. :-)

My review comments for the netwerk/ part. Does not include nsHttpChannelAuthInternal.cpp because I have to catch the last train home, will do that tomorrow.

(In reply to comment #152) > +++ b/netwerk/protocol/http/public/nsIHttpAuthenticableChannel.idl > + * The channel being authenticated. > + */ > + readonly attribute nsIChannel channel; > > Is that useful? When would it not be "this"? Well, this was my biggest problem. This is used here: PromptForIdentity() { ... authPrompt->AsyncPromptAuth(channel, .. ... } After some deep look at the sources I've realized that only nsIChannel::URI is used. So, we have three options: 1. let it as it is; 2. change authPrompt(nsIChannel) to authPrompt(nsIURL) (a lot of changes and touching in some interfaces that are going to freeze) 3. make nsIHttpAuthenticable channel descend from nsIChannel instead of nsIProxiedChannel. Wellington.

4. Use QueryInterface to go from nsIHttpAuthenticableChannel -> nsIChannel (that's what I meant)

Comment on attachment 433190 [details] review comments for the netwerk/ part of attachment 429716 [details] [diff] [review] (partial) Please ignore these comments: - CheckForSuperfluousAuth(); + if (!mAuthRetryPending) + mAuthInternal->CheckForSuperfluousAuth(); Why did you add the if? + if (!mAuthRetryPending) + mAuthInternal->CheckForSuperfluousAuth(); Same question

That said, I'm not sure there's much point in moving CheckSuperfluousAuth to this new interface. Websockets don't need the functionality, and it looks like the function doesn't need any of the auth-specific member variables either.

ok. I will make these changes. BTW, should the new handshake proposal be in the next patch?

There is IETF meeting next week, though I'm not sure whether they will come up with anything better to replace the ugly handshake which the latest version of the protocol has. pywebsocket is apparently updated to the latest version of the protocol. So I'm not sure. Fix first all the comments, and if you have time you could update the implementation to support the latest version. Also the API has changed a bit, but I'm hoping Hixie will remove CloseEvent interface and add ERROR state to the WebSocket, or something like that.

Please wait a bit, I'm also reviewing the non-netwerk portions of the patch and have comments. Will attach them when I'm done.

ok

Here you go.

Here there are some answers, comments and questions about what I've done until now. Tomorrow I continue implementing :).

Comment on attachment 437475 [details] some comments and questions >> Hmm, am I misreading the spec or has host authentication been removed from it? >Yes, it has. However I think Mozilla should handle this case. The new handshake >has a strange proposal for improving security, but I think the old http auth works >nice... What do you think, remove or let it? Which version are you reading. I think you should be implementing v75. IIRC v76 has (had?) the bizarre and ugly handshake. I can't access v76 now, so perhaps it has been removed for now. Or was the host auth in some older version? I wonder why it was removed.

http://www.whatwg.org/specs/web-socket-protocol is the latest version. -75 has security bugs, so I strongly recommend avoiding it.

http://dev.w3.org/html5/websockets/ has the link to the protocol and that gives v.75 here.

But anyway, it is ok at least to me to implement v75 and update the implementation in another bug.

If you implement -75, please at least fix the security bugs.

AFAICT 0.75 is fundamentally flawed from a security perspective. Namely, it has given no consideration to security beyond very simple agent-specified port blocking, which is utterly inadequate to address a slew of CSRF and behind-the-firewall network attacks 0.75 will enable. I'm not going to judge whether 0.76 is the optimal design, but some sort of server handshaking is necessary to ensure the server has opted into websocket access. We cannot ship 0.75.

No one, I hope, is saying we could or should really ship v75. But we could get the patch here "done" and update the protocol implementation in a different bug. The patch is huge (partly because it moves some code around) and is hard to review. Making smaller changes in a followup bugs would be easier, I think. (Note, Chrome did ship some old version of the protocol)

(In reply to comment #169) >but some sort of server handshaking is > necessary to ensure the server has opted into websocket access. And btw, v75 does have some sort of handshake ;)

Oops, you're right, I was overreacting to the w3 draft. :) But we should go with 0.76, or wait for the spec to stabilize. We should not ship with known security weaknesses in something as potentially powerful as web sockets.

Shipping is different than getting some code - which can be improved - to landed.

Another option is to complete review of this patch and then write another patch on top of it that updates to v76, and land the patches at the same time.

Parent's revision is 2c44a32052ad

other comments, answers and questions...

Comment on attachment 437724 [details] other comments >> It might be safer to do Close asynchronously. Close may >> dispatch events. >It gives some assertions when the window is closed, because the close event is sent >for a document which doesn't exists anymore. For now I let as it was. Do you have >any suggestion? > Perhaps Close could check if window is still "alive".

Comment on attachment 437724 [details] other comments >Yes, but some of these changes are related to the v.76. I will let this interface >for when the patch implements this new version. Yeah, that's fine. >It is because the ProxyService doesn't provide the manual http and https proxies >(because of the ws and wss schemes). So, if a null proxy is given and the config >is manual the code should try with the http and https proxies specified by the >prefs. You should add a comment for that. >Well, ws messages shouldn't be in the cache. I don't know exactly how the netwerk's >transport layer (nsISocketTransport and soon) handle this (because IMHO this should >be done by the http protocol, but actually I don't know). For now I will remove the >VALIDATE_NEVER. If you wnat I remove the others ones. Those two comments were fairly independent. The nsISocketTransport::BYPASS_CACHE flag means that the DNS cache will be bypassed. There seems to be no reason for that here. The "again" was referring to the code that passes nsIDNSService::RESOLVE_BYPASS_CACHE, which also seems to be unnecessary. As for VALIDATE_NEVER and the other load flags - the way you use them here doesn't really make sense. They are really meant to be set by the user, but there is no user here. If the protocol doesn't support caching (like websockets, but also file:, etc), you should just not set any of the cache-related flags. The LOAD_BACKGROUND one does make sense since that affects whether the throbber/progress bar should show that a load is pending, which I guess you don't want here. >Well, I tried, but I couldn't find a way to convert to UTF-8. So I simply appended >the string to a nsCString... Yes, leaving it as an nsCString is the only correct behaviour for cookies. >> It might be safer to do Close asynchronously. Close may >> dispatch events. leaving this one to smaug

Comment on attachment 437475 [details] some comments and questions you forgot this one.

Comment on attachment 437475 [details] some comments and questions >if (mProxyConfig == eProxyConfig_Direct || > (mProxyConfig == eProxyConfig_Manual && > !CanUseProxy(uri, info.defaultPort))) // it was the unique call to this method > return NS_OK; > >which was preventing the use of filters. (If I am correct it is a bug, isn't it?) CanUseProxy returned true when filters existed. So this code didn't return early, so there was no bug. > No, it needs mSuppressDefensiveAuth (when calling ConfirmAuth). oh :/ OK >Well, if you want I can split the patch into three new ones. One for content refactoring, >one for netwerk refactoring, and one for WebSockets itself. I think I'd prefer that. > It is because NS_ENSURE_SUCCESS_BODY expects this variable. Oh I see. That macro behaved differently than I expected. Your code is correct. >> You don't yet support instantiating web sockets from DOM workers, is that >> correct? >No, this patch doesn't. See #504553. I think Olli is going to implement it. OK. Parts of your patch won't work so well when instantiating sockets on a non-main thread. >> what if you're using a proxy server? in that case, you shouldn't leak the host >> names to the DNS server... > Well, in this case, should get the proxy's IP (and then connect one ws a time using it) > or should it avoid checking IPs? You should get the spec changed to describe the desired behaviour :) Using only one websocket per proxy doesn't sound desirable. Maybe use the hostname instead of the IP in such a case? But really, the spec should say. >Yes, it has. However I think Mozilla should handle this case. The new handshake >has a strange proposal for improving security, but I think the old http auth works >nice... What do you think, remove or let it? I'm not sure that we should implement a feature that's been removed from the spec... it won't be interoperable with other browsers, right? >I've moved the SetCookie stuff to an RUNNABLE_ON_MAIN_THREAD_METHOD method, and I've >delete the condition var. I think it solves the problem. OK. Though still... this code blocks the socket transport thread for an unspecified amount of time. It might be better to create a "websocket thread" and use that for all the asyncWait calls.

I didn't! I just did them in the wrong order ;)

> Perhaps Close could check if window is still "alive". But then the close event won't be called sometimes. It seems window::onbeforeload is dispatched whenever ws::close should be. Perhaps we could make it in some similar, what do you think about Olli? > about the load flags Thanks by the explanation :) >>> If proxyconfig == direct, there can still be a filter that >>> turns that into a non-direct proxyinfo. >> if (mProxyConfig == eProxyConfig_Direct || >> (mProxyConfig == eProxyConfig_Manual && >> !CanUseProxy(uri, info.defaultPort))) > CanUseProxy returned true when filters existed. So this code didn't return > early, so there was no bug. But when mProxyConfig == eProxyConfig_Direct CanUseProxy will not be ever called. >> Well, if you want I can split the patch into three new ones. >> One for content refactoring, one for netwerk refactoring, >> and one for WebSockets itself. > I think I'd prefer that. ok, I will take the last patch that I've uploaded and split it, right now. > You should get the spec changed to describe the desired behaviour :) Using > only one websocket per proxy doesn't sound desirable. Maybe use the hostname > instead of the IP in such a case? But really, the spec should say. The last patch that I've uploaded doesn't check the ip anymore when there is a proxy. Hoewever I will write to Ian asking about. > I'm not sure that we should implement a feature that's been removed from the > spec... it won't be interoperable with other browsers, right? Ok, I've removed the 401 auth stuff. > It might be better to create a "websocket thread" and use that for all the > asyncWait calls. Ok. One thread per ws or just one global "websocket thread"?

The proxy stuff is supposed to work the same for Web Sockets as for HTTP. Re the new handshake and auth and so forth, please do send feedback to the list, so the spec can be updated accordingly. Your implementation feedback is extremely valuable. I want the spec to be what you want to implement.

(In reply to comment #182) > But then the close event won't be called sometimes. That is IMO, and I think something we can't avoid easily. If webapps cares a lot about the ws connection, it could do something to it in beforeunload or unload listener. > It seems > window::onbeforeload is dispatched whenever ws::close should be. I don't understand this? You mean beforeunload? Why should close be dispatched at that time. beforeunload can actually prevent closing the window, so the connection shouldn't be automatically closed at that time.

> I don't understand this? You mean beforeunload? Why should close be dispatched > at that time. beforeunload can actually prevent closing the window, so the > connection shouldn't be automatically closed at that time. I meant to say that where nsGlobalWindow dispatches this event, it could be changed in some way to notify websockets if the window is going to close. But I don't know if it is a good solution. > If webapps cares a lot about the ws connection, it could do something to > it in beforeunload or unload listener. It sounds good to me. But I think that the API spec should be changed to make clear that this event might not be dispatched. So developers would take care when using the close event.