Bug 550275 implemented the structured clone algorithm, window.postMessage should use it.

Bug 624506 points out that we should consider not accepting non-string arguments in window.postMessage if we don't implement structured cloning, so as not to poison the well.

I'd love to see this fixed asap, but I won't hold the release for it.

I have a patch for this. I'm sorting through the test failures.

Two things of note: 1. Including jsapi.h into nsIDOMWindowInternal.h really sucks, but I couldn't find another way to get the jsval stuff to work (shame xpidl doesn't do the right thing here). 2. My understanding of the JSAPI is pretty low. I'm not sure if I've overused or underused JSAutoEnterRequest and I have no idea if I need to worry about compartments here ... I basically played with it until it didn't assert.

Comment on attachment 534346 [details] [diff] [review] Patch Review of attachment 534346 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsWebSocket.cpp @@ +825,5 @@ > + > + nsIScriptContext* scriptContext = sgo->GetContext(); > + NS_ENSURE_TRUE(scriptContext, NS_ERROR_FAILURE); > + > + JSContext* jsContext = (JSContext*)scriptContext->GetNativeContext(); Nit: Almost everything uses 'cx' for JSContext... Let's keep that up. @@ +839,5 @@ > + utf16Data.Length()); > + } > + NS_ENSURE_TRUE(jsString, NS_ERROR_FAILURE); > + > + jsval jsData = STRING_TO_JSVAL(jsString); I would keep all this in the autorequest block. ::: content/events/src/nsDOMMessageEvent.h @@ +56,5 @@ > public: > nsDOMMessageEvent(nsPresContext* aPresContext, nsEvent* aEvent) > + : nsDOMEvent(aPresContext, aEvent), > + mDataRooted(false), > + mData(JSVAL_VOID) This initialization order is backwards (based on order of declaration), please reverse. ::: dom/base/nsGlobalWindow.cpp @@ +5860,5 @@ > } > > ~PostMessageEvent() > { > MOZ_COUNT_DTOR(PostMessageEvent); Assert that mMessage is null here? @@ +5929,5 @@ > nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager(); > nsresult rv = > ssm->CheckSameOriginURI(mProvidedOrigin, targetURI, PR_TRUE); > if (NS_FAILED(rv)) > return NS_OK; This will leak mMessage... Need to have Run create a local JSAutoStructuredCloneBuffer to ensure deletion when exiting. @@ +5936,5 @@ > + // Get the JSContext for the target window > + nsIScriptContext* scriptContext = mTargetWindow->GetContext(); > + NS_ENSURE_TRUE(scriptContext, NS_ERROR_FAILURE); > + > + JSContext* jsContext = (JSContext*)scriptContext->GetNativeContext(); Nit: Almost everything uses 'cx' for JSContext... Let's keep that up. @@ +5953,5 @@ > + JSStructuredCloneCallbacks callbacks = { > + nsnull, nsnull, nsnull > + }; > + > + if (!buffer.read(&messageData, jsContext, &callbacks)) Just pass null for the callbacks arg rather than make a useless one. That way you get the system callbacks. @@ +6008,5 @@ > +nsGlobalWindow::PostMessageMoz(const jsval& aMessage, > + const nsAString& aOrigin, > + JSContext* aCx) > +{ > + FORWARD_TO_OUTER(PostMessageMoz, (aMessage, aOrigin, aCx), NS_ERROR_NOT_INITIALIZED); This line exceeds 80 chars. @@ +6076,5 @@ > this, > providedOrigin, > nsContentUtils::IsCallerTrustedForWrite()); > + > + // We *must* clone the data here, or the jsval could modified Nit: "could be modified" @@ +6082,5 @@ > + JSAutoStructuredCloneBuffer buffer; > + JSStructuredCloneCallbacks callbacks = > + { nsnull, nsnull, nsnull }; > + > + if (!buffer.write(aCx, aMessage, &callbacks, nsnull)) Pass null for callbacks arg. ::: dom/interfaces/base/nsIDOMWindowInternal.idl @@ +39,5 @@ > > #include "nsIDOMWindow2.idl" > > +%{ C++ > +#include "jsapi.h" just forward declare 'struct jsval' ::: dom/interfaces/events/nsIDOMMessageEvent.idl @@ +38,5 @@ > > #include "nsIDOMEvent.idl" > > +%{ C++ > +#include "jsapi.h" And here. ::: dom/tests/mochitest/whatwg/postMessage_structured_clone_helper.html @@ +7,5 @@ > + <script type="application/javascript"> > + var generator = new getTestContent() > + function receiveMessage(evt) > + { > + if (evt.data === generator.next()) I'm a little concerned about this... How does this work for the object case?

