Patrick McManus [:mcmanus] Assignee
	Description • 13 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 2 • 13 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 3 • 13 years ago

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

	Patrick McManus [:mcmanus] Assignee
	Comment 4 • 13 years ago

interface change, so request sr?

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

	Patrick McManus [:mcmanus] Assignee
	Comment 5 • 13 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 13 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 7 • 13 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 13 years ago

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

sr=me

	Marco Bonardo [:mak]
	Comment 9 • 13 years ago

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

	Eric Shepherd [:sheppy]
	Comment 10 • 13 years ago

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