Last Comment Bug 553125 - Make window.postMessage generate a structured clone
: Make window.postMessage generate a structured clone
Status: RESOLVED FIXED
[tracking+ in c#10]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
:
Mentors:
: 624506 (view as bug list)
Depends on: 550275 659215
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-17 18:36 PDT by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2011-08-09 05:45 PDT (History)
8 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
Patch (23.95 KB, patch)
2011-05-22 20:00 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (27.72 KB, patch)
2011-05-23 10:52 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2010-03-17 18:36:55 PDT
Bug 550275 implemented the structured clone algorithm, window.postMessage should use it.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-01-10 14:57:12 PST
*** Bug 624506 has been marked as a duplicate of this bug. ***
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-01-10 14:58:02 PST
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, jst@mozilla.com) 2011-01-13 18:36:35 PST
I'd love to see this fixed asap, but I won't hold the release for it.
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-21 12:38:33 PDT
I have a patch for this.  I'm sorting through the test failures.
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-22 20:00:45 PDT
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.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-05-22 22:36:12 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-23 10:52:54 PDT
Created attachment 534482 [details] [diff] [review]
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!) 2011-05-23 11:11:45 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-23 13:19:50 PDT
http://hg.mozilla.org/mozilla-central/rev/6647ba316a10
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2011-06-01 12:06:22 PDT
Tracking for Firefox 6 since we took the followup fix in bug 659215.
Comment 11 Eric Shepherd [:sheppy] 2011-07-08 14:26:10 PDT
Updated:

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

And added to Firefox 6 for developers.
Comment 12 Johnathan Nightingale [:johnath] 2011-08-08 14:44:26 PDT
Untracking based on triage call today
Comment 13 Eric Shepherd [:sheppy] 2011-08-09 05:43:19 PDT
(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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-09 05:45:02 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.