Comments addressed, and changes similar to the WebSocket changes made for the newly-landed EventSource.

Comment on attachment 534482 [details] [diff] [review] Patch Review of attachment 534482 [details] [diff] [review]: ----------------------------------------------------------------- A few tiny changes needed, r=me with that. ::: content/base/src/nsEventSource.cpp @@ +1338,5 @@ > if (NS_FAILED(rv)) { > return; > } > > + // Lets play get the JSContext Nit: Either cut the snark or use proper "Let's" ;) @@ +1353,5 @@ > nsAutoPtr<Message> > message(static_cast<Message*>(mMessagesToDispatch.PopFront())); > > + // Now we can turn our string into a jsval > + JSString* jsString; This shouldn't be outside the request scope. @@ +1378,5 @@ > > nsCOMPtr<nsIDOMMessageEvent> messageEvent = do_QueryInterface(event); > rv = messageEvent->InitMessageEvent(message->mEventName, > PR_FALSE, PR_FALSE, > + jsData, Nit: This could probably fit on the previous line. ::: content/base/src/nsWebSocket.cpp @@ +818,5 @@ > if (NS_FAILED(rv)) { > return NS_OK; > } > > + // Lets play get the JSContext Nit: again @@ +830,5 @@ > + NS_ENSURE_TRUE(cx, NS_ERROR_FAILURE); > + > + // Now we can turn our string into a jsval > + NS_ConvertUTF8toUTF16 utf16Data(aData); > + JSString* jsString; Both of these should be inside the request block. ::: content/events/src/nsDOMMessageEvent.cpp @@ +43,5 @@ > NS_IMPL_CYCLE_COLLECTION_CLASS(nsDOMMessageEvent) > > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsDOMMessageEvent, nsDOMEvent) > + if (tmp->mDataRooted) > + tmp->UnrootData(); I think our style guide gods declared that all new code get braces around single line if statements. @@ +56,5 @@ > +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsDOMMessageEvent) > + if (JSVAL_IS_GCTHING(tmp->mData)) { > + void *gcThing = JSVAL_TO_GCTHING(tmp->mData); > + NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(gcThing, > + "mData") Nit: No need for a separate line here. ::: dom/base/nsGlobalWindow.cpp @@ +5896,5 @@ > + JSContext* cx = (JSContext*)scriptContext->GetNativeContext(); > + NS_ENSURE_TRUE(cx, NS_ERROR_FAILURE); > + > + // If we bailed before this point we're going to leak mMessage, but > + // that's probably better than crashing. I don't understand this... How would we crash? @@ +5955,5 @@ > + { > + JSAutoRequest ar(cx); > + > + if (!buffer.read(&messageData, cx, nsnull)) > + return NS_ERROR_FAILURE; Should be NS_ERROR_DOM_DATA_CLONE_ERR

Tracking for Firefox 6 since we took the followup fix in bug 659215.

Updated: https://developer.mozilla.org/en/DOM/window.postMessage And added to Firefox 6 for developers.

Untracking based on triage call today

(In reply to Johnathan Nightingale [:johnath] from comment #12) > Untracking based on triage call today Does this mean this is being removed from Firefox 6 then?

No, it means that the drivers don't think they need to worry about it any more. See comment 10 for why it was tracked to begin with.