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

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

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.

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