The default bug view has changed. See this FAQ.
Bug 472529 (websocket)

Support for Web sockets' HTML5 Draft Recommendation (websocket)

RESOLVED FIXED in mozilla2.0b1

Status

()

Core
General
--
enhancement
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Wellington Fernando de Macedo, Assigned: Wellington Fernando de Macedo)

Tracking

unspecified
mozilla2.0b1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(Whiteboard: [evang-wanted-3.7], URL)

Attachments

(20 attachments, 35 obsolete attachments)

44.36 KB, application/zip
Details
6.14 KB, text/plain
Details
22.16 KB, application/x-zip-compressed
Details
6.84 KB, text/plain
Details
9.70 KB, text/plain
Details
3.78 KB, text/plain
Details
15.11 KB, text/plain
Details
3.88 KB, text/plain
Details
148.46 KB, text/plain
Details
813 bytes, text/plain
Details
5.45 KB, text/plain
Details
3.98 KB, text/plain
Details
6.71 KB, text/plain
Details
1.77 KB, text/plain
Details
10.98 KB, text/plain
Details
3.06 KB, text/plain
Details
1.66 KB, text/plain
Details
22.24 KB, patch
smaug
: review+
Details | Diff | Splinter Review
112.60 KB, patch
Details | Diff | Splinter Review
355.53 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.

Comment 1

8 years ago
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.
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Comment 4

8 years ago
Created attachment 358882 [details] [diff] [review]
initial work

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.
Assignee: nobody → wfernandom2004
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

8 years ago
Summary: Support for Web sockets' HTML5 Draft Recommendation → Support for Web sockets' HTML5 Draft Recommendation (websocket)

Updated

8 years ago
Alias: websocket
Summary: Support for Web sockets' HTML5 Draft Recommendation (websocket) → Support for Web sockets' HTML5 Draft Recommendation

Updated

8 years ago
Alias: websocket
Summary: Support for Web sockets' HTML5 Draft Recommendation → Support for Web sockets' HTML5 Draft Recommendation (websocket)

Comment 5

8 years ago
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.
(Assignee)

Comment 6

8 years ago
Created attachment 362389 [details] [diff] [review]
work in progress

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.
(Assignee)

Comment 7

8 years ago
Created attachment 363569 [details] [diff] [review]
work in progress

this has the work I've done this week.
Attachment #358882 - Attachment is obsolete: true
Attachment #362389 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
Created attachment 364317 [details] [diff] [review]
working patch

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?)
Attachment #363569 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
Created attachment 364323 [details]
my test

here are my test files.

Wellington.
Alias: websocket
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!
(Assignee)

Comment 11

8 years ago
Created attachment 370763 [details] [diff] [review]
working patch

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.
Attachment #364317 - Attachment is obsolete: true
Attachment #364323 - Attachment is obsolete: true
Attachment #370763 - Flags: review?(Olli.Pettay)
Yes, once the patch is reviewed and landed this would be included in nightlies.
(Assignee)

Comment 13

8 years ago
Created attachment 370769 [details]
test files
(Assignee)

Comment 14

8 years ago
Created attachment 370772 [details] [diff] [review]
working patch (small merge problem)
Attachment #370763 - Attachment is obsolete: true
Attachment #370772 - Flags: review?(Olli.Pettay)
Attachment #370763 - Flags: review?(Olli.Pettay)
Created attachment 371462 [details]
initial comments

Updated

8 years ago
Attachment #370772 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Updated

8 years ago
Attachment #370772 - Flags: review- → review?(cbiesinger)
(Assignee)

Comment 16

8 years ago
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?
(Assignee)

Updated

8 years ago
Attachment #370772 - Flags: review?(cbiesinger) → review?
(Assignee)

Updated

8 years ago
Attachment #370772 - Flags: review?
(Assignee)

Comment 18

8 years ago
Created attachment 376938 [details] [diff] [review]
working patch

fix missing parenthesis in two conditions in a netwerk file.
Attachment #370772 - Attachment is obsolete: true
Attachment #376938 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 19

8 years ago
Created attachment 376971 [details]
apache test

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.

Comment 21

8 years ago
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.
(Assignee)

Comment 22

8 years ago
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 :)
(Assignee)

Comment 23

