553125 - Make window.postMessage generate a structured clone

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Johnny Stenback (:jst)]

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

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/6647ba316a10

Comment 10

[Johnny Stenback (:jst)]

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

Comment 11

[Eric Shepherd [:sheppy]]

Updated:

https://developer.mozilla.org/en/DOM/window.postMessage

And added to Firefox 6 for developers.

Comment 12

[Johnathan Nightingale [:johnath]]

Untracking based on triage call today

Comment 13

[Eric Shepherd [:sheppy]]

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

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.