674527 - Update WebSocket API to latest draft - array of protocols in ctor

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

+++ 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]]

Attachment: patch v1

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

(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]]

Attachment: patch 2

interface change, so request sr?

Updated patch reflects jonas's review comments modulo comment 3.

Comment 5

[Patrick McManus [:mcmanus]]

(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]

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

(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 [:bzbarsky]]

Comment on attachment 550199 [details] [diff] [review]
patch 2

sr=me

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ccbd9a4df280

Comment 10

[Eric Shepherd [:sheppy]]

Our docs didn't mention that this wasn't supported, so all that was needed was a note on Firefox 8 for developers.