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

RESOLVED FIXED in mozilla8

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

({dev-doc-complete})

Trunk
mozilla8
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 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.
(Assignee)

Updated

6 years ago
Blocks: 666349
No longer depends on: 666349
(Assignee)

Comment 1

6 years ago
Created attachment 548779 [details] [diff] [review]
patch v1
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #548779 - Flags: review?(jonas)
(Assignee)

Updated

6 years ago
Blocks: 674716
(Assignee)

Updated

6 years ago
No longer blocks: 674716
(Assignee)

Updated

6 years ago
Blocks: 674890
(Assignee)

Updated

6 years ago
No longer blocks: 674890
(Assignee)

Updated

6 years ago
Blocks: 674905
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 3

6 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?
(Assignee)

Comment 4

6 years ago
Created attachment 550199 [details] [diff] [review]
patch 2

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

Comment 5

6 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..
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.
(Assignee)

Comment 7

6 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)
Comment on attachment 550199 [details] [diff] [review]
patch 2

sr=me
Attachment #550199 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/ccbd9a4df280
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.