Closed Bug 387706 (postMessage) Opened 17 years ago Closed 16 years ago

Implement HTML5's cross-document messaging API (postMessage)

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: Waldo, Assigned: Waldo)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 12 obsolete files)

97.61 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
5.22 KB, patch
Details | Diff | Splinter Review
I got interested in this over the last weekend while doing some cross-domain Mochitesting, but it appears the state of the art for this involves watching the value of the hash fragment associated with the URL for each window in the communication and doing the p-word: "polling".

HTML5 introduces a method for cross-domain communication using a new event type, MessageEvent, and a new |postMessage(message)| method on documents, which dispatches an event to the document on which it is called.  The data property of the event contains the string provided in |message|, and domain/uri properties allow verification of the originator of the event, for security purposes.  Finally, a source property references the calling code's document, presumably to ease back-and-forth communication a little.  It's tentatively on Document, but I think it might end up being moved to Window, and my incomplete implementation puts it there.

This spec requires that you punch a hole through same-origin security policies *if* the method being called is named "postMessage" *and* it takes the given set of arguments *and* it is called on a Window; this is really fairly scary to verify correctly, in my uninformed opinion, since it has to be at the wrapper layer and not the API layer.  It also requires determining the from-Window by inspecting the call stack, which I've been told is non-trivial.  Eli Friedman suggested a simple change which would make this unnecessary: instead make the API |yourWindow.postMessage(message, otherWindow)|, and do all the same-origin carefulness in the postMessage implementation.  (I think this doesn't necessitate changes to the wrapper code, but I don't know for sure.)  I've proposed this to the WHATWG mailing list, and the one response so far has been positive.

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2007-July/012163.html

