The default bug view has changed. See this FAQ.

Make window.postMessage generate a structured clone

RESOLVED FIXED in mozilla6

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: khuey)

Tracking

({dev-doc-complete})

Trunk
mozilla6
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox6-, blocking2.0 -)

Details

(Whiteboard: [tracking+ in c#10])

Attachments

(1 attachment, 1 obsolete attachment)

Bug 550275 implemented the structured clone algorithm, window.postMessage should use it.
Duplicate of this bug: 624506
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.
blocking2.0: --- → ?
I'd love to see this fixed asap, but I won't hold the release for it.
blocking2.0: ? → -
I have a patch for this.  I'm sorting through the test failures.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Created attachment 534346 [details] [diff] [review]
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.
Attachment #534346 - Flags: review?(bent.mozilla)
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?
Created attachment 534482 [details] [diff] [review]
Patch

Comments addressed, and changes similar to the WebSocket changes made for the newly-landed EventSource.
Attachment #534346 - Attachment is obsolete: true
Attachment #534482 - Flags: review?(bent.mozilla)
Attachment #534346 - Flags: review?(bent.mozilla)
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
Attachment #534482 - Flags: review?(bent.mozilla) → review+
http://hg.mozilla.org/mozilla-central/rev/6647ba316a10
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Depends on: 659215
Tracking for Firefox 6 since we took the followup fix in bug 659215.
tracking-firefox6: --- → +
Updated:

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

And added to Firefox 6 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Whiteboard: [tracking+ in c#10]
Untracking based on triage call today
tracking-firefox6: + → -
(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.
You need to log in before you can comment on or make changes to this bug.