Closed Bug 674527 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Networking: WebSockets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound])

Attachments

(1 file, 1 obsolete file)

+++ 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.
Blocks: 666349
No longer depends on: 666349
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #548779 - Flags: review?(jonas)
Blocks: 674716
No longer blocks: 674716
Blocks: 674890
No longer blocks: 674890
Blocks: 674905
No longer blocks: 674905
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.
Attachment #548779 - Flags: review?(jonas) → review+
(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?
Attached patch patch 2Splinter Review
interface change, so request sr?

Updated patch reflects jonas's review comments modulo comment 3.
Attachment #548779 - Attachment is obsolete: true
Attachment #550199 - Flags: superreview?(bzbarsky)
Attachment #550199 - Flags: review+
(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..
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)
Comment on attachment 550199 [details] [diff] [review]
patch 2

sr=me
Attachment #550199 - Flags: superreview?(bzbarsky) → superreview+
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/ccbd9a4df280
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Our docs didn't mention that this wasn't supported, so all that was needed was a note on Firefox 8 for developers.
You need to log in before you can comment on or make changes to this bug.