Last Comment Bug 674527 - Update WebSocket API to latest draft - array of protocols in ctor
: Update WebSocket API to latest draft - array of protocols in ctor
Status: RESOLVED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Patrick McManus [:mcmanus] PTO until Sep 6
:
Mentors:
Depends on:
Blocks: 666349
  Show dependency treegraph
 
Reported: 2011-07-27 07:33 PDT by Patrick McManus [:mcmanus] PTO until Sep 6
Modified: 2011-08-24 08:09 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (21.36 KB, patch)
2011-07-27 07:40 PDT, Patrick McManus [:mcmanus] PTO until Sep 6
jonas: review+
Details | Diff | Splinter Review
patch 2 (21.14 KB, patch)
2011-08-02 14:15 PDT, Patrick McManus [:mcmanus] PTO until Sep 6
mcmanus: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] PTO until Sep 6 2011-07-27 07:33:30 PDT
+++ This bug was initially created as a clone of Bug #666349 +++

We don't support constructing a WebSocket with an array of protocols.

This includes checks for duplicates and tests to make sure sub-protocol negotiation is done correctly both with array and domstring variants.
Comment 1 Patrick McManus [:mcmanus] PTO until Sep 6 2011-07-27 07:40:12 PDT
Created attachment 548779 [details] [diff] [review]
patch v1
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-02 10:22:00 PDT
Comment on attachment 548779 [details] [diff] [review]
patch v1

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

r=me with those changes.

::: content/base/public/nsIMozWebSocket.idl
@@ +109,5 @@
>    [noscript] void init(in nsIPrincipal principal,
>                         in nsIScriptContext scriptContext,
>                         in nsPIDOMWindow ownerWindow,
>                         in DOMString url,
> +                       in nsIDOMDOMStringList protocol);

Hmm.. it's very silly to need to use a DOMStringList here since this function isn't scriptable anyway. Would be much cleaner if you could use a nsTArray<nsString>&, the tricky part is getting the IDL magic right.

Try using |in nsStringTArrayRef protocol| in the init function and put one of the following above the interface declaration:

[ref] native nsStringTArrayRef(nsTArray<nsString>);

or if that doesn't work:

%{C++
typedef nsTArray<nsString> nsStringTArrayDef
%}
[ref] native nsStringTArrayRef(nsStringTArrayDef);

::: content/base/src/nsWebSocket.cpp
@@ +306,5 @@
> +
> +    if (!protocolList.IsEmpty())
> +      protocolList.Append(NS_LITERAL_CSTRING(", "));
> +    mOwner->mRequestedProtocol->Item(index, protocol16);
> +    AppendUTF16toUTF8(protocol16, protocolList);

What happens if someone passes in a protocol with a "," in it? We should probably throw an exception in that case to avoid unexpected behavior which could lead to XSS or other website issues. For example if it receives the protocol from a third party and checks it against a blacklist.

I take it the websocket protocol pass the protocol list as a comma-separated list?

@@ +750,5 @@
>    NS_ENSURE_STATE(scriptPrincipal);
>    nsCOMPtr<nsIPrincipal> principal = scriptPrincipal->GetPrincipal();
>    NS_ENSURE_STATE(principal);
>  
> +  nsCOMPtr<nsDOMStringList> protocolArray =

Use a nsRefPtr when holding references to things that aren't interfaces.

@@ +753,5 @@
>  
> +  nsCOMPtr<nsDOMStringList> protocolArray =
> +    new nsDOMStringList();
> +  if (!protocolArray)
> +    return NS_ERROR_OUT_OF_MEMORY;

new is infallible so no need to nullcheck. We willl already have aborted if the allocation failed :)

@@ +759,5 @@
> +  if (aArgc == 2) {
> +    JSObject *jsobj;
> +
> +    if (JS_ValueToObject(aContext, aArgv[1], &jsobj) &&
> +        JS_IsArrayObject(aContext, jsobj)) {

Hmm.. I *believe* that JS_ValueToObject will do a bunch of unnecessary work for the case when the passed in value is a primitive. I think what you want to use is JSVAL_IS_OBJECT/JSVAL_TO_OBJECT

@@ +773,5 @@
> +        jsstr = JS_ValueToString(aContext, value);
> +        if (!jsstr)
> +          return NS_ERROR_DOM_SYNTAX_ERR;
> +
> +        deleteProtector.set(jsstr);

Why do you need this? Just being on the stack should prevent the string from getting GC'ed.

@@ +784,5 @@
> +          return NS_ERROR_DOM_SYNTAX_ERR;
> +        PRBool contains = PR_FALSE;
> +        protocolArray->Contains(protocolElement, &contains);
> +        if (contains)
> +          return NS_ERROR_DOM_SYNTAX_ERR;

Since you're doing the Contains check here, might be consistent to put the "," check here too.
Comment 3 Patrick McManus [:mcmanus] PTO until Sep 6 2011-08-02 12:46:51 PDT
(In reply to comment #2)

> 
> @@ +773,5 @@
> > +        jsstr = JS_ValueToString(aContext, value);
> > +        if (!jsstr)
> > +          return NS_ERROR_DOM_SYNTAX_ERR;
> > +
> > +        deleteProtector.set(jsstr);
> 
> Why do you need this? Just being on the stack should prevent the string from
> getting GC'ed.
> 

bug 654197 said that the same usage pattern required the reference and that the stack was insufficient. So I'll keep it unless you clarify that it should still go?
Comment 4 Patrick McManus [:mcmanus] PTO until Sep 6 2011-08-02 14:15:56 PDT
Created attachment 550199 [details] [diff] [review]
patch 2

interface change, so request sr?

Updated patch reflects jonas's review comments modulo comment 3.
Comment 5 Patrick McManus [:mcmanus] PTO until Sep 6 2011-08-02 14:17:42 PDT
(In reply to comment #2)
> Comment on attachment 548779 [details] [diff] [review] [diff] [details] [review]
> patch v1

> Try using |in nsStringTArrayRef protocol| in the init function and put one
> of the following above the interface declaration:
> 
> [ref] native nsStringTArrayRef(nsTArray<nsString>);
> 

thanks - I couldn't figure that out..
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-02 17:21:21 PDT
Oh, another issue I just thought of. What happens if the protocol begins or ends with a " "? Won't we end up dropping that when parsing the comma separated list?

We should probably forbid space (or even whitespace) characters in the protocols as well.
Comment 7 Patrick McManus [:mcmanus] PTO until Sep 6 2011-08-02 17:51:31 PDT
(In reply to comment #6)
> Oh, another issue I just thought of. What happens if the protocol begins or
> ends with a " "? Won't we end up dropping that when parsing the comma
> separated list?
> 
> We should probably forbid space (or even whitespace) characters in the
> protocols as well.

0x20 is already out of range. (required to be x21->x7e)
Comment 8 Boris Zbarsky [:bz] 2011-08-02 19:22:39 PDT
Comment on attachment 550199 [details] [diff] [review]
patch 2

sr=me
Comment 9 Marco Bonardo [::mak] 2011-08-04 02:54:16 PDT
http://hg.mozilla.org/mozilla-central/rev/ccbd9a4df280
Comment 10 Eric Shepherd [:sheppy] 2011-08-24 08:09:56 PDT
Our docs didn't mention that this wasn't supported, so all that was needed was a note on Firefox 8 for developers.

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