8 years ago
(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.
(Assignee)

Comment 25

8 years ago
(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.

Comment 26

8 years ago
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.
(Assignee)

Comment 28

8 years ago
Created attachment 380454 [details] [diff] [review]
mochitest working

(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...
Attachment #376938 - Attachment is obsolete: true
Attachment #380454 - Flags: review?(jduell.mcbugs)
Attachment #376938 - Flags: review?(jduell.mcbugs)
Comment on attachment 380454 [details] [diff] [review]
mochitest working

I'll need to look at the httpd.js changes...
Attachment #380454 - Flags: review?(jwalden+bmo)
Depends on: 495877
Attachment #380454 - Flags: review?(jwalden+bmo)
Attachment #380454 - Flags: review?(jduell.mcbugs)
Attachment #380454 - Flags: review-
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.
(Assignee)

Comment 33

8 years ago
>> 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.
(Assignee)

Comment 35

8 years ago
(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.
(Assignee)

Comment 37

8 years ago
Created attachment 383931 [details] [diff] [review]
working patch - with pref

(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.
Attachment #380454 - Attachment is obsolete: true
(Assignee)

Comment 38

8 years ago
(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.
(Assignee)

Comment 39

8 years ago
Should I set the review flag to someone? Who? (Sorry, I forgot to ask this before)
Attachment #383931 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 40

8 years ago
Created attachment 386295 [details] [diff] [review]
last patch merged with head revision
Attachment #383931 - Attachment is obsolete: true
Attachment #386295 - Flags: review?(jduell.mcbugs)
Attachment #383931 - Flags: review?(jduell.mcbugs)
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 44

8 years ago
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 45

8 years ago
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?
(Assignee)

Comment 48

8 years ago
Created attachment 388340 [details] [diff] [review]
working patch

> 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.
Attachment #386295 - Attachment is obsolete: true
Attachment #388340 - Flags: review?(jduell.mcbugs)
Attachment #386295 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

8 years ago
Blocks: 504553
(Assignee)

Comment 49

8 years ago
Created attachment 388994 [details] [diff] [review]
working patch (netwerk linking issue corrected)
Attachment #388340 - Attachment is obsolete: true
Attachment #388994 - Flags: review?(jduell.mcbugs)
Attachment #388340 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 50

8 years ago
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).
(Assignee)

Comment 52

8 years ago
> +#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
(Assignee)

Comment 53

8 years ago
Created attachment 390817 [details] [diff] [review]
patch (comments jduell)
Attachment #388994 - Attachment is obsolete: true
Attachment #390817 - Flags: review?(jduell.mcbugs)
Attachment #388994 - Flags: review?(jduell.mcbugs)

Comment 54

8 years ago
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)
(Assignee)

Comment 57

8 years ago
> 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?
(Assignee)

Comment 58

8 years ago
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.
(Assignee)

Comment 59

8 years ago
> 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.
(Assignee)

Comment 62

8 years ago
> 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?
(Assignee)

Comment 65

8 years ago
(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.
(Assignee)

Comment 68

8 years ago
(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.

Comment 70

8 years ago
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
(Assignee)

Comment 71

8 years ago
> 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?
(Assignee)

Comment 72

8 years ago
ps: My first phrase should be before the Biesinger's phrase, because it was an unrelated subject, sorry.
(Assignee)

Comment 73

8 years ago
Created attachment 402012 [details] [diff] [review]
working-path (with authentication)

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.
Attachment #390817 - Attachment is obsolete: true
Attachment #402012 - Flags: review?(jduell.mcbugs)
Attachment #390817 - Flags: review?(jduell.mcbugs)
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.
Attachment #402012 - Flags: review?(jduell.mcbugs) → review?(jonas)
(Assignee)

Comment 76

8 years ago
Created attachment 403123 [details] [diff] [review]
working patch (complete)

This patch does proxy resolution and it is complete.
Wellington.
Attachment #402012 - Attachment is obsolete: true
Attachment #403123 - Flags: review?(jonas)
Attachment #402012 - Flags: review?(jonas)
(Assignee)

Comment 77

8 years ago
Created attachment 403170 [details] [diff] [review]
working patch (complete)

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.
Attachment #403123 - Attachment is obsolete: true
Attachment #403170 - Flags: review?(jonas)
Attachment #403123 - Flags: review?(jonas)

Updated

8 years ago
blocking2.0: --- → ?
Attachment #403170 - Flags: review?(jonas) → review?(cbiesinger)
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
(Assignee)

Comment 80

7 years ago
(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.
(Assignee)

Comment 82

7 years ago
Created attachment 415025 [details] [diff] [review]
patch sharing auth code

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.
Attachment #415025 - Flags: review?(cbiesinger)

Comment 83

7 years ago
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.
Keywords: dev-doc-needed
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 87

7 years ago
(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.
(Assignee)

Comment 88

7 years ago
(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?
(Assignee)

Comment 91

7 years ago
> 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.

Updated

7 years ago
Assignee: wfernandom2004 → jeremy.orem+bugs

Updated

7 years ago
Assignee: jeremy.orem+bugs → wfernandom2004
Blocks: 537782
Blocks: 537787

Comment 92

7 years ago
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!
Attachment #415025 - Flags: superreview?(cbiesinger)
Attachment #415025 - Flags: review?(cbiesinger)
Attachment #415025 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 96

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

Comment 97

7 years ago
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.
Created attachment 426735 [details]
minor review comments

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.
(Assignee)

Comment 99

7 years ago
Created attachment 426920 [details] [diff] [review]
up-to-date patch

Olli, this patch is up-to-date (parent revision is 51f9d293c939).
Attachment #403170 - Attachment is obsolete: true
Attachment #415025 - Attachment is obsolete: true
Attachment #426920 - Flags: superreview?(cbiesinger)
Attachment #426920 - Flags: review?(Olli.Pettay)
Attachment #403170 - Flags: review?(cbiesinger)
Attachment #415025 - Flags: superreview?(cbiesinger)
Attachment #415025 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 100

7 years ago
Created attachment 426923 [details] [diff] [review]
up-to-date patch (full context)

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.
(Assignee)

Comment 101

7 years ago
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.

Comment 103

7 years ago
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.
Created attachment 426964 [details]
some more comments (mainly about event handling)
 
+  DOM_CLASSINFO_MAP_BEGIN(WebSocket, nsIWebSocket)
+    DOM_CLASSINFO_MAP_ENTRY(nsIWebSocket)
+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget)
+  DOM_CLASSINFO_MAP_END
Should have also nsIDOMNSEventTarget
Created attachment 426991 [details]
comments (handshake phase)

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?)
(Assignee)

Comment 108

7 years ago
> (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.
Created attachment 427173 [details]
comments (mainly about nsWebSocket.cpp)
Whiteboard: [evang-wanted-3.7]
Created attachment 427315 [details]
some more comments

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.

Updated

7 years ago
Attachment #426920 - Flags: review?(Olli.Pettay) → review-

Updated

7 years ago
Attachment #427315 - Attachment is patch: false
(Assignee)

Comment 112

7 years ago
> 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.
Created attachment 427587 [details]
hand-modified patch for nsHttpChannel/nsHttpChannelAuthInternal review

I wonder if there was some way to generate a patch which was easier to review.
Created attachment 427602 [details]
some comments about new networking/auth interfaces
Created attachment 427603 [details]
some comments about new networking/auth interfaces

This is the right file

Updated

7 years ago
Attachment #427602 - Attachment is obsolete: true
(Assignee)

Comment 119

7 years ago
Created attachment 428262 [details] [diff] [review]
updated patch
Attachment #426920 - Attachment is obsolete: true
Attachment #426923 - Attachment is obsolete: true
Attachment #428262 - Flags: superreview?(cbiesinger)
Attachment #428262 - Flags: review?(Olli.Pettay)
Attachment #426920 - Flags: superreview?(cbiesinger)
For some reason I can't 'hg import' or patch -p1 the latest patch.
I get lots of Hunk #X failed errors
(Assignee)

Comment 121

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

Comment 122

7 years ago
Created attachment 428497 [details] [diff] [review]
updated patch

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

Wellington.
Attachment #428262 - Attachment is obsolete: true
Attachment #428497 - Flags: superreview?(cbiesinger)
Attachment #428497 - Flags: review?(Olli.Pettay)
Attachment #428262 - Flags: superreview?(cbiesinger)
Attachment #428262 - Flags: review?(Olli.Pettay)

Comment 123

7 years ago
and you forgot again ;) To save you some hassle: applies to rev 1918f1187eb6
(nsHttpChannel.cpp gets changed a lot)
Created attachment 428799 [details]
few comments
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
(Assignee)

Comment 126

7 years ago
(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.
(Assignee)

Comment 127

7 years ago
(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 :)
(Assignee)

Comment 128

7 years ago
> +  // 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.
(Assignee)

Comment 130

7 years ago
(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.
(Assignee)

Comment 132

7 years ago
(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.

Updated

7 years ago
(Assignee)

Comment 136

7 years ago
> +    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.
(Assignee)

Comment 137

7 years ago
CheckInnerWindowCorrectness::CheckInnerWindowCorrectness ->
nsDOMEventTargetHelper::CheckInnerWindowCorrectness/
(Assignee)

Comment 138

7 years ago
> (...) 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.
(Assignee)

Comment 140

7 years ago
Created attachment 429716 [details] [diff] [review]
updated patch

parent's revision is 4e1b68ecf126
Attachment #428497 - Attachment is obsolete: true
Attachment #429716 - Flags: review?(jduell.mcbugs)
Attachment #428497 - Flags: superreview?(cbiesinger)
Attachment #428497 - Flags: review?(Olli.Pettay)
Created attachment 429731 [details]
some comments

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.
Attachment #429716 - Flags: review+
(Assignee)

Comment 143

7 years ago
> 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.
Attachment #429716 - Flags: superreview?(cbiesinger)
Attachment #429716 - Flags: review?(jduell.mcbugs)
Attachment #429716 - Flags: review?
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.

Comment 150

7 years ago
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. :-)
Created attachment 433190 [details]
review comments for the netwerk/ part of attachment 429716 [details] [diff] [review] (partial)

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.
(Assignee)

Comment 153

7 years ago
(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.
Created attachment 433317 [details]
Review comments for netwerk/protocol/http/src/nsHttpChannelAuthInternal.cpp
(Assignee)

Comment 158

7 years ago
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.
(Assignee)

Comment 161

7 years ago
ok
Created attachment 433424 [details]
review comments for the non-netwerk/ parts

Here you go.
Attachment #429716 - Flags: superreview?(cbiesinger) → superreview-
(Assignee)

Comment 163

7 years ago
Created attachment 437475 [details]
some comments and questions

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

Updated

7 years ago
Attachment #437475 - Attachment mime type: application/octet-stream → text/plain
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.
(Assignee)

Comment 175

7 years ago
Created attachment 437723 [details] [diff] [review]
updated patch

Parent's revision is 2c44a32052ad
Attachment #429716 - Attachment is obsolete: true
Attachment #429716 - Flags: review?
(Assignee)

Comment 176

7 years ago
Created attachment 437724 [details]
other comments

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".

Updated

7 years ago
Attachment #437724 - Flags: feedback?(cbiesinger)
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
Attachment #437724 - Flags: feedback?(cbiesinger)
(Assignee)

Comment 179

7 years ago
Comment on attachment 437475 [details]
some comments and questions

you forgot this one.
Attachment #437475 - Flags: feedback?(cbiesinger)
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 ;)
Attachment #437475 - Flags: feedback?(cbiesinger)
(Assignee)

Comment 182

7 years ago
> 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.
(Assignee)

Comment 185

7 years ago
> 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.
(In reply to comment #185)
> > 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.
Well, you can actually dispatch the event, but it might be so late in 
window destruction phase that nothing can really handle it.
This all sounds a lot more like an implementation detail than something for 
a spec, IMO. XMLHttpRequest has just the same problem.

Though, perhaps this all could be defined somewhere both for XMLHttpRequest, 
websockets, etc, if needed.
(In reply to comment #182)
> But when mProxyConfig == eProxyConfig_Direct CanUseProxy will not be ever
> called.

Hmm, you're right. The behaviour was still correct though since filters were applied by the caller. In your version of the patch though, the caller didn't apply filters, so that was wrong.

> > 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.

Saw the email, thanks.

> > 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"?

I don't really have an opinion on that... whatever's easier to implement.
(Assignee)

Comment 188

7 years ago
Comment on attachment 437723 [details] [diff] [review]
updated patch

I've updated and splited the patch. The parent's revision of the following three patches is 020a0670ef30. The sequence to apply them is content refactoring, netwerk refactoring and websocket patch.
Attachment #437723 - Attachment is obsolete: true
(Assignee)

Comment 189

7 years ago
Created attachment 441314 [details] [diff] [review]
content refactoring patch
Attachment #441314 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 190

7 years ago
Created attachment 441315 [details] [diff] [review]
netwerk refactoring patch

> The behaviour was still correct though since filters were applied by the caller.
I've realized that the filters weren't been applied by websockets. And there was already a lot of redundant code... Because of this I decided to add three new flags to nsIProtocolProxyService that could be used when resolving proxies. Doing it I could remove the redundant code and let all proxy stuff inside the netwerk module. Biesi, please, check if it is ok to you.
Attachment #441315 - Flags: review?(cbiesinger)
(Assignee)

Comment 191

7 years ago
Created attachment 441316 [details] [diff] [review]
websocket patch

> Perhaps Close could check if window is still "alive".
Olli, I've put again the document check before dispatching events. This is the simplest and most effective way I've found to do it. Do you have any other suggestion?
Attachment #441316 - Flags: superreview?(cbiesinger)
Attachment #441316 - Flags: review?(Olli.Pettay)
Why mOwner->CheckInnerWindowCorrectness() isn't enough?
(Assignee)

Comment 193

7 years ago
(In reply to comment #192)
> Why mOwner->CheckInnerWindowCorrectness() isn't enough?

Because it only checks if the current inner window of the mOwner's outer window is actually the mOwner. Apparently, when the outer window is closed it clears the document, but not its inner window.
Comment on attachment 441314 [details] [diff] [review]
content refactoring patch


> nsresult
> nsXMLHttpRequest::CheckChannelForCrossSiteRequest(nsIChannel* aChannel)
> {
>   nsresult rv;
> 
>-  // First check if this is a same-origin request, or if cross-site requests
>-  // are enabled.
>-  if ((mState & XML_HTTP_REQUEST_XSITEENABLED) ||
>-      CheckMayLoad(mPrincipal, aChannel)) {
>+  // First check if cross-site requests are enabled
>+  if ((mState & XML_HTTP_REQUEST_XSITEENABLED)) {
>+    return NS_OK;
>+  }
>+
>+  // or if this is a same-origin request.
>+  NS_ASSERTION(!nsContentUtils::IsSystemPrincipal(mPrincipal),
>+               "Shouldn't get here!");
>+  if (nsContentUtils::CheckMayLoad(mPrincipal, aChannel)) {
>     return NS_OK;
>   }
I wonder why this change. Why not just add nsContentUtils::
to the CheckMa.yLoad?
Perhaps this change was requested in some comment, but I don't
recall now where.
Attachment #441314 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 195

7 years ago
It is because the original CheckMayLoad func from nsXMLHttpRequest.cpp was first asserting if its nsIPrincipal parameter wasn't the System Principal. That assertion was so specific to nsXMLHttpRequest. So, when I moved this function to nsContentUtils.cpp I had to remove that assertion.
(Assignee)

Comment 196

7 years ago
Hmm, I've just remembered. I've done this change when implementing server-sent events. I've reused some code from there when I started this bug. In websockets this change isn't used, actually. If you want I can remove this modification.
Now that I see the reason for the change, no need to change that part of the patch.
Comment on attachment 441315 [details] [diff] [review]
netwerk refactoring patch

Good idea, I like it. Just a few minor comments:

+++ b/netwerk/base/public/nsIProtocolProxyService.idl
+    const unsigned long RESOLVE_PREFERRING_THE_SOCKS_PROXY = 1 << 1;

I think a better name would be:
 RESOLVE_PREFER_SOCKS_PROXY

and for the others RESOLVE_IGNORE_URI_SCHEME, RESOLVE_PREFER_HTTPS_PROXY.

If PREFER_HTTPS includes IGNORE_URI_SCHEME, shouldn't SOCKS do so too?

+++ b/netwerk/base/src/nsProtocolProxyService.cpp
+        !mSOCKSProxyHost.IsEmpty() &&mSOCKSProxyPort > 0) {

please add a space after &&

+++ b/netwerk/base/src/nsProtocolProxyService.h
+    static PRInt32               PROXYCONFIG_DIRECT4X;
+    static PRInt32               PROXYCONFIG_COUNT;

Just make these static consts in the .cc file

+++ b/netwerk/protocol/http/public/nsIHttpAuthenticableChannel.idl
+     * If the channel being authenticated is under SSL.

under -> using, perhaps?

+     * It returns if the proxy http method used is the CONNECT. If no proxy is
+     * being used it must return PR_FALSE.


Returns if the proxy HTTP method used is CONNECT. If no proxy is  being used it must return PR_FALSE.

+     * It sets the Proxy-Authorization request header. An empty string 
+     * will clear it.

s/It//

+     * It sets the Authorization request header. An empty string
+     * will clear it.

s/It//

+     * It is called asynchronously from
+     * nsIHttpChannelAuthProvider::processAuthentication if it returns
+     * NS_ERROR_IN_PROGRESS.

Maybe change the second "it" to "that method", to make it a little clearer that the two "it" in this sentence refer to different methods.

Similar for onAuthCancelled.

I know that I suggested that wording... sorry for asking you to change it again, but I think that does make the comment clearer.
Attachment #441315 - Flags: review?(cbiesinger) → review-
Comment on attachment 441316 [details] [diff] [review]
websocket patch

+++ b/content/base/src/nsWebSocket.cpp
+  // If aAsUTF8 is false then aData is encoded as US-ASCII.
+  nsresult PostMessage(const nsString& aMessage);

The comment seems outdated now :)

+  // TIMEOUT_TRY_CONNECT_AGAIN miliseconds.

mili -> milli

+  mLockDisconnect("Disconnect lock"),
+  mLockOutgoingMessages("Outgoing messages lock"),
+  mLockReceivedMessages("Received messages lock"),

please add "WebSocket" to the names of the mutexes

   ret = nsTextFormatter::ssprintf(strRequestTmp,
                                   NS_LITERAL_STRING("Host: %s:%lu\r\n").get(),
                                   mOwner->mAsciiHost.get(), mOwner->mPort);
   CHECK_TRUE_AND_FAIL_IF_FAILED(ret != (PRUint32)-1);
   // nsTextFormatter::ssprintf gives us some extras \0...
-  strRequest.Append(nsDependentString(strRequestTmp.get()));
+  buf->Append(NS_ConvertUTF16toUTF8(nsDependentString(strRequestTmp.get())));

I'd avoid the conversion/nsTextFormatter and just assemble the header with some AppendLiteral/Append/AppendInt calls, like you do for Origin and others.

+       nsContentUtils::GetDocumentFromScriptContext(mOwner->mScriptContext);  

there's trailing whitespace on this line, please remove

+    message.AppendLiteral("Failed to establish connection from ");
+    message.Append(NS_ConvertUTF8toUTF16(sourceSpec));
+    message.AppendLiteral(" to ");
+    message.Append(NS_ConvertUTF8toUTF16(targetSpec));
+    console->LogStringMessage(message.get());

shouldn't that be localizable?

+    mSocketThread = nsnull;

you also need to ->Shutdown() the thread
Attachment #441316 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 200

7 years ago
(In reply to comment #198)
> If PREFER_HTTPS includes IGNORE_URI_SCHEME, shouldn't SOCKS do so too?
No, I think it shouldn't. Actually, the socks condition (in nsProtocolProxyService.cpp) doesn't check the uri's scheme. Also, in this patch (in nsWebSocket.cpp), whenever RESOLVE_PREFER_SOCKS_PROXY is used the IGNORE_URI_SCHEME is used together. However, I think that future codes can prefer getting its scheme specific to the main http one when they can't get a SOCKS proxy (because it isn't configured, or failed, etc). What do you think about this possibility?

> I know that I suggested that wording... sorry for asking you to change it
> again, but I think that does make the comment clearer.
No problem.

> shouldn't that be localizable?
Yes... but is it really necessary? I'm asking this because I use the pt-br version of Firefox, and usually I get a lot of error/warnings the console in english.

> you also need to ->Shutdown() the thread
I tried this yesterday. But I got some assertions because events dispatched to the thread. I didn't check it, but now I think it was because the close event. I will take a look at that.
(In reply to comment #200)
> (In reply to comment #198)
> > If PREFER_HTTPS includes IGNORE_URI_SCHEME, shouldn't SOCKS do so too?
> No, I think it shouldn't. Actually, the socks condition (in
> nsProtocolProxyService.cpp) doesn't check the uri's scheme. Also, in this patch
> (in nsWebSocket.cpp), whenever RESOLVE_PREFER_SOCKS_PROXY is used the
> IGNORE_URI_SCHEME is used together. However, I think that future codes can
> prefer getting its scheme specific to the main http one when they can't get a
> SOCKS proxy (because it isn't configured, or failed, etc). What do you think
> about this possibility?

Good point. I agree now that it shouldn't include IGNORE_URI_SCHEME.
Oh, for your other questions:

(In reply to comment #200)
> > shouldn't that be localizable?
> Yes... but is it really necessary? I'm asking this because I use the pt-br
> version of Firefox, and usually I get a lot of error/warnings the console in
> english.

Hm, I think those other cases are also bugs. But, I'll leave it to a DOM peer to answer that authoritatively (smaug?)

> > you also need to ->Shutdown() the thread
> I tried this yesterday. But I got some assertions because events dispatched to
> the thread. I didn't check it, but now I think it was because the close event.
> I will take a look at that.

Please do. If you don't shutdown the thread, you'll keep accumulating useless threads, which does not seem ideal.
Comment on attachment 441316 [details] [diff] [review]
websocket patch

Change the pref to false for now, and make the error messages localizable.
Attachment #441316 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 204

7 years ago
Created attachment 442418 [details] [diff] [review]
netwerk refactoring patch
Attachment #441315 - Attachment is obsolete: true
Attachment #442418 - Flags: review?(cbiesinger)
(Assignee)

Comment 205

7 years ago
Created attachment 442419 [details] [diff] [review]
websocket patch

> > you also need to ->Shutdown() the thread
> I tried this yesterday. But I got some assertions because events dispatched to
> the thread. I didn't check it, but now I think it was because the close event.
> I will take a look at that.
Using one thread per ws was raising many assertions when calling Shutdown(). Even 
codes not from the websocket patch was giving assertions (when closing the window).
I'm not sure the reason of that, but it seems that shutdown() tries to sync also
the main thread's events, and that isn't sometimes expected...  Well,  I've
changed the code to use only one global thread (like the netwerk module does),
and the problem has been solved.

The parent's revision is still the 020a0670ef30 one.
Attachment #441316 - Attachment is obsolete: true
Attachment #442419 - Flags: superreview?(cbiesinger)
Attachment #442419 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Blocks: 562681

Comment 206

7 years ago
While i had luck building a former Version for Linux, it failed for a Windows build with VS2005SP1.

in nsWebSocket.cpp:1944 i got a error C2784, some template param mismatch, __thiscall expected, but got __stdcall (forgot to save the compiler output)

  nsCOMPtr<nsIRunnable> event =
   NS_NEW_RUNNABLE_METHOD(nsWebSocket, mOwner, Close);

i guess it should rather be something like this:

  nsCOMPtr<nsIRunnable> event =
   NS_NEW_RUNNABLE_METHOD(nsWebSocketEstablishedConnection, this, CloseOwner);
NS_NEW_RUNNABLE_METHOD should be changed to use NS_NewRunnableMethod
Comment on attachment 442419 [details] [diff] [review]
websocket patch

If you update the patch to use NS_NewRunnableMethod, r=me.
Attachment #442419 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 442418 [details] [diff] [review]
netwerk refactoring patch

(In reply to comment #198)
> +++ b/netwerk/base/src/nsProtocolProxyService.h
> +    static PRInt32               PROXYCONFIG_DIRECT4X;
> +    static PRInt32               PROXYCONFIG_COUNT;
> 
> Just make these static consts in the .cc file


What I meant was, don't declare them in the class at all and make them global constants, i.e.:
static const PRInt32 PROXYCONFIG_DIRECT4X = 3;
static const PRInt32 PROXYCONFIG_COUNT = 6;


Why did you add those Cancel calls in nsHttpChannelAuthProvider.cpp? AFAICT they are not needed, am I wrong?
Comment on attachment 442419 [details] [diff] [review]
websocket patch

+nsIThread *gWebSocketThread = nsnull;

should be static

+  nsCOMPtr<nsIConsoleService>
+    console(do_GetService("@mozilla.org/consoleservice;1", &rv));

Normally the console( would be put on the first line, i.e.:

  nsCOMPtr<nsIConsoleService> console(
    do_GetService("@mozilla.org/consoleservice;1", &rv));


+  const PRUnichar *formatStrings[] =
+    { NS_ConvertASCIItoUTF16(targetSpec).get() };

This will access free'd memory. You need something like:
  NS_ConvertASCIItoUTF16 specUTF16(targetSpec);
  const PRUnichar *formatStrings[] = { specUTF16.get() };

I think you also want UTF8 instead of ASCII.

+    NS_RELEASE(gWebSocketThread);
+    gWebSocketThread = nsnull;

NS_RELEASE already sets the variable to null, so you don't need the second line.
Attachment #442419 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 211

7 years ago
(In reply to comment #209)
> What I meant was, don't declare them in the class at all and make them global
> constants, i.e.:
> static const PRInt32 PROXYCONFIG_DIRECT4X = 3;
> static const PRInt32 PROXYCONFIG_COUNT = 6;
ok.

> Why did you add those Cancel calls in nsHttpChannelAuthProvider.cpp? AFAICT
> they are not needed, am I wrong?
Actually nsHttpChannel calls the nsHttpChannelAuthProvider->cancel() method
inside its Cancel() method (and because of that really it isn't need). However
I think that the auth implementation is the one that should cancel the prompt
when needed.
At those two points in the code, an auth prompt should not be up, right?
(Assignee)

Comment 213

7 years ago
(In reply to comment #212)
> At those two points in the code, an auth prompt should not be up, right?

Hmm, right. So, I will let as it was.
Comment on attachment 442418 [details] [diff] [review]
netwerk refactoring patch

r=biesi with those two changes
Attachment #442418 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 215

7 years ago
Created attachment 444192 [details] [diff] [review]
content refactoring patch

just updating to the trunk
Attachment #441314 - Attachment is obsolete: true
Attachment #444192 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 216

7 years ago
Created attachment 444193 [details] [diff] [review]
netwerk refactoring patch
Attachment #442418 - Attachment is obsolete: true
Attachment #444193 - Flags: review?(cbiesinger)
(Assignee)

Comment 217

7 years ago
Created attachment 444196 [details] [diff] [review]
websocket patch

Parent's revision of this set of patches is 386cd7683b91.

I would like to start the implementation of the v76 (see bug 562681). If there is no more comments on this bug I will start that one :)
Attachment #442419 - Attachment is obsolete: true
Attachment #444196 - Flags: superreview?(cbiesinger)
Attachment #444196 - Flags: review?(Olli.Pettay)

Comment 218

7 years ago
Are these going to get pushed before work is completed on bug 562681?
Wellington, just go ahead with v76.
I'll push the patch to tryserver tomorrow, and if everything looks ok there, 
and biesi (or I) doesn't find anything to fix in the patches I'll push the patch 
to m-c. 
bug 562681 could hopefully happen soon after that.
Attachment #444196 - Flags: superreview?(cbiesinger) → superreview+
Attachment #444193 - Flags: review?(cbiesinger) → review+

Updated

7 years ago
Attachment #444192 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 444196 [details] [diff] [review]
websocket patch


>+NS_IMETHODIMP
>+nsWebSocket::Initialize(nsISupports* aOwner,
>+                        JSContext* cx,
>+                        JSObject* obj,
>+                        PRUint32 argc,
>+                        jsval* argv)
>+{
>+  nsAutoString urlParam, protocolParam;
>+
>+  PRBool prefEnabled =
>+    nsContentUtils::GetBoolPref("network.websocket.enabled", PR_TRUE);
>+  if (!prefEnabled) {
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }
>+
>+  if (argc != 1 && argc != 2) {
>+    return NS_ERROR_DOM_SYNTAX_ERR;
>+  }
>+
>+  JSString* jsstr = JS_ValueToString(cx, argv[0]);
I think you should have JSAutorequest right before this.
Attachment #444196 - Flags: review?(Olli.Pettay) → review+
I uploaded the patches to tryserver.
The patch seems to build ok
https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-try-cc2bdc16c309/
And no test failures.

Comment 223

7 years ago
I tested the try build using Jetty test application (a simple chat), and it fails to create the WebSocket with :
Error: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "http://l-at12128:8081/ws/ Line: 18"]

The websocket location used is ws://l-at12128:8081/ws/

0017:         var location = document.location.toString().replace('http:','ws:');
0018:         this._ws=new WebSocket(location);

Comment 224

7 years ago
Ignore the previous comment, I had not set the network.websocket.enabled preference to "true". 

It works !
(Assignee)

Comment 225

7 years ago
Created attachment 444645 [details] [diff] [review]
websocket patch

> I think you should have JSAutorequest right before this.
Done. Are you going to push it to m-c?
Attachment #444196 - Attachment is obsolete: true
Attachment #444645 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 226

7 years ago
Created attachment 444647 [details] [diff] [review]
websocket patch

sorry, wrong patch.
Attachment #444645 - Attachment is obsolete: true
Attachment #444647 - Flags: review?(Olli.Pettay)
Attachment #444645 - Flags: review?(Olli.Pettay)
Comment on attachment 444647 [details] [diff] [review]
websocket patch

Yeah, I'll try to push this to m-c asap.

We need tests, but IMO it makes more sense to have those once the protocol has been updated.

(The fun part is that there is now another proposal for the protocol, but I think we should still implement the v76, at least for now.)
Attachment #444647 - Flags: review?(Olli.Pettay) → review+
We need tests for a feature that we add.  If the protocol is done enough for us to implement, it's done enough for us to add tests for our current implementation.  This is a basic tenet of how we build software at Mozilla these days, and I really don't think that we should be pushing a new web-facing feature without appropriate test coverage.  How will we know that we didn't break things when we're updating to the new protocol?

I'm presuming that the paths affected by the content and netwerk patches were already covered by existing tests, since none were added.  But presumably the refactoring was to permit new kinds of callers, so how do we know that those kinds of callers work?  How should someone working on a patch to network or content make sure that they didn't break websockets?

There are references above to WebKit's tests, and Jason pretty clearly indicated that we should have some tests for this as well.  It looks like Jeff's changes to httpd.js to make it suitable for this work didn't happen, so forking httpd.js to websocketd.js and making it do what you want is probably the right basis.  (I don't share Jeff's need for perfection in all things, so even though we can't test all possible client errors with httpd.js, I think that it could still be a basis for meaningful coverage of the code.  But I wouldn't get held up by polishing httpd.js patches to Jeff's satisfaction, honestly, I'd just fork to get the test tool you need and go.)
Ok. I won't push the patch to m-c then. Even pref'ed off.

Comment 230

7 years ago
(In reply to comment #229)
> Ok. I won't push the patch to m-c then. Even pref'ed off.

Some manual testing would be good too. I'd vote for an off-by default push.
If it went through try-server, we should have some builds available for people to test with (and we could put out a call to a wider audience than just our usual nightly testers, even) -- can we do that, while we get the tests from this bug repackaged, or WebKit's tests ported, or httpd.js forked, or whatever we need to have some coverage before we land?

This is great work, Wellington!  Thanks for doing it!
(Assignee)

Comment 232

7 years ago
> This is great work, Wellington!  Thanks for doing it!
You're welcome.

About the tests, I can provide the WebSockets tests if the test infra supports the protocol. Could someone else write the needed netwerk and content refactoring tests?

In bug 546616 I've uploaded a patch that makes httpd.js support ws. However that patch is just a hack I've done. Some help there would be appreciated.
Existing tests certainly do test content refactoring parts. All the
XHR tests use that code. So I don't think any new tests are required for that.
Clint may be able to help expedite the necessary infrastructure work, copying him here.

Comment 235

7 years ago
(In reply to comment #234)
> Clint may be able to help expedite the necessary infrastructure work, copying
> him here.

I'm looking over everything, gonna get a couple ideas together tomorrow and report back.

Comment 236

7 years ago
There are three basic approaches here:
1. use a third party websocket server
2. modify httpd.js to support this
3. fork httpd.js to build our own websocket server

So, I want to take option 3 off the table right now.  I tend to think that building our own websockets server when we already have a bunch of modules that are already built and are actively being developed would be a waste of our time.  (There are a whole bunch of existing websocket implementations out there, but pywebsocket looks like the best, keep reading).

That leaves us with 1 & 2.  I think 1 is the best option here.  We can use something like pywebsocket[1] configured in standalone mode to run locally alongside the httpd.js server that we always run when we run mochitest.  It would run on a different port but we could use the existing PAC code inside the mochitest profile to proxy ws requests to the ws server, just like we currently do for the http server, see: http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#386

The "server side" tests would be in the tree and would be coded in python and configured as handlers for pywebsocket.  You can see their example "echo" server to get an idea of the required code: http://code.google.com/p/pywebsocket/source/browse/trunk/src/example/echo_wsh.py

This has a couple of benefits:
* currently pywebsocket has been proposed for inclusion as a module for Apache to support websockets [2].  I think that's important because Apache is widely deployed and we could be testing something very similar to what will be deployed "in the wild".  Also, we have other people besides us who are working to keep that code base current w.r.t. changes in the specification and bug fixes.
* it keeps our http test server focused on doing http traffic.  Httpd.js is already a complicated piece of machinery.  I'd prefer to keep it specific to our http needs.  Having it do more than that is going to be a maintenance headache.

And a few downsides:
* we have to run a second server with mochitest, which is kind of a pain.  We have to put pywebsocket in our tree, package it, and deploy it when mochitest is started.
* the server-side of the tests for this will need to land (after packaging) with the websocket server and will be in a different language from the tests themselves.  (python versus javascript, respectively).

Option 2 is attractive because of its relative straightforwardness in the short term.  You can see a prospective patch over on bug 546616.  However, this adds yet more complexity to httpd.js and to all our proxy code throughout the mochitest stack.  It feels to me that this could come back around to bite us in the long term in terms of making the "websocket" bits of httpd.js harder to keep current with the specification, and in terms of overall fragility from accidentally introducing bugs into httpd.js when trying to add/fix features for ws support.  

So, my vote is for option 1.  I think once we have a decision on this we ought to morph bug 546616 into the vessel for this code in order to keep this bug focused on the ws implementation details.

[1]: http://code.google.com/p/pywebsocket/
[2]: https://issues.apache.org/bugzilla/show_bug.cgi?id=47485
Nice summary, Clint.  I agree that the pywebsocket solution (option #1) sounds overall like the best route.
My vote would be for option 1 as well, it may take some time to get all the bits and pieces figured out to get a third party server to run in our system, but that should be a one time cost vs the added burden of making httpd.js even harder to work on.
OK.  Marking this as blocking 1.9.3beta1+, BUT!!!  If it's not finished for beta1, then we will likely remove blocking status.
blocking2.0: ? → beta1+

Comment 240

7 years ago
I've given Jonathan (jgriffin) my notes on this, and he's going to start working on the test automation side of this next week, as his current project starts to end.
(Assignee)

Comment 241

7 years ago
Just forwarding. Chromium has updated its protocol implementation.
http://blog.chromium.org/2010/06/websocket-protocol-updated.html
I'm currently working on adding support for this to mochitest.  I assume that in order to test WebSocket support, I need to apply all three patches (content, netwerk, and websockets).  However, only content applies cleanly to m-c, the other two do not.  Wellington and/or Olli, can you update the patches so I can use them in testing the necessary mochitest changes?
(Assignee)

Comment 243

7 years ago
Created attachment 449370 [details] [diff] [review]
netwerk refactoring patch
Attachment #444193 - Attachment is obsolete: true
(Assignee)

Comment 244

7 years ago
Created attachment 449371 [details] [diff] [review]
websocket patch

> Wellington and/or Olli, can you update the patches so I can
> use them in testing the necessary mochitest changes?
They are here. The parent's revision is 661a75f69012
Attachment #444647 - Attachment is obsolete: true
Depends on: 570789
(Assignee)

Comment 245

7 years ago
Created attachment 451983 [details] [diff] [review]
netwerk refactoring patch

netwerk refactoring patch updated to the trunk (rev bc28b9c1545d)
Attachment #449370 - Attachment is obsolete: true

Updated

7 years ago
Depends on: 572975
http://hg.mozilla.org/mozilla-central/rev/fb11e27cda71
http://hg.mozilla.org/mozilla-central/rev/64a60466f0d4
http://hg.mozilla.org/mozilla-central/rev/bcd52abd2495
http://hg.mozilla.org/mozilla-central/rev/2717d84e1537
Marking this fixed.
If you find any bugs in the implementation, please file followup bugs
and make them block this bug. No need to report problems in this bug (this bug
is long enough now).
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Depends on: 574897
Depends on: 575701

Updated

7 years ago
No longer depends on: 495877
Target Milestone: --- → mozilla2.0b1
What is the status of WebSockets for Firefox 4? I've heard conflicting information, from "it's completely out" to "it'll be prefixed" to "it's totally in", often from the same people. I need to know what the real situation is so I can get it documented to the appropriate degree.
It will (probably ) be prefixed: bug 602028
(In reply to comment #250)
> It will (probably ) be prefixed: bug 602028

I wouldn't go that far, but someone needs to make a decisive statement about prefixing. Let's discuss prefixing pros and cons more over in that bug.

/be
Keywords: dev-doc-needed
I can't seem to get Websockets to work at all.
This page is saying to me that Websockets is not supported in my browser, using Firefox 6 and Firefox trunk: http://jimbergman.net/websocket-web-browser-test/

This doesn't work either: http://websocket.org/echo.html

network.websocket.enabled is true in about:config

According to https://developer.mozilla.org/en/WebSockets , MozWebSocket should be used? But bug 602028 was wontfixed, no? A quick test alert(window.MozWebSocket) seems to work. But according to bug 616733, this is disabled?
See https://bugzilla.mozilla.org/show_bug.cgi?id=659324

Comment 254

6 years ago
Hello,
FF cannot connect if SSL is used (wss), I have no problem with no-ssl connection, I have no problem to connect to SSL based server using Chrome or my own client, but using FF I get "error:14094412:SSL routines:SSL3_READ_BYTES:sslv3 alert bad certificate" error, I guess it may be because I have my own certificate (no authority), but if so, no error should be thrown... there is no GUI interface to accept such certifikate, so connection should be accepted automatically
(In reply to Bronislav Klučka from comment #254)
> Hello,
> FF cannot connect if SSL is used (wss), I have no problem with no-ssl
> connection, I have no problem to connect to SSL based server using Chrome or
> my own client, but using FF I get "error:14094412:SSL
> routines:SSL3_READ_BYTES:sslv3 alert bad certificate" error, I guess it may
> be because I have my own certificate (no authority), but if so, no error
> should be thrown... there is no GUI interface to accept such certifikate, so
> connection should be accepted automatically

you are correct that you cannot connect over ssl without a valid cert. We treat it just like other sub-resources (e.g. img).

You could get a cert with a recognized signature, ( http://cert.startcom.org/ ) or you could add your cert through the firefox preferences, or you can access an html page on the same server where a gui will pop up allowing you to add an exception for that cert - that exception will get cached and be valid for websockets.

Comment 256

6 years ago
(In reply to Patrick McManus from comment #255)
> 
> you are correct that you cannot connect over ssl without a valid cert. We
> treat it just like other sub-resources (e.g. img).
> 
> You could get a cert with a recognized signature, (
> http://cert.startcom.org/ ) or you could add your cert through the firefox
> preferences, or you can access an html page on the same server where a gui
> will pop up allowing you to add an exception for that cert - that exception
> will get cached and be valid for websockets.

I've added the certificate for 443 port (https) for visiting the page, I've added the certificate fo 81 port (where wss is running), I'm still getting the same error

Comment 257

6 years ago
Firefox 6 does not expose close code and close reason in close event.
Firefox 6 does handle binary content.

Comment 258

6 years ago
Firefox 6 does not handle binary content.

(In reply to Bronislav Klučka from comment #257)
> Firefox 6 does not expose close code and close reason in close event.
> Firefox 6 does handle binary content.
No longer blocks: 504553
You need to log in before you can comment on or make changes to this bug.