I have an implementation of this which I conservatively estimate to be 60% done (most of which is actually just adding the new event type).  I've added the actual event, and it can be created and dispatched correctly.  postMessage itself is implemented, but I'm pretty sure it's not doing the right things security-wise yet.  I'm uncertain of exactly how outer/inner windows work, so my actions to handle them are slightly suspect at the moment, but I believe correct (based only on debug addresses printed in the test page I've written).  I have not tested whether this actually works cross-origin, in general and specifically with respect to what I need to do about the isTrusted property on the event and how to make sure it's set correctly.  I also haven't taken steps to ensure content can't send MessageEvents arbitrary places; I've been assuming the default browser security would prevent this, but I haven't tested.  I'm tempted to ask that 'message' events not be dispatchable by content to play it safe, but this might (?) be overkill.  I'm pretty sure there's some security principle that agrees with the restriction, but I can't remember it at the moment.

With the API modification, I think it's feasible to have this done for 1.9, but I'm not counting on it -- think of this as a somewhat-dark horse, I suppose.


Current patch issues, probably not exhaustive, but making an attempt at it:
- inner/outerness not verified to be correct
- no Mochitests yet (requires some Mochitest work, bug 384192 has a patch)
  - testing that it works same-origin
  - testing that it works cross-origin
  - testing that it works with different origins, but joined via
    document.domain
  - testing that MessageEvent is/isn't dispatchable in content, and that
    trustedness is set appropriately if it's dispatchable
  - testing that isTrusted is set correctly for manual and postMessage
    dispatch
  - testing that MessageEvents can't be manually dispatched across origins
- not sure how isTrusted should be set for each dispatch case, and whether
  it needs particular settings to work cross-domain
- haven't thought through the security in postMessage especially fully, yet
- comments are out-of-date wrt the extra parameter, and possibly also just
  in general
- spec issues:
  - patch deviates from spec wrt the extra argument *and* wrt using Window
    instead of Document for source property and location
  - what order for params?  I think I prefer window, then msg, but it's not
    specified yet
  - what domain/URI for |window.frames.kidFrame.postMessage|?  if same
    domain?  if joined domains via document.domain setting?
I bet mrbkap will be interested in the wrapper-related issues!
Okay, this is either ready for review or pretty close to it -- close enough I don't know where to go next with it, so hopefully I can get a few pointers on where to go.

Don't be fooled by the patch size -- adding an event involves changes to a handful of widely dispersed locations, MessageEvent itself occupies space but is a fairly simple change, and the pach contains four or five tests of the things mentioned in comment 0.  I don't think there's more than 10K or so of really interesting (and non-test) code change here.

There's still one big, not-mentioned-in-the-spec question remaining, as far as I know: does postMessage dispatch its event synchronously or asynchronously?  The void return value is meaningless, because you don't want to expose information about how the dispatched event might have been handled (see the dummy PRBool in the implementation).  I believe my tests all handle either case, even tho the patch currently dispatches sync, but this really needs to be nailed down, with a test.

Regarding previous issues:

- inner/outerness should be okay now, I think
- includes the mentioned tests
- isTrusted is always set to false, think this is correct (the IDL for it
  is non-existent)
- comments are up-to-date
- regarding domain/URI on event for |window.frames.kidFrame.postMessage|
  calls, the patch always uses the domain/URI of |this| in the call; this
  means that same-origin sites can spoof messages from each other, which
  seems undesirable but entirely reasonable; if they cared, they could do
  much worse pretty easily
Attachment #271872 - Attachment is obsolete: true
Attachment #272299 - Flags: review?(jonas)
Comment on attachment 272299 [details] [diff] [review]
Better patch (probably not done, but unsure where to go next)

Random comments...

>+class nsMessageEvent : public nsEvent
...

If you're going to create messageevents only using normal DocumentEvent::CreateEvent
method, you don't need new class which extends nsEvent, just class which extends
nsDOMEvent. Though, to keep things consistent, having this class
might not be that bad idea.


>+  nsresult rv = NS_NewDOMMessageEvent(getter_AddRefs(event),
>+                                      presContext,
>+                                      nsnull);

Couldn't you create the event using nsIDOMDocumentEvent::CreateEvent method?


>+#ifdef DEBUG
>+  {
>+    nsCOMPtr<nsIDOMNSEvent> nsEvent(do_QueryInterface(message));
>+    PRBool isTrusted;
>+    nsEvent->GetIsTrusted(&isTrusted);
>+    NS_ASSERTION(!isTrusted, "this shouldn't be a trusted event");
>+  }
>+#endif

This doesn't look quite valid. What if the caller has UniversalBrowserWrite
rights.
Comment on attachment 272299 [details] [diff] [review]
Better patch (probably not done, but unsure where to go next)

Another thing to do in the next iteration: test postMessage on a window which has redefined dispatchEvent and made it do bad things, like access properties on the window which tried to send the message.  I'm guessing it might be possible to tickle the code into executing using the caller's principal without some care, but I don't know for sure.  Alternately, and perhaps a better idea, is to inline dispatchEvent in the function (or do similar to avoid calling any functions on the argument window), which is probably the best thing to do.
Attachment #272299 - Flags: review?(jonas)
The spec moved the method to window but didn't change event.source to the window (instead to the document); I think this might have been an oversight and will check with Hixie before changing.

Also, test_postMessage_override.html fails; I'm still looking into the problem.
Attachment #272299 - Attachment is obsolete: true
Oh, that patch is against my whole tree, so it contains some extra gunk I forgot to remove -- easily ignored, mostly in layout.
Attachment #278172 - Attachment is obsolete: true
Attached patch Ready-to-review patch (obsolete) — Splinter Review
Changes since the last include:
- removing nsMessageEvent completely; in hindsight it's clearly the right
  choice, and it moves the members to a location where they can be easily
  hooked up to cycle collection
- moving the members of nsMessageEvent into nsDOMMessageEvent
- implementing cycle collection love for nsDOMMessageEvent, correctly enough
  that I don't get leaks if I attach such an event to the window contained
  in the event (verified with dbaron's leak-gauge.pl), although I'm only
  assuming they were present before; I studied the macro definitions long
  enough that I think it's all correct, but knowledgeable review would help
- changing the target of the postMessage event to the window's primary
  document, per the latest HTML5 drafts
- removing code from a setTimeout/setInterval helper function and replacing
  it with a call to the get-the-caller's-inner-window function
- fixed a crash that would happen if you tried to get the .source of a
  newborn MessageEvent (or did the same after calling initMessageEvent with
  a null source), and added tests for these behaviors


With these changes, I now consider this patch ready to go.  (I've heard only one other suggestion, adding an unscriptable version of this, but given that you can trivially cross origins there I don't think that's really a big concern.)  Everything's implemented spec-compatibly, the spec has undergone some discussion and results are in this patch, and the patch contains enough tests that I don't know of anything else I think could use it, even in hyper-anticipatory mode.  The question now is what happens with this patch.  We're past feature freeze, but given enough bang for the buck, safety, etc. I expect we'd take a feature or two.  Assuming I hit all the event integration points I think this patch is reasonably safe.  There aren't many cross-code concerns here, adding classinfo is a well-understood process, and MessageEvent itself isn't interesting security-wise (only the window in it is, but that's handled elsewhere).  Further, the tests in the patch should be some assurance that this has been implemented correctly.  The bang for the buck is that web pages can communicate across origins on the client, in a controlled manner capable of being secured by web authors, without resorting to hacks.  The current state of the art for doing what postMessage does is to set the hash on otherWindow.location to some value and polling with setInterval to detect value changes, which is racy, inefficient, and a poor API which will at the very least be abstracted over by any web developer who uses it.  The functionality is useful for things like widgets, better sandboxes (yet still sandboxes with tuning hooks), and better-structured cross-window communication in general.

In light of recent discussions regarding time spent on non-blockers I'm not requesting review on this, on the grounds that we should decide we're at least interested in this before investing anyone's time (outside of my weekends and evenings) on this.
Attachment #278192 - Attachment is obsolete: true
Another couple problems I just noticed:

Since DispatchEvent calls off to content, if that content throws it'll set a pending exception.  I don't know and haven't tested the details, but I suspect if that happens, DispatchEvent will return an error but will not clear the pending JS exception (and when PostMessage returns that exception might be detected by XPConnect and might be thrown) -- hence, if we wish to maintain the illusion that anything that happened underneath postMessage is by-default undetectable, we need to clear the pending exception ourselves.  Either way, the next patch here will include some sort of test for this; I'll have to find out a way to cause a bogus pending exception to trigger an error, because I don't know how to do so at the moment.  Note to self: also include tests for swallowing exceptions in same-origin and cross-origin code, and send an email about this to WHATWG; we probably don't want to swallow for a same-origin postMessage but do want to swallow for a cross-origin postMessage.

The edit to nsGUIEvent.h may be unnecessary if we don't care about nsEventDispatcher::CreateEvent being able to create MessageEvents.  I suspect we don't and that those two hunks of the patch can be removed, since nothing actually would use that functionality now anyway, but I could easily be wrong.
(In reply to comment #9)
> The edit to nsGUIEvent.h may be unnecessary if we don't care about
> nsEventDispatcher::CreateEvent being able to create MessageEvents.

::CreateEvent could still create events using string "messageevent".
And adding NS_MESSAGE_EVENT, but not adding a new nsEvent struct is
kind of strange. So IMO no need to add anything to nsGUIEvent.h.


>+nsDOMMessageEvent::InitMessageEvent(const nsAString& aType,
>+                                    PRBool aCanBubble,
>+                                    PRBool aCancelable,
>+                                    const nsAString& aData,
>+                                    const nsAString& aDomain,
>+                                    const nsAString& aURI,
>+                                    nsIDOMWindow* aSource)
>+{
>+  if (!aSource)
>+    return NS_ERROR_NULL_POINTER;
Why aSource can't be null? HTML5 doesn't say that the method should throw if it is null.

> 	nsIDOMCommandEvent.idl			\
>+	nsIDOMMessageEvent.idl    \
> 	$(NULL)
No tabs before '\' ?

Btw, is it possible to link to some specific version of html5? The spec keeps
changing and the links may get wrong. Just that there would be some
reference to which the interfaces are based on and then one could compare
that to the latest version of html5.

Have you tested this with chrome; chrome posting to some other chrome window, 
chrome posting to content?
(In reply to comment #10)
> >+  if (!aSource)
> >+    return NS_ERROR_NULL_POINTER;
> Why aSource can't be null? HTML5 doesn't say that the method should throw
> if it is null.

Looks like a bit of detritus from fixing the crash I mentioned in a previous comment; it doesn't appear to be necessary any more (and isn't forbidden), so I'll remove it.


> > 	nsIDOMCommandEvent.idl			\
> >+	nsIDOMMessageEvent.idl    \
> > 	$(NULL)
> No tabs before '\' ?

I have my editor settings set to display tabs as two spaces, I think; I'll get this in the next update.


> Btw, is it possible to link to some specific version of html5? The spec keeps
> changing and the links may get wrong. Just that there would be some
> reference to which the interfaces are based on and then one could compare
> that to the latest version of html5.

#whatwg says that somewhere under http://dev.w3.org/cvsweb/ is the place for that info (when it's up, since it's not at the moment).


> Have you tested this with chrome; chrome posting to some other chrome window, 
> chrome posting to content?

The testing I've done is pretty much exactly what's in the patch by way of tests -- good practice in ensuring that it's clear exactly what testing has been done, not to mention making functionality harder to accidentally break in the future.  You raise a good point, tho -- I'm not sure chrome-to-content is safe, given that you're providing content with a chrome window object.  (Maybe wrappers might prevent you from doing anything with that, but best not to tempt fate.)  That probably isn't the best idea, and maybe we should throw a security error if the caller is chrome and the target isn't (not sure offhand whether IsCallerChrome tests that or just tests for privileges -- will need to investigate that).  Whatever we do, I'll add some tests for it.
The postMessage API has been implemented in Safari/WebKit and Opera.  If it were implemented in Firefox 3 as well, many web developers could begin using it and the chances of it being adopted by IE would be increased.

Both WebKit and Opera use the document.domain value for the "domain" of the message event, but this patch uses the principal's host:

> // Note: not the domain off the principal, because that might be lowered to
> // join with another domain, i.e. "foo.bar.com" lowered to "bar.com",
...
> rv |= docURI->GetHost(domain);

The HTML 5 spec is somewhat ambiguous on this point.  It reads:

> the domain attribute must be set to the domain of the document that the script
> that invoked the methods is associated with

Both Opera and WebKit have interpreted the "domain of the document" to refer to the document.domain value.  A full description of the principal sending the message would need to include the document.domain value as well as the principal's URL, so it makes some amount of sense to use the document.domain value here.
Jeff, what is the status of the latest patch? Is it still up-to-date and ready to go? If so I'll bring this up at the next gecko meeting on tuesday.
Adam, it would be great if you could raise this issue on the WHATWG mailing list. To me it seems more correct to use the true domain of the document rather than document.domain. Especially since document.domain can be set to any higher up domain without any verification at all that the receiver is aware of this.
It (or the same patch in my tree with whatever minimal ongoing unbitrotting
I've done) is good to go; only thing I think I wanted to do was get tests for
chrome->content posting as requested, and I can drop some time on that now and
get some churned out.
Ian Hickson has clarified the HTML 5 spec.  This patch implements the spec'd behavior.  I'll fill a bug against WebKit to make the message's domain property equal to the principal's host.
This takes care of everything mentioned in this bug -- patch should be good to go.

Note that this patch allows content to postMessage to chrome.  We could disable this at the cost of an extra check somewhere, but I don't see much point.  It'd slow down the code, in order for something to go wrong chrome must have used postMessage on content anyway, and anyone using postMessage must be checking the uri and domain properties anyway (and in any case, postMessage is string data anyway, minimizing the possible harm a fair amount).
Attachment #278391 - Attachment is obsolete: true
Comment on attachment 296672 [details] [diff] [review]
With tests for chrome->content, content->chrome, chrome->chrome

>Index: testing/mochitest/harness.xul

>     function populate() {
>       $("list-holder").setAttribute("rowspan", 1 + count);
>-      $("test-list").innerHTML += listContent.toLowerCase();
>-      $("test-table").innerHTML += tableContent.toLowerCase();
>+      $("test-list").innerHTML += listContent;
>+      $("test-table").innerHTML += tableContent;
>       $("wrapper").innerHTML += " "; // redraw the table
>     }

>Index: testing/mochitest/server.js

> function makeTags() {
>   // map our global HTML generation functions
>-  for each(var tag in tags) {
>-      this[tag] = makeTagFunc(tag);
>+  for each (var tag in tags) {
>+      this[tag] = makeTagFunc(tag.toLowerCase());
>   }
> }


Despite appearances these necessary parts of the patch, for in their absence, these bits of code conspire in the chrome Mochitest harness to cause the chrome test in the patch to be run from a URL which is entirely lowercase (breaking the uri-checking I do on the event posted from chrome to content).
This patch includes changes to make the tests runnable in non-Gecko-based browsers (Gecko-specific tests are present but guarded by a small check for the browser being Gecko-based).  This included things like removing |forEach| and adding more code to handle failures.  (These changes could be useful in shortening time-to-failure if some part of postMessage were ever to regress.  Previously, the tests pretty much universally relied on a postMessage response to terminate tests, but this one adds a sentinel test-finisher in case the main one fails to run.)  Last, I beefed up some a few tests, particularly the joined-principals one, which wasn't testing domain-joining as fully as it should have.  The patch has no substantive code changes.
Attachment #296672 - Attachment is obsolete: true
I'm currently getting this patch on a tryserver, but Windows sucks and defines a PostMessage macro which interferes with the current patch here, and my Windows build environment is slow enough that fixing this and testing for compile errors is taking forever.  That said, this is *almost* done, at which point I'll post the revised patch and provide URLs to the tryserver builds.
Attached patch I hate Windows (obsolete) — Splinter Review
This is the patch submitted to the tryserver, with support for Windows (PostMessage is apparently a Windows macro that breaks the build).  It's as ready for review as I can make it without more feedback.

Tryserver builds for this patch are available here; feel free to do whatever testing is necessary on them:

https://build.mozilla.org/tryserver-builds/2008-01-22_16:59-jwalden@mit.edu-postMessage/
Attachment #298427 - Attachment is obsolete: true
Attachment #298610 - Flags: review?(jonas)
Comment on attachment 298610 [details] [diff] [review]
I hate Windows

>+nsGlobalWindow::PostMessage(const nsAString& aMessage)
...
>+  // First, get the caller's window
>+  nsRefPtr<nsGlobalWindow> callerInnerWin = CallerInnerWindow();
>+  if (!callerInnerWin)
>+    return NS_OK;
>+  NS_ASSERTION(callerInnerWin->IsInnerWindow(), "should have gotten an inner window here");
>+
>+  // Obtain the caller's URI for the event
>+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(callerInnerWin->mDocument);
>+  if (!doc)
>+    return NS_OK;

Can you use nsContentUtils::GetDocumentFromCaller or nsContentUtils::GetDocumentFromContext here? I hate that we have so many code paths that all look different and that all do basically the same thing.

>+  nsCOMPtr<nsIURI> docURI = doc->GetDocumentURI();
>+  if (!docURI)
>+    return NS_OK;
>+
>+  // Note: we don't use the principal to get this information because in chrome
>+  // context, the principal is the system principal and knows nothing about the
>+  // current page.

Actually, I think you want to use the principal, otherwise we'll get the wrong domain for written into about:blank documents and probably a few other ones too.

Not sure what we want to do for chrome documents. We definitely want .source to be null. It'd also be cool if .domain was the first part of the chrome uri, i.e. 'mochitest' in your tests.

>+  // Create the event -- not sure the doc can be null, but to be safe, check.
>+  nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(mDocument);
>+  if (!docEvent)
>+    return NS_OK;

No need for this check, all documents implement this.

>+  // Finally, dispatch the event, ignoring the result to prevent an exception
>+  // from revealing anything about the document for this window.
>+  PRBool dummy;
>+  nsCOMPtr<nsIDOMEventTarget> targetDoc = do_QueryInterface(mDocument);
>+  targetDoc->DispatchEvent(message, &dummy);

Hmm.. I think you might need to actively cancel pending JS events if one exists.


>Index: modules/libpref/src/init/all.js
>+pref("capability.policy.default.Window.postMessage.get", "allAccess");

You need to add this to /minimo/customization/all.js too

>Index: testing/mochitest/server.js

>@@ -132,18 +132,17 @@ function runServer()
>                   .getService(Ci.nsIProperties).get("CurProcD", Ci.nsIFile);
>   serverBasePath.initWithPath(procDir.parent.parent.path);
>   serverBasePath.append("_tests");
>   serverBasePath.append("testing");
>   serverBasePath.append("mochitest");
>   server = new nsHttpServer();
>   server.registerDirectory("/", serverBasePath);
> 
>-  if (environment["CLOSE_WHEN_DONE"])
>-    server.registerPathHandler("/server/shutdown", serverShutdown);
>+  server.registerPathHandler("/server/shutdown", serverShutdown);

why is this no longer needed?

Would like to see the changes to PostMessage
Attachment #298610 - Flags: review?(jonas) → review-
Jst, do you know the answer to the GetDocumentFromCaller and the exception questions above?
I investigated the GetDocumentFrom(Caller|Context) part of this.  Context is definitely the wrong choice by my reading of the spec (will confirm with Hixie when I can catch him on IRC).  The spec says "the document that the script that invoked the methods is associated with", which I understand to be the function that called postMessage -- so for A/B same domain, if A calls a function on B which then calls postMessage, the source should be B, not A.  I can't find that I had a test for this, even tho I could have sworn I had one, so it's good you mention this.  :-)  I wrote a test, verified the current implementation passed it and then switched to Context to verify it failed -- all good so far.

I then tried a switch to Caller.  Adding the following code in place of the CallerInnerWindow() stuff:

  // First, get the caller's document
  nsCOMPtr<nsIDOMDocument> callerDoc = nsContentUtils::GetDocumentFromCaller();
  if (!callerDoc)
    return NS_OK;
  nsCOMPtr<nsIDocument> doc = do_QueryInterface(callerDoc);

and changing |callerInnerWin->GetOuterWindowInternal()| to |doc->GetWindow()| (which does return an outer window) causes the new source-test to pass.  However, other tests, specifically the postMessage_joined_helper2.html source-check and the postMessage_helper.html post-to-other-same-domain source-check, then fail:


not ok - unexpected test result got "unexpected data: unexpected source in message posted to window at: http://example.org/tests/dom/tests/mochitest/whatwg/postMessage_joined_helper2.html", expected "test-passed"

not ok - unexpected message: unexpected source in message posted to window at: <http://localhost:8888/tests/dom/tests/mochitest/whatwg/postMessage_helper.html>; evt.source == <[object Window @ 0x3daa4290 (native @ 0x3daa33cc)]>; window.parent == <[object Window @ 0x3d10cbc0 (native @ 0x3d10789c)]>


Changing doc->GetWindow() to the equivalent of |doc.defaultView| causes the same failures.  So there's *some* difference between using Caller and doing the calculations the posted patch (and setTimeout and setInterval) uses, but it's not clear to me what that difference is given the two methods work in radically different ways.


> Actually, I think you want to use the principal, otherwise we'll get the wrong
> domain for written into about:blank documents and probably a few other ones
> too.

Hmm, the spec doesn't directly address about:blank, but I think it says in that case domain should be null and uri should be about:blank.  I'll ask Hixie about it and see what's supposed to happen, and I'll make sure the next patch has a test for whatever's supposed to happen.


> Not sure what we want to do for chrome documents. We definitely want .source
> to be null.

This is just an nsContentUtils::IsCallerChrome() check, right?


> It'd also be cool if .domain was the first part of the chrome uri,
> i.e. 'mochitest' in your tests.

That happens already -- see test_postMessage_chrome.html.


> Hmm.. I think you might need to actively cancel pending JS events if one
> exists.

Assuming you meant exceptions, then it seems I do need to do this -- there's no web-visible effect, but the exception is logged to the console.  Is there some easier way to do this than to copy the top half of the CallerInnerWindow() function and call JS_ClearPendingException(), maybe a utility function or something?


> You need to add this to /minimo/customization/all.js too

Blah; what rules govern when this file needs to be edited, for future note?  I'd never even heard of it before.


> >-  if (environment["CLOSE_WHEN_DONE"])
> >-    server.registerPathHandler("/server/shutdown", serverShutdown);
> >+  server.registerPathHandler("/server/shutdown", serverShutdown);
> 
> why is this no longer needed?

This was just general complexity reduction as the presence of the handler when not needed is benign, but it's probably better off in a different bug/patch.


> Would like to see the changes to PostMessage

As soon we figure out whether Caller can work, I'm on it.
(In reply to comment #24)
> Context is definitely the wrong choice by my reading of the spec (will
> confirm with Hixie when I can catch him on IRC).  The spec says "the
> document that the script that invoked the methods is associated with",
> which I understand to be the function that called postMessage -- so for
> A/B same domain, if A calls a function on B which then calls postMessage,
> the source should be B, not A.

This is indeed what was intended.


> > Actually, I think you want to use the principal, otherwise we'll get the
> > wrong domain for written into about:blank documents and probably a few
> > other ones too.
> 
> Hmm, the spec doesn't directly address about:blank, but I think it says in
> that case domain should be null and uri should be about:blank.

As it turns out, our current behavior with the posted patch is correct, or at least will be when the spec attacks origins a bit more.  The domain/uri for a message sent from an about:blank window filled with content using open/write/close is the domain/uri of the script that did the writing.  It'd help me to have more guesses as to what situations might break so that I can make sure the current approach works, because as-is I don't see it causing any bugs.  I'm working on some tests for data: URLs (they even found a bug!), but other suggestions would be appreciated.
(In reply to comment #22)
> (From update of attachment 298610 [details] [diff] [review])
> >+nsGlobalWindow::PostMessage(const nsAString& aMessage)
> ...
> >+  // First, get the caller's window
> >+  nsRefPtr<nsGlobalWindow> callerInnerWin = CallerInnerWindow();
> >+  if (!callerInnerWin)
> >+    return NS_OK;
> >+  NS_ASSERTION(callerInnerWin->IsInnerWindow(), "should have gotten an inner window here");
> >+
> >+  // Obtain the caller's URI for the event
> >+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(callerInnerWin->mDocument);
> >+  if (!doc)
> >+    return NS_OK;
> 
> Can you use nsContentUtils::GetDocumentFromCaller or
> nsContentUtils::GetDocumentFromContext here? I hate that we have so many code
> paths that all look different and that all do basically the same thing.

Unfortunately I don't think we can use either of the helpers in nsContentUtils here :( GetDocumentFromCaller() is arguably the one we should use, but there are differences in how it works and how this code works. This code is more correct, as it gets the inner that's actually on the scope chain, GetDocumentFromCaller() doesn't seem to guarantee that. I'd recommend leaving this as is and filing a followup bug on fixing GetDocumentFromCaller() to do the right thing and start using it here, but at first thought I don't want to fix that for Firefox 3 as we've had hard to find regressions relating to how that works in the past, IIRC.

> >+  // Create the event -- not sure the doc can be null, but to be safe, check.
> >+  nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(mDocument);
> >+  if (!docEvent)
> >+    return NS_OK;
> 
> No need for this check, all documents implement this.

Commenting w/o enough context here, but mDocument can be null in a window if it's been closed already.

> >+  // Finally, dispatch the event, ignoring the result to prevent an exception
> >+  // from revealing anything about the document for this window.
> >+  PRBool dummy;
> >+  nsCOMPtr<nsIDOMEventTarget> targetDoc = do_QueryInterface(mDocument);
> >+  targetDoc->DispatchEvent(message, &dummy);
> 
> Hmm.. I think you might need to actively cancel pending JS events if one
> exists.

JS exceptions don't propagate across JS contexts, but if this code could ever run on the same JS context with a caller and callee from different origins, we don't need to worry about this.
> > Hmm.. I think you might need to actively cancel pending JS events if one
> > exists.
> 
> JS exceptions don't propagate across JS contexts, but if this code could ever
> run on the same JS context with a caller and callee from different origins, we
> don't need to worry about this.

Did you mean we *do* need to worry about this?

And couldn't you end up with a situation where Window A posts a message to Window B, which posts a message to Window A. If Window A then throws, would that exception still be pending once we return to Window A again?

(In reply to comment #25)
> > > Actually, I think you want to use the principal, otherwise we'll get the
> > > wrong domain for written into about:blank documents and probably a few
> > > other ones too.
> > 
> > Hmm, the spec doesn't directly address about:blank, but I think it says in
> > that case domain should be null and uri should be about:blank.
> 
> As it turns out, our current behavior with the posted patch is correct, or at
> least will be when the spec attacks origins a bit more.  The domain/uri for a
> message sent from an about:blank window filled with content using
> open/write/close is the domain/uri of the script that did the writing.  It'd
> help me to have more guesses as to what situations might break so that I can
> make sure the current approach works, because as-is I don't see it causing any
> bugs.  I'm working on some tests for data: URLs (they even found a bug!), but
> other suggestions would be appreciated.

If you want written-into-about:blank documents to act as if they have the domain of the window they were written from, then you should use the principal.

Basically a test would look like this:


addEventHandler("message", function(event) {
  is(event.domain == document.domain &&
     event.uri == location.href &&
     event.message == "hi");
}, false);
w = window.open();
w.write("<script>window.opener.postMessage('hi')</script>");
(In reply to comment #27)
> > > Hmm.. I think you might need to actively cancel pending JS events if one
> > > exists.
> > 
> > JS exceptions don't propagate across JS contexts, but if this code could ever
> > run on the same JS context with a caller and callee from different origins, we
> > don't need to worry about this.
> 
> Did you mean we *do* need to worry about this?

Yes, that's exactly what I meant. Sorry.
I made source null for postMessage from chrome, regardless whether it's ->content or not, which seemed simpler than doing a chrome-check on the target document.  This also made content->chrome posting silently fail due to the event being untrusted -- which is definitely a good thing, since that means exposing the chrome window as we were was exposing something that hadn't previously been exposed.

I also added the about:blank and data: tests -- see test_postMessage_special.xhtml and tell me what I'm missing, because the comments here said what that test tests shouldn't work if I'm not using the principal.

There's also miscellaneous consolidation of testing code in the non-test_* pages, to report as many errors as possible at once instead of short-circuiting early.  This probably also reduced line-number counts in those files, although the new test more than made up for it overall.
Attachment #298610 - Attachment is obsolete: true
Attachment #298878 - Flags: review?(jonas)
Here are some tryserver versions of the previous patch, since the chrome-source->null change is a fairly significant difference between the previous one.  Windows and Mac are done now, Linux hopefully following fairly soon-ish:

https://build.mozilla.org/tryserver-builds/2008-01-24_12:53-jwalden@mit.edu-postMessage/
How about this, will this work if you use the principal but not the document uri?

addEventHandler("message", function(event) {
  is(event.domain == document.domain &&
     event.uri == location.href &&
     event.message == "hi");
}, false);
w = window.open();
d = w.document;
s = d.createElement('script');
s.appendChild(d.createTextNode("window.opener.postMessage('hi');"));
d.body.appendChild(s);
Aha!  That indeed does fail with the old patch.  It doesn't with this patch, which first uses the URI from the principal and then falls back to the URI from the document (since the system principal has no URI).

One change from the previous patch: the uri/domain for the data: URL case changed to that of the parent window -- which probably makes sense given that we give data: URLs the principal of their parent/opener, I guess.
Attachment #298955 - Attachment is obsolete: true
Attachment #299127 - Flags: review?(jonas)
Attachment #298955 - Flags: review?(jonas)
The postMessage spec has a note that how IDN affects uri/domain isn't described; I forged ahead and gave us a specific behavior -- that the properties have the non-punycode value.  I also made the data:-test Moz-specific and fixed some style nits.
Attachment #299127 - Attachment is obsolete: true
Attachment #299251 - Flags: review?(jonas)
Attachment #299127 - Flags: review?(jonas)
at the fx3 meeting on 1/22 I think we talked about getting another build up on the try server to get a few people pounding on it.  is there another build with all the latest patches around?
It's in the pipeline as we speak.
So the pipeline got shot to pieces by infra bustage, and the minimo hunk in the patch requiring a custom mozconfig shot my chances at quickly getting builds to pieces, but I *think* eventually I'll get a Linux build at the following URL to complement the Win/Mac builds already there:

https://build.mozilla.org/tryserver-builds/2008-01-25_17:22-jwalden@mit.edu-postMessage-v10/
(In reply to comment #37)
> So the pipeline got shot to pieces by infra bustage, and the minimo hunk in the
> patch requiring a custom mozconfig shot my chances at quickly getting builds to
> pieces

Why are we caring about Minimo? dougt has long ago said that Minimo is dead, so I don't see any reason to worry about it.
There is no need to make changes to the all.js in mozilla/minimo.
just to be clear: I am not preventing anyone from making the change, just hoping that no one spends time thinking about it (sort of calls out to be removed at this point!).
Comment on attachment 299251 [details] [diff] [review]
With IDN testing and some nitpicks

r/sr=me, but would like jst to sr the inner/outer/caller window stuff.
Attachment #299251 - Flags: superreview+
Attachment #299251 - Flags: review?(jonas)
Attachment #299251 - Flags: review+
Comment on attachment 299251 [details] [diff] [review]
With IDN testing and some nitpicks

For added safety I think we should change the end of CallerInnerWindow() where we use nsGlobalWindow::FromWrapper() to do a QI to nsPIDOMWindow instead, and if that succeeds, then cast. That way we'll be guaranteed that the native we get from the scope chain really is a DOM window object (which it really should be, and apparently has been so far as the timeout code hasn't ever appeared to cause crashes, but just in case).

sr=jst for the DOM CallerInnerWindow() and FORWARD_TO_INNER_CREATE code that Jonas asked me to look at as well with the above issue dealt with.
Attachment #299251 - Flags: review+
Doh, forgot this comment:

>+class nsDOMMessageEvent : public nsIDOMMessageEvent,
>+                          public nsDOMEvent
>+{
>+public:
>+  nsDOMMessageEvent(nsPresContext* aPresContext, nsEvent* aEvent)
>+    : nsDOMEvent(aPresContext, aEvent)
>+  {
>+  };

Extra ';'s make GCC cry.
This should finally be good to go!  Want to get this in before freeze for the beta...
Attachment #299251 - Attachment is obsolete: true
Attachment #300199 - Flags: superreview+
Attachment #300199 - Flags: review+
Attachment #300199 - Flags: approval1.9?
Comment on attachment 300199 [details] [diff] [review]
Final patch with CallerInnerWindow and semicolon fixes

Ship it!
Attachment #300199 - Flags: approval1.9? → approval1.9+
All the unit test tinderboxen are liking this, and the rest of the tinderboxen are doing fine as well -- marking FIXED!  Thanks to smaug, jst, and sicking for drivebys during patch creation, questions about behavior that led to the addition of new testcases to the patch, and for review feedback.  Also a huge thanks to the WebKit people for asking about this -- wouldn't have made Firefox 3 without your request that this be re-evaluated; I wouldn't have thought there was a chance this would make 3.

I've got an email with implementation feedback, issues encountered along the way, and a link to a zip of the testcases included in this patch, with instructions on use, which I'll be sending to WHATWG momentarily.  Hopefully we can get some good implementation compatibility out of them, making things easier for the web developers that start using this functionality.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9beta3
Depends on: 414815
I added postMessage to the Firefox 3 for developers page, and the DOM:window.postMessage page documents pretty much everything interesting about the method to a web developer.  The new "MessageEvent" value that can be passed to document.createEvent was added to the relevant page.  A link to DOM:event.initMessageEvent was added to the DOM:event page.

The only things, more or less, that are missing are a DOM:event.initMessageEvent page and a MessageEvent page.  These two are far more drudge than actual interesting content, and I don't see a whole lot of use to them for web content, so I didn't write them -- for completeness they should probably be written by Firefox 3, but they should be low priority.

Comments on DOM:window.postMessage are appreciated.  I tried to describe every relevant detail of how the API works, but I could have missed something.  Also, if you happen to feel the very prominent warnings I added about checking the value of event.domain and maybe event.uri/event.source before using the sent data are insufficient, let me know.  :-)

http://developer.mozilla.org/en/docs/DOM:window.postMessage
Keywords: dev-doc-needed
Hourly build that contained this bug fix was the first in which launching Firefox 3 with -ProfileManager didn't work. This feature remains broken since some time on 01-29. See bug 414912. Please check to make sure you are not the reason why I can't use newer builds.
Works for me; furthermore, I can't see how the code changes here could possibly have affected the startup process.  I suggest you check out the other patches in that checkin range.
The other check-in is listed to be just adding new image files, so yours seems to be the most probable cause.
Jeff Walden:
nsIDOMWindowInternal.idl  ver 1.34 ln. 49:                                                                               
#ifdef PostMessage                                                              
#undef PostMessage                                                              
#endif                                                                          

This code breaks WinApi's PostMessage definition. And makes troubles to me.
I guess we could do the dance of something like

#ifdef PostMessage
#define PostMessage_Temp_Store PostMessage
#undef PostMessage
#endif

<interface goes here>

#ifdef PostMessage_Temp_Store
#define PostMessage PostMessage_Temp_Store
#undef PostMessage_TempStore
#endif

in the .idl file. :(
Sicking: the C pre-processor does not evaluate macros that way. Each body is substituted when its macro is called, and then rescanned for nested macros. So what you propose, given a use of PostMessage after the last #endif you show, would result in PostMessage_Temp_Store (I corrected the missing _ in the #undef):

$ cpp
#define PostMessage WindowsPostMessage

#ifdef PostMessage
#define PostMessage_Temp_Store PostMessage
#undef PostMessage
#endif

PostMessage

#ifdef PostMessage_Temp_Store
#define PostMessage PostMessage_Temp_Store
#undef PostMessage_Temp_Store
#endif

PostMessage

^D
# 1 "<stdin>"
# 1 "<built-in>"
# 1 "<command line>"
# 1 "<stdin>"







PostMessage






PostMessage_Temp_Store


/be
Suggest we hack up a different method name in the C++, instead of trying to #undef PostMessage.

/be
How?  The method in the API is "postMessage", and I'm not aware of a way to disconnect the method name in the interface/typelibs from the name it has in C++ or in headers.
Hixie, see how evil Windows macros preempt obvious names?

I'm suggesting you call the method something else, and use the JS API to give it the postMessage alias. Seriously.

/be
We can't really change the name since other browsers have already implemented this and there is already pages out there that use it.

We've been working around this exact problem many times before, and I think we've always ended up doing what the current patch does now.


Ivan: Could you simply include the appropriate windows header to get your define again?
http://msdn2.microsoft.com/en-us/library/hsttss76.aspx

At a glance this evil looks like it may work to avoid the completely
unnecessary JS API usage.  I'll investigate further.
The Windows tryserver likes this patch, and the others didn't spew errors.  There may be a more principled way to do the compiler check; I just grabbed a somewhat-random preprocessor macro from the following URL that looked like it'd be available in the MSVC++ compiler:

http://msdn2.microsoft.com/en-us/library/b0084kay.aspx

GCC also claims to support this, although it's apparently not documented:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35054

Intel's compiler also claims to support this at least on Linux:

http://www.google.com/url?sa=t&ct=res&cd=1&url=ftp%3A%2F%2Fdownload.intel.com%2Fsupport%2Fperformancetools%2Fc%2Flinux%2Fv9%2Fcref_cls.pdf&ei=vrGnR_zsMZrCerCL7YgD&usg=AFQjCNEhK53KoR7qyDLm3DQ5gUKSAWzTGQ&sig2=Xv0468kxMeKf3qxbCSdWlw

It'd almost be worth doing this for all Windows builds, except that someone using a weird compiler might get hit by the undefined PostMessage (the unrecognized pragma would be ignored).  I don't have the docs to try and support every such compiler, and I can't trivially test the patch in all these different compilers, so I'd prefer to either just fix MSVC++ (patches accepted, of course) or have someone else take this over to add support for said compilers.
Attachment #301408 - Flags: review?(benjamin)
(In reply to comment #58)
> We can't really change the name since other browsers have already implemented
> this and there is already pages out there that use it.

Hello, did I say change the JS name? I said use the JS API to bind it, but let the IDL and C++ name be other than the collide-y one.

But hey, if push/pop_macro works, great!

/be
Depends on: 397929
Comment on attachment 301408 [details] [diff] [review]
This works on Windows for at least the Windows tryserver compiler

The patch in bug 397929 is a better way to do this that doesn't require madness to implement/declare attributes/methods with C++ names that collide with Windows APIs.
Attachment #301408 - Flags: review?(benjamin)
Depends on: 417075
Depends on: 459236
Component: DOM: Mozilla Extensions → DOM
Depends on: 949488
postMessage dont work if origin is file://
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: