Closed
Bug 674527
Opened 13 years ago
Closed 13 years ago
Update WebSocket API to latest draft - array of protocols in ctor
Categories
(Core :: Networking: WebSockets, defect)
Core
Networking: WebSockets
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [inbound])
Attachments
(1 file, 1 obsolete file)
21.14 KB,
patch
|
mcmanus
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
+++ 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•13 years ago
|
Assignee | ||
Comment 1•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.
Attachment #548779 -
Flags: review?(jonas) → review+
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?
Assignee | ||
Comment 4•13 years ago
|
||
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•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..
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•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)
![]() |
||
Comment 8•13 years ago
|
||
Comment on attachment 550199 [details] [diff] [review] patch 2 sr=me
Attachment #550199 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → mozilla8
Comment 9•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ccbd9a4df280
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•