Last Comment Bug 472529 - (websocket) Support for Web sockets' HTML5 Draft Recommendation (websocket)
(websocket)
: Support for Web sockets' HTML5 Draft Recommendation (websocket)
Status: RESOLVED FIXED
[evang-wanted-3.7]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- enhancement with 41 votes (vote)
: mozilla2.0b1
Assigned To: Wellington Fernando de Macedo
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 570789 572975 574897 575701
Blocks: 537782 537787 562681
  Show dependency treegraph
 
Reported: 2009-01-07 11:00 PST by Wellington Fernando de Macedo
Modified: 2012-07-26 20:10 PDT (History)
89 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+


Attachments
initial work (71.69 KB, patch)
2009-01-26 11:46 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
work in progress (86.02 KB, patch)
2009-02-14 04:14 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
work in progress (99.63 KB, patch)
2009-02-22 06:34 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
working patch (111.03 KB, patch)
2009-02-26 07:52 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
my test (30.75 KB, application/zip)
2009-02-26 08:16 PST, Wellington Fernando de Macedo
no flags Details
working patch (144.81 KB, patch)
2009-04-02 18:26 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
test files (44.36 KB, application/zip)
2009-04-02 19:18 PDT, Wellington Fernando de Macedo
no flags Details
working patch (small merge problem) (144.84 KB, patch)
2009-04-02 19:22 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
initial comments (6.14 KB, text/plain)
2009-04-07 09:25 PDT, Olli Pettay [:smaug]
no flags Details
working patch (144.86 KB, patch)
2009-05-12 08:26 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
apache test (22.16 KB, application/x-zip-compressed)
2009-05-12 12:19 PDT, Wellington Fernando de Macedo
no flags Details
mochitest working (182.89 KB, patch)
2009-05-29 09:37 PDT, Wellington Fernando de Macedo
jwalden+bmo: review-
Details | Diff | Review
working patch - with pref (150.93 KB, patch)
2009-06-18 08:06 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
last patch merged with head revision (127.31 KB, patch)
2009-07-01 09:04 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
working patch (130.99 KB, patch)
2009-07-13 15:23 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
working patch (netwerk linking issue corrected) (132.34 KB, patch)
2009-07-16 14:01 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
patch (comments jduell) (133.23 KB, patch)
2009-07-27 08:07 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
working-path (with authentication) (195.18 KB, patch)
2009-09-21 19:40 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
working patch (complete) (194.06 KB, patch)
2009-09-27 10:46 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
working patch (complete) (201.37 KB, patch)
2009-09-27 20:18 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
patch sharing auth code (473.60 KB, patch)
2009-11-29 14:25 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
minor review comments (6.84 KB, text/plain)
2010-02-12 13:31 PST, Olli Pettay [:smaug]
no flags Details
up-to-date patch (489.68 KB, patch)
2010-02-14 13:57 PST, Wellington Fernando de Macedo
bugs: review-
Details | Diff | Review
up-to-date patch (full context) (659.44 KB, patch)
2010-02-14 14:02 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
some more comments (mainly about event handling) (9.70 KB, text/plain)
2010-02-15 04:25 PST, Olli Pettay [:smaug]
no flags Details
comments (handshake phase) (3.78 KB, text/plain)
2010-02-15 09:15 PST, Olli Pettay [:smaug]
no flags Details
comments (mainly about nsWebSocket.cpp) (15.11 KB, text/plain)
2010-02-16 11:47 PST, Olli Pettay [:smaug]
no flags Details
some more comments (3.88 KB, text/plain)
2010-02-17 05:57 PST, Olli Pettay [:smaug]
no flags Details
hand-modified patch for nsHttpChannel/nsHttpChannelAuthInternal review (148.46 KB, text/plain)
2010-02-18 07:37 PST, Olli Pettay [:smaug]
no flags Details
some comments about new networking/auth interfaces (655.30 KB, text/plain)
2010-02-18 09:14 PST, Olli Pettay [:smaug]
no flags Details
some comments about new networking/auth interfaces (813 bytes, text/plain)
2010-02-18 09:19 PST, Olli Pettay [:smaug]
no flags Details
updated patch (474.17 KB, patch)
2010-02-22 11:57 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
updated patch (474.61 KB, patch)
2010-02-23 12:00 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
few comments (5.45 KB, text/plain)
2010-02-24 13:28 PST, Olli Pettay [:smaug]
no flags Details
updated patch (481.56 KB, patch)
2010-03-02 05:55 PST, Wellington Fernando de Macedo
bugs: review+
cbiesinger: superreview-
Details | Diff | Review
some comments (3.98 KB, text/plain)
2010-03-02 07:41 PST, Olli Pettay [:smaug]
no flags Details
review comments for the netwerk/ part of attachment 429716 (partial) (6.71 KB, text/plain)
2010-03-17 15:46 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details
Review comments for netwerk/protocol/http/src/nsHttpChannelAuthInternal.cpp (1.77 KB, text/plain)
2010-03-18 06:07 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details
review comments for the non-netwerk/ parts (10.98 KB, text/plain)
2010-03-18 14:33 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details
some comments and questions (3.06 KB, text/plain)
2010-04-06 18:25 PDT, Wellington Fernando de Macedo
no flags Details
updated patch (483.57 KB, patch)
2010-04-07 17:57 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
other comments (1.66 KB, text/plain)
2010-04-07 17:58 PDT, Wellington Fernando de Macedo
no flags Details
content refactoring patch (22.22 KB, patch)
2010-04-24 16:44 PDT, Wellington Fernando de Macedo
bugs: review+
Details | Diff | Review
netwerk refactoring patch (354.15 KB, patch)
2010-04-24 16:52 PDT, Wellington Fernando de Macedo
cbiesinger: review-
Details | Diff | Review
websocket patch (111.86 KB, patch)
2010-04-24 16:57 PDT, Wellington Fernando de Macedo
bugs: review+
cbiesinger: superreview+
Details | Diff | Review
netwerk refactoring patch (354.14 KB, patch)
2010-04-29 09:43 PDT, Wellington Fernando de Macedo
cbiesinger: review+
Details | Diff | Review
websocket patch (112.39 KB, patch)
2010-04-29 09:45 PDT, Wellington Fernando de Macedo
bugs: review+
cbiesinger: superreview+
Details | Diff | Review
content refactoring patch (22.24 KB, patch)
2010-05-07 15:13 PDT, Wellington Fernando de Macedo
bugs: review+
Details | Diff | Review
netwerk refactoring patch (355.96 KB, patch)
2010-05-07 15:15 PDT, Wellington Fernando de Macedo
cbiesinger: review+
Details | Diff | Review
websocket patch (112.38 KB, patch)
2010-05-07 15:20 PDT, Wellington Fernando de Macedo
bugs: review+
cbiesinger: superreview+
Details | Diff | Review
websocket patch (112.40 KB, patch)
2010-05-11 06:14 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
websocket patch (112.40 KB, patch)
2010-05-11 06:18 PDT, Wellington Fernando de Macedo
bugs: review+
Details | Diff | Review
netwerk refactoring patch (356.27 KB, patch)
2010-06-04 15:54 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
websocket patch (112.60 KB, patch)
2010-06-04 15:56 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
netwerk refactoring patch (355.53 KB, patch)
2010-06-17 10:13 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review

Description Wellington Fernando de Macedo 2009-01-07 11:00:15 PST
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 Olli Pettay [:smaug] 2009-01-08 09:04:35 PST
WebSockets API may merge with or replace http://dev.w3.org/2006/webapi/network-api/network-api.html.
Comment 2 Jonas Sicking (:sicking) 2009-01-08 10:31:36 PST
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.
Comment 3 Wellington Fernando de Macedo 2009-01-08 10:59:11 PST
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.
Comment 4 Wellington Fernando de Macedo 2009-01-26 11:46:35 PST
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.
Comment 5 Olli Pettay [:smaug] 2009-01-27 06:38:18 PST
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.
Comment 6 Wellington Fernando de Macedo 2009-02-14 04:14:31 PST
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.
Comment 7 Wellington Fernando de Macedo 2009-02-22 06:34:59 PST
Created attachment 363569 [details] [diff] [review]
work in progress

this has the work I've done this week.
Comment 8 Wellington Fernando de Macedo 2009-02-26 07:52:18 PST
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?)
Comment 9 Wellington Fernando de Macedo 2009-02-26 08:16:28 PST
Created attachment 364323 [details]
my test

here are my test files.

Wellington.
Comment 10 Hixie (not reading bugmail) 2009-03-22 22:41:53 PDT
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!
Comment 11 Wellington Fernando de Macedo 2009-04-02 18:26:30 PDT
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.
Comment 12 Jonas Sicking (:sicking) 2009-04-02 18:37:20 PDT
Yes, once the patch is reviewed and landed this would be included in nightlies.
Comment 13 Wellington Fernando de Macedo 2009-04-02 19:18:26 PDT
Created attachment 370769 [details]
test files
Comment 14 Wellington Fernando de Macedo 2009-04-02 19:22:31 PDT
Created attachment 370772 [details] [diff] [review]
working patch (small merge problem)
Comment 15 Olli Pettay [:smaug] 2009-04-07 09:25:48 PDT
Created attachment 371462 [details]
initial comments
Comment 16 Wellington Fernando de Macedo 2009-04-09 09:53:06 PDT
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.
Comment 17 Olli Pettay [:smaug] 2009-05-12 08:01:03 PDT
Jason, could you perhaps look at the networking part?
Comment 18 Wellington Fernando de Macedo 2009-05-12 08:26:03 PDT
Created attachment 376938 [details] [diff] [review]
working patch

fix missing parenthesis in two conditions in a netwerk file.
Comment 19 Wellington Fernando de Macedo 2009-05-12 12:19:49 PDT
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.
Comment 20 Jason Duell [:jduell] (needinfo? me) 2009-05-12 16:16:44 PDT
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 Reid 2009-05-12 16:32:16 PDT
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.
Comment 22 Wellington Fernando de Macedo 2009-05-12 16:41:45 PDT
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 :)
Comment 23 Wellington Fernando de Macedo 2009-05-13 06:49:56 PDT
(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.
Comment 24 Jeff Walden [:Waldo] (remove +bmo to email) 2009-05-20 13:05:34 PDT
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.
Comment 25 Wellington Fernando de Macedo 2009-05-20 13:42:16 PDT
(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 Dmitry Sychov 2009-05-22 01:10:46 PDT
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?
Comment 27 Olli Pettay [:smaug] 2009-05-22 01:18:13 PDT
(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.
Comment 28 Wellington Fernando de Macedo 2009-05-29 09:37:43 PDT
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...
Comment 29 Jeff Walden [:Waldo] (remove +bmo to email) 2009-05-29 11:55:15 PDT
Comment on attachment 380454 [details] [diff] [review]
mochitest working

I'll need to look at the httpd.js changes...
Comment 30 Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-01 15:54:24 PDT
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.
Comment 31 Jonas Sicking (:sicking) 2009-06-01 16:49:30 PDT
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.
Comment 32 Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-01 17:53:06 PDT
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.
Comment 33 Wellington Fernando de Macedo 2009-06-01 18:30:08 PDT
>> 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.
Comment 34 Jason Duell [:jduell] (needinfo? me) 2009-06-10 11:58:20 PDT
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.
Comment 35 Wellington Fernando de Macedo 2009-06-10 12:12:42 PDT
(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.
Comment 36 Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-10 17:31:56 PDT
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.
Comment 37 Wellington Fernando de Macedo 2009-06-18 08:06:32 PDT
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.
Comment 38 Wellington Fernando de Macedo 2009-06-18 08:17:24 PDT
(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.
Comment 39 Wellington Fernando de Macedo 2009-06-18 08:28:05 PDT
Should I set the review flag to someone? Who? (Sorry, I forgot to ask this before)
Comment 40 Wellington Fernando de Macedo 2009-07-01 09:04:00 PDT
Created attachment 386295 [details] [diff] [review]
last patch merged with head revision
Comment 41 Hixie (not reading bugmail) 2009-07-06 20:47:42 PDT
FYI the spec has renamed the postMessage() method to send().
Comment 42 Hixie (not reading bugmail) 2009-07-06 20:59:35 PDT
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).
Comment 43 Hixie (not reading bugmail) 2009-07-06 21:26:53 PDT
FYI the disconnect() method also got renamed to close().
Comment 44 timeless 2009-07-09 07:41:15 PDT
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 timeless 2009-07-09 08:23:50 PDT
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,
Comment 46 Olli Pettay [:smaug] 2009-07-09 08:28:14 PDT
> So from memory this is an enum which is effectively vaguely public,
No it is not.
Comment 47 Jonas Sicking (:sicking) 2009-07-09 09:55:57 PDT
(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?
Comment 48 Wellington Fernando de Macedo 2009-07-13 15:23:38 PDT
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.
Comment 49 Wellington Fernando de Macedo 2009-07-16 14:01:36 PDT
Created attachment 388994 [details] [diff] [review]
working patch (netwerk linking issue corrected)
Comment 50 Wellington Fernando de Macedo 2009-07-18 07:03:43 PDT
I've found this at apache's bugzilla, about Web Sockets:
https://issues.apache.org/bugzilla/show_bug.cgi?id=47485
Comment 51 Jason Duell [:jduell] (needinfo? me) 2009-07-23 11:14:00 PDT
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).
Comment 52 Wellington Fernando de Macedo 2009-07-27 07:06:42 PDT
> +#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
Comment 53 Wellington Fernando de Macedo 2009-07-27 08:07:11 PDT
Created attachment 390817 [details] [diff] [review]
patch (comments jduell)
Comment 54 Takuya Oikawa 2009-08-10 01:19:47 PDT
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 55 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-20 11:51:35 PDT
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.
Comment 56 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-20 12:06:15 PDT
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)
Comment 57 Wellington Fernando de Macedo 2009-08-20 16:12:26 PDT
> 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?
Comment 58 Wellington Fernando de Macedo 2009-08-20 16:21:57 PDT
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.
Comment 59 Wellington Fernando de Macedo 2009-08-20 16:27:01 PDT
> Jonas and I
Jason and I, sorry.
Comment 60 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-20 16:50:51 PDT
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).
Comment 61 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-21 04:26:12 PDT
(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.
Comment 62 Wellington Fernando de Macedo 2009-08-21 06:44:49 PDT
> 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.
Comment 63 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-21 06:51:52 PDT
Sure, but the proxy can be configured for Digest or NTLM authentication.
Comment 64 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-21 06:56:07 PDT
And, why shouldn't the websocket protocol handler handle 401/www-authenticate?
Comment 65 Wellington Fernando de Macedo 2009-08-21 07:04:15 PDT
(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
Comment 66 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-21 07:36:04 PDT
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...
Comment 67 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-21 07:37:51 PDT
Note that your IRC quote is from February, whereas that spec link is from August.
Comment 68 Wellington Fernando de Macedo 2009-08-21 07:59:30 PDT
(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.
Comment 69 Hixie (not reading bugmail) 2009-08-21 12:51:08 PDT
Yeah I added that recently.
Comment 70 Takuya Oikawa 2009-09-09 03:37:12 PDT
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
Comment 71 Wellington Fernando de Macedo 2009-09-09 06:27:30 PDT
> 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?
Comment 72 Wellington Fernando de Macedo 2009-09-09 06:36:26 PDT
ps: My first phrase should be before the Biesinger's phrase, because it was an unrelated subject, sorry.
Comment 73 Wellington Fernando de Macedo 2009-09-21 19:40:21 PDT
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.
Comment 74 Jason Duell [:jduell] (needinfo? me) 2009-09-25 18:45:33 PDT
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 75 Jason Duell [:jduell] (needinfo? me) 2009-09-25 18:49:03 PDT
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.
Comment 76 Wellington Fernando de Macedo 2009-09-27 10:46:27 PDT
Created attachment 403123 [details] [diff] [review]
working patch (complete)

This patch does proxy resolution and it is complete.
Wellington.
Comment 77 Wellington Fernando de Macedo 2009-09-27 20:18:36 PDT
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.
Comment 78 Christian :Biesinger (don't email me, ping me on IRC) 2009-11-17 13:52:16 PST
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...
Comment 79 Christian :Biesinger (don't email me, ping me on IRC) 2009-11-17 14:04:01 PST
See also http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-November/024108.html
Comment 80 Wellington Fernando de Macedo 2009-11-22 07:32:21 PST
(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
Comment 81 Christian :Biesinger (don't email me, ping me on IRC) 2009-11-23 06:29:08 PST
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.
Comment 82 Wellington Fernando de Macedo 2009-11-29 14:25:54 PST
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.
Comment 83 Volkmar Kostka 2009-12-10 03:11:08 PST
Is this code already merged on trunk? Using this would give my employers web application a leading edge. ;-)
Comment 84 Christopher Blizzard (:blizzard) 2009-12-10 14:24:40 PST
Not yet.  Still waiting for review on the patch.
Comment 85 Boris Zbarsky [:bz] 2009-12-29 15:21:10 PST
Why are those protocols URI_LOADABLE_BY_ANYONE, exactly?  For that matter, why are they INHERIT_PRINCIPAL, if they don't return data?
Comment 86 Honza Bambas (:mayhemer) 2010-01-02 06:24:53 PST
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.
Comment 87 Wellington Fernando de Macedo 2010-01-02 06:34:08 PST
(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.
Comment 88 Wellington Fernando de Macedo 2010-01-02 06:37:46 PST
(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.
Comment 89 Honza Bambas (:mayhemer) 2010-01-02 07:07:30 PST
(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.
Comment 90 Boris Zbarsky [:bz] 2010-01-02 08:37:00 PST
> 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?
Comment 91 Wellington Fernando de Macedo 2010-01-02 09:09:51 PST
> 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.
Comment 92 d 2010-01-28 01:57:35 PST
Has work on this stopped? It would be nice to have Web Sockets shipped with Firefox 3.7.
Comment 93 Jonas Sicking (:sicking) 2010-01-28 15:45:57 PST
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.
Comment 94 Hixie (not reading bugmail) 2010-01-28 16:10:44 PST
Yes, answer will be yes.
Comment 95 Jason Duell [:jduell] (needinfo? me) 2010-02-02 12:24:02 PST
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!
Comment 96 Wellington Fernando de Macedo 2010-02-02 17:31:45 PST
nice :) 
if questions about the code arise please send an email or ping me on irc.
Comment 97 Rohit Kalhans 2010-02-08 13:26:57 PST
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.
Comment 98 Olli Pettay [:smaug] 2010-02-12 13:31:38 PST
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.
Comment 99 Wellington Fernando de Macedo 2010-02-14 13:57:48 PST
Created attachment 426920 [details] [diff] [review]
up-to-date patch

Olli, this patch is up-to-date (parent revision is 51f9d293c939).
Comment 100 Wellington Fernando de Macedo 2010-02-14 14:02:55 PST
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.
Comment 101 Wellington Fernando de Macedo 2010-02-14 15:01:28 PST
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.
Comment 102 Hixie (not reading bugmail) 2010-02-14 15:26:42 PST
Just a heads-up — the handshake is likely to change in the coming weeks, based on discussion on hybi.
Comment 103 E 2010-02-15 02:16:55 PST
Hixie.
Websockets handshake can change? How? It's standard.
Comment 104 Olli Pettay [:smaug] 2010-02-15 02:47:26 PST
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.
Comment 105 Olli Pettay [:smaug] 2010-02-15 04:25:17 PST
Created attachment 426964 [details]
some more comments (mainly about event handling)
Comment 106 Olli Pettay [:smaug] 2010-02-15 05:13:19 PST
 
+  DOM_CLASSINFO_MAP_BEGIN(WebSocket, nsIWebSocket)
+    DOM_CLASSINFO_MAP_ENTRY(nsIWebSocket)
+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget)
+  DOM_CLASSINFO_MAP_END
Should have also nsIDOMNSEventTarget
Comment 107 Olli Pettay [:smaug] 2010-02-15 09:15:50 PST
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?)
Comment 108 Wellington Fernando de Macedo 2010-02-15 09:31:34 PST
> (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).
Comment 109 Olli Pettay [:smaug] 2010-02-15 09:41:22 PST
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.
Comment 110 Olli Pettay [:smaug] 2010-02-16 11:47:50 PST
Created attachment 427173 [details]
comments (mainly about nsWebSocket.cpp)
Comment 111 Olli Pettay [:smaug] 2010-02-17 05:57:11 PST
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.
Comment 112 Wellington Fernando de Macedo 2010-02-17 06:04:40 PST
> 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.
Comment 113 Olli Pettay [:smaug] 2010-02-17 06:13:40 PST
(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 114 Olli Pettay [:smaug] 2010-02-17 12:55:02 PST
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.
Comment 115 Olli Pettay [:smaug] 2010-02-18 07:36:03 PST
nsHttpChannel::OnAuthAvailable and nsHttpChannel::OnAuthCancelled have at least wrong LOG(), but I also need to understand the changes to those methods.
Comment 116 Olli Pettay [:smaug] 2010-02-18 07:37:49 PST
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.
Comment 117 Olli Pettay [:smaug] 2010-02-18 09:14:20 PST
Created attachment 427602 [details]
some comments about new networking/auth interfaces
Comment 118 Olli Pettay [:smaug] 2010-02-18 09:19:02 PST
Created attachment 427603 [details]
some comments about new networking/auth interfaces

This is the right file
Comment 119 Wellington Fernando de Macedo 2010-02-22 11:57:25 PST
Created attachment 428262 [details] [diff] [review]
updated patch
Comment 120 Olli Pettay [:smaug] 2010-02-23 09:57:20 PST
For some reason I can't 'hg import' or patch -p1 the latest patch.
I get lots of Hunk #X failed errors
Comment 121 Wellington Fernando de Macedo 2010-02-23 10:13:45 PST
(In reply to comment #120)
I forgot to say when I uploaded, the parent's revision is d80b2bb478dc
Comment 122 Wellington Fernando de Macedo 2010-02-23 12:00:19 PST
Created attachment 428497 [details] [diff] [review]
updated patch

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

Wellington.
Comment 123 Christoph Stritt 2010-02-24 04:01:02 PST
and you forgot again ;) To save you some hassle: applies to rev 1918f1187eb6
(nsHttpChannel.cpp gets changed a lot)
Comment 124 Olli Pettay [:smaug] 2010-02-24 13:28:55 PST
Created attachment 428799 [details]
few comments
Comment 125 Honza Bambas (:mayhemer) 2010-02-24 14:39:40 PST
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
Comment 126 Wellington Fernando de Macedo 2010-02-24 15:12:16 PST
(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.
Comment 127 Wellington Fernando de Macedo 2010-02-24 15:12:54 PST
(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 :)
Comment 128 Wellington Fernando de Macedo 2010-02-24 15:15:10 PST
> +  // 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.
Comment 129 Jonas Sicking (:sicking) 2010-02-24 15:18:09 PST
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.
Comment 130 Wellington Fernando de Macedo 2010-02-24 15:23:24 PST
(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 ... "
Comment 131 Jonas Sicking (:sicking) 2010-02-24 15:28:37 PST
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.
Comment 132 Wellington Fernando de Macedo 2010-02-24 15:33:27 PST
(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.
Comment 133 Honza Bambas (:mayhemer) 2010-02-24 15:36:41 PST
(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 134 Olli Pettay [:smaug] 2010-02-25 02:21:07 PST
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 135 Olli Pettay [:smaug] 2010-02-25 03:33:58 PST
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.
Comment 136 Wellington Fernando de Macedo 2010-02-25 11:01:19 PST
> +    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.
Comment 137 Wellington Fernando de Macedo 2010-02-25 11:03:01 PST
CheckInnerWindowCorrectness::CheckInnerWindowCorrectness ->
nsDOMEventTargetHelper::CheckInnerWindowCorrectness/
Comment 138 Wellington Fernando de Macedo 2010-02-25 11:14:17 PST
> (...) when the window became frozen (...)
Sorry. I meant when the window is closed but not actually deleted (by the garbage collector).
Comment 139 Olli Pettay [:smaug] 2010-02-25 11:57:36 PST
(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.
Comment 140 Wellington Fernando de Macedo 2010-03-02 05:55:37 PST
Created attachment 429716 [details] [diff] [review]
updated patch

parent's revision is 4e1b68ecf126
Comment 141 Olli Pettay [:smaug] 2010-03-02 07:41:30 PST
Created attachment 429731 [details]
some comments

jduell, feel free to review the patch. These comments are pretty minor.
Comment 142 Olli Pettay [:smaug] 2010-03-02 07:42:53 PST
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.
Comment 143 Wellington Fernando de Macedo 2010-03-02 07:50:12 PST
> 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.
Comment 144 Jason Duell [:jduell] (needinfo? me) 2010-03-02 12:35:43 PST
Has the promised new WS handshake been announced yet? (I'm way behind on my hybi email.)  If so, have we implemented it?
Comment 145 Hixie (not reading bugmail) 2010-03-02 12:40:21 PST
Spec should be available by the end of the week.
Comment 146 Jason Duell [:jduell] (needinfo? me) 2010-03-03 19:00:35 PST
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.
Comment 147 Hixie (not reading bugmail) 2010-03-03 19:32:46 PST
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. :-)
Comment 148 Olli Pettay [:smaug] 2010-03-04 02:26:06 PST
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).
Comment 149 Olli Pettay [:smaug] 2010-03-04 02:44:26 PST
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 mykolas.juraitis 2010-03-04 02:49:44 PST
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.
Comment 151 Hixie (not reading bugmail) 2010-03-04 03:01:16 PST
Please discuss the protocol on the hybi or whatwg lists, rather than in a bug database. :-)
Comment 152 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-17 15:46:34 PDT
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.
Comment 153 Wellington Fernando de Macedo 2010-03-17 17:57:27 PDT
(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.
Comment 154 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-18 04:31:05 PDT
4. Use QueryInterface to go from nsIHttpAuthenticableChannel -> nsIChannel

(that's what I meant)
Comment 155 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-18 05:43:35 PDT
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
Comment 156 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-18 05:45:37 PDT
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.
Comment 157 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-18 06:07:25 PDT
Created attachment 433317 [details]
Review comments for netwerk/protocol/http/src/nsHttpChannelAuthInternal.cpp
Comment 158 Wellington Fernando de Macedo 2010-03-18 13:05:12 PDT
ok. I will make these changes. BTW, should the new handshake proposal be in the next patch?
Comment 159 Olli Pettay [:smaug] 2010-03-18 13:13:32 PDT
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.
Comment 160 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-18 13:16:50 PDT
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.
Comment 161 Wellington Fernando de Macedo 2010-03-18 14:06:29 PDT
ok
Comment 162 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-18 14:33:24 PDT
Created attachment 433424 [details]
review comments for the non-netwerk/ parts

Here you go.
Comment 163 Wellington Fernando de Macedo 2010-04-06 18:25:36 PDT
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 :).
Comment 164 Olli Pettay [:smaug] 2010-04-06 22:44:25 PDT
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.
Comment 165 Hixie (not reading bugmail) 2010-04-06 22:46:51 PDT
http://www.whatwg.org/specs/web-socket-protocol is the latest version. -75 has security bugs, so I strongly recommend avoiding it.
Comment 166 Olli Pettay [:smaug] 2010-04-06 22:53:06 PDT
http://dev.w3.org/html5/websockets/ has the link to the protocol and that
gives v.75 here.
Comment 167 Olli Pettay [:smaug] 2010-04-06 22:56:00 PDT
But anyway, it is ok at least to me to implement v75 and update the implementation
in another bug.
Comment 168 Hixie (not reading bugmail) 2010-04-07 00:19:49 PDT
If you implement -75, please at least fix the security bugs.
Comment 169 Lucas Adamski [:ladamski] 2010-04-07 12:10:41 PDT
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.
Comment 170 Olli Pettay [:smaug] 2010-04-07 12:19:10 PDT
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)
Comment 171 Olli Pettay [:smaug] 2010-04-07 12:39:04 PDT
(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 ;)
Comment 172 Lucas Adamski [:ladamski] 2010-04-07 12:48:56 PDT
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.
Comment 173 Olli Pettay [:smaug] 2010-04-07 13:05:29 PDT
Shipping is different than getting some code - which can be improved - to landed.
Comment 174 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-04-07 16:10:27 PDT
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.
Comment 175 Wellington Fernando de Macedo 2010-04-07 17:57:58 PDT
Created attachment 437723 [details] [diff] [review]
updated patch

Parent's revision is 2c44a32052ad
Comment 176 Wellington Fernando de Macedo 2010-04-07 17:58:55 PDT
Created attachment 437724 [details]
other comments

other comments, answers and questions...
Comment 177 Olli Pettay [:smaug] 2010-04-14 10:26:10 PDT
Comment on attachment 437724 [details]
other comments


>> It might be safer to do Close asynchronously. Close may
>> dispatch events.
>It gives some assertions when the window is closed, because the close event is sent
>for a document which doesn't exists anymore. For now I let as it was. Do you have
>any suggestion?
>
Perhaps Close could check if window is still "alive".
Comment 178 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-21 08:27:44 PDT
Comment on attachment 437724 [details]
other comments

>Yes, but some of these changes are related to the v.76. I will let this interface
>for when the patch implements this new version.

Yeah, that's fine.

>It is because the ProxyService doesn't provide the manual http and https proxies
>(because of the ws and wss schemes). So, if a null proxy is given and the config
>is manual the code should try with the http and https proxies specified by the
>prefs.

You should add a comment for that.

>Well, ws messages shouldn't be in the cache. I don't know exactly how the netwerk's
>transport layer (nsISocketTransport and soon) handle this (because IMHO this should
>be done by the http protocol, but actually I don't know). For now I will remove the
>VALIDATE_NEVER. If you wnat I remove the others ones.

Those two comments were fairly independent. The nsISocketTransport::BYPASS_CACHE flag means that the DNS cache will be bypassed. There seems to be no reason for that here. The "again" was referring to the code that passes nsIDNSService::RESOLVE_BYPASS_CACHE, which also seems to be unnecessary.

As for VALIDATE_NEVER and the other load flags - the way you use them here doesn't really make sense. They are really meant to be set by the user, but there is no user here. If the protocol doesn't support caching (like websockets, but also file:, etc), you should just not set any of the cache-related flags. The LOAD_BACKGROUND one does make sense since that affects whether the throbber/progress bar should show that a load is pending, which I guess you don't want here.

>Well, I tried, but I couldn't find a way to convert to UTF-8. So I simply appended
>the string to a nsCString...

Yes, leaving it as an nsCString is the only correct behaviour for cookies.

>> It might be safer to do Close asynchronously. Close may
>> dispatch events.

leaving this one to smaug
Comment 179 Wellington Fernando de Macedo 2010-04-21 08:39:22 PDT
Comment on attachment 437475 [details]
some comments and questions

you forgot this one.
Comment 180 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-21 08:40:36 PDT
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.
Comment 181 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-21 08:41:12 PDT
I didn't! I just did them in the wrong order ;)
Comment 182 Wellington Fernando de Macedo 2010-04-21 11:08:51 PDT
> 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"?
Comment 183 Hixie (not reading bugmail) 2010-04-21 11:38:15 PDT
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.
Comment 184 Olli Pettay [:smaug] 2010-04-21 12:59:51 PDT
(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.
Comment 185 Wellington Fernando de Macedo 2010-04-21 13:13:44 PDT
> 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.
Comment 186 Olli Pettay [:smaug] 2010-04-21 14:07:14 PDT
(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.
Comment 187 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-22 10:54:25 PDT
(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.
Comment 188 Wellington Fernando de Macedo 2010-04-24 16:32:32 PDT
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.
Comment 189 Wellington Fernando de Macedo 2010-04-24 16:44:22 PDT
Created attachment 441314 [details] [diff] [review]
content refactoring patch
Comment 190 Wellington Fernando de Macedo 2010-04-24 16:52:11 PDT
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.
Comment 191 Wellington Fernando de Macedo 2010-04-24 16:57:39 PDT
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?
Comment 192 Olli Pettay [:smaug] 2010-04-25 05:26:37 PDT
Why mOwner->CheckInnerWindowCorrectness() isn't enough?
Comment 193 Wellington Fernando de Macedo 2010-04-25 08:07:58 PDT
(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 194 Olli Pettay [:smaug] 2010-04-25 08:58:27 PDT
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.
Comment 195 Wellington Fernando de Macedo 2010-04-25 09:10:44 PDT
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.
Comment 196 Wellington Fernando de Macedo 2010-04-25 09:16:49 PDT
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.
Comment 197 Olli Pettay [:smaug] 2010-04-25 09:57:24 PDT
Now that I see the reason for the change, no need to change that part of the patch.
Comment 198 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-26 08:02:18 PDT
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.
Comment 199 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-26 08:37:35 PDT
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
Comment 200 Wellington Fernando de Macedo 2010-04-26 11:57:23 PDT
(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.
Comment 201 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-28 07:32:47 PDT
(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.
Comment 202 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-28 07:34:41 PDT
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 203 Olli Pettay [:smaug] 2010-04-28 09:17:49 PDT
Comment on attachment 441316 [details] [diff] [review]
websocket patch

Change the pref to false for now, and make the error messages localizable.
Comment 204 Wellington Fernando de Macedo 2010-04-29 09:43:40 PDT
Created attachment 442418 [details] [diff] [review]
netwerk refactoring patch
Comment 205 Wellington Fernando de Macedo 2010-04-29 09:45:39 PDT
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.
Comment 206 Christoph Stritt 2010-05-03 07:50:34 PDT
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);
Comment 207 Olli Pettay [:smaug] 2010-05-03 08:17:49 PDT
NS_NEW_RUNNABLE_METHOD should be changed to use NS_NewRunnableMethod
Comment 208 Olli Pettay [:smaug] 2010-05-03 09:32:28 PDT
Comment on attachment 442419 [details] [diff] [review]
websocket patch

If you update the patch to use NS_NewRunnableMethod, r=me.
Comment 209 Christian :Biesinger (don't email me, ping me on IRC) 2010-05-05 07:22:25 PDT
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 210 Christian :Biesinger (don't email me, ping me on IRC) 2010-05-05 07:32:25 PDT
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.
Comment 211 Wellington Fernando de Macedo 2010-05-05 07:45:38 PDT
(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.
Comment 212 Christian :Biesinger (don't email me, ping me on IRC) 2010-05-05 08:15:42 PDT
At those two points in the code, an auth prompt should not be up, right?
Comment 213 Wellington Fernando de Macedo 2010-05-05 11:12:07 PDT
(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 214 Christian :Biesinger (don't email me, ping me on IRC) 2010-05-07 06:35:38 PDT
Comment on attachment 442418 [details] [diff] [review]
netwerk refactoring patch

r=biesi with those two changes
Comment 215 Wellington Fernando de Macedo 2010-05-07 15:13:58 PDT
Created attachment 444192 [details] [diff] [review]
content refactoring patch

just updating to the trunk
Comment 216 Wellington Fernando de Macedo 2010-05-07 15:15:15 PDT
Created attachment 444193 [details] [diff] [review]
netwerk refactoring patch
Comment 217 Wellington Fernando de Macedo 2010-05-07 15:20:53 PDT
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 :)
Comment 218 d 2010-05-08 00:59:16 PDT
Are these going to get pushed before work is completed on bug 562681?
Comment 219 Olli Pettay [:smaug] 2010-05-08 13:19:24 PDT
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.
Comment 220 Olli Pettay [:smaug] 2010-05-10 09:17:33 PDT
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.
Comment 221 Olli Pettay [:smaug] 2010-05-10 09:27:37 PDT
I uploaded the patches to tryserver.
Comment 222 Olli Pettay [:smaug] 2010-05-10 13:48:18 PDT
The patch seems to build ok
https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-try-cc2bdc16c309/
And no test failures.
Comment 223 Fabrice Desré 2010-05-11 02:40:56 PDT
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 Fabrice Desré 2010-05-11 02:45:42 PDT
Ignore the previous comment, I had not set the network.websocket.enabled preference to "true". 

It works !
Comment 225 Wellington Fernando de Macedo 2010-05-11 06:14:57 PDT
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?
Comment 226 Wellington Fernando de Macedo 2010-05-11 06:18:49 PDT
Created attachment 444647 [details] [diff] [review]
websocket patch

sorry, wrong patch.
Comment 227 Olli Pettay [:smaug] 2010-05-11 07:04:12 PDT
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.)
Comment 228 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-05-11 07:40:15 PDT
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.)
Comment 229 Olli Pettay [:smaug] 2010-05-11 07:54:21 PDT
Ok. I won't push the patch to m-c then. Even pref'ed off.
Comment 230 d 2010-05-11 07:56:51 PDT
(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.
Comment 231 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-05-11 08:06:07 PDT
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!
Comment 232 Wellington Fernando de Macedo 2010-05-11 08:39:30 PDT
> 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.
Comment 233 Olli Pettay [:smaug] 2010-05-11 08:47:16 PDT
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.
Comment 234 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-05-11 08:53:52 PDT
Clint may be able to help expedite the necessary infrastructure work, copying him here.
Comment 235 cmtalbert 2010-05-11 20:22:12 PDT
(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 cmtalbert 2010-05-14 12:53:57 PDT
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
Comment 237 Jason Duell [:jduell] (needinfo? me) 2010-05-14 14:26:57 PDT
Nice summary, Clint.  I agree that the pywebsocket solution (option #1) sounds overall like the best route.
Comment 238 Johnny Stenback (:jst, jst@mozilla.com) 2010-05-14 14:53:06 PDT
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.
Comment 239 Damon Sicore (:damons) 2010-05-28 13:28:44 PDT
OK.  Marking this as blocking 1.9.3beta1+, BUT!!!  If it's not finished for beta1, then we will likely remove blocking status.
Comment 240 cmtalbert 2010-05-28 14:53:26 PDT
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.
Comment 241 Wellington Fernando de Macedo 2010-06-03 07:48:38 PDT
Just forwarding. Chromium has updated its protocol implementation.
http://blog.chromium.org/2010/06/websocket-protocol-updated.html
Comment 242 Jonathan Griffin (:jgriffin) 2010-06-04 13:10:01 PDT
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?
Comment 243 Wellington Fernando de Macedo 2010-06-04 15:54:33 PDT
Created attachment 449370 [details] [diff] [review]
netwerk refactoring patch
Comment 244 Wellington Fernando de Macedo 2010-06-04 15:56:19 PDT
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
Comment 245 Wellington Fernando de Macedo 2010-06-17 10:13:38 PDT
Created attachment 451983 [details] [diff] [review]
netwerk refactoring patch

netwerk refactoring patch updated to the trunk (rev bc28b9c1545d)
Comment 247 Olli Pettay [:smaug] 2010-06-19 05:21:41 PDT
http://hg.mozilla.org/mozilla-central/rev/2717d84e1537
Comment 248 Olli Pettay [:smaug] 2010-06-19 07:15:57 PDT
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).
Comment 249 Eric Shepherd [:sheppy] 2010-10-22 13:09:36 PDT
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.
Comment 250 Paul Rouget [:paul] 2010-10-25 01:52:22 PDT
It will (probably ) be prefixed: bug 602028
Comment 251 Brendan Eich [:brendan] 2010-10-25 11:24:14 PDT
(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
Comment 252 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2011-08-17 04:23:02 PDT
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?
Comment 253 Olli Pettay [:smaug] 2011-08-17 04:29:58 PDT
See https://bugzilla.mozilla.org/show_bug.cgi?id=659324
Comment 254 Bronislav Klučka 2011-08-25 12:27:20 PDT
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
Comment 255 Patrick McManus [:mcmanus] 2011-08-25 12:35:52 PDT
(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 Bronislav Klučka 2011-08-26 08:29:49 PDT
(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 Bronislav Klučka 2011-08-28 17:15:05 PDT
Firefox 6 does not expose close code and close reason in close event.
Firefox 6 does handle binary content.
Comment 258 Bronislav Klučka 2011-08-28 17:25:44 PDT
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.

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