430251 - (async-postMessage) Update to latest HTML5 and make postMessage dispatch its event asynchronously

Status: RESOLVED FIXED

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

Description

[Jeff Walden [:Waldo]]

Attachment: Patch, mostly test fixup

I spent some time changing our implementation/tests today to see how much work making it async would be.  All total it was about a day's work or so, the vast majority of which was devoted to fixing tests.  (I've come to the conclusion that async postMessage is absolutely horrible for back-and-forth conversation, particularly due to test_postMessage_origin.xhtml.)  This patch implements async dispatch and is basically 90% done.

The only remaining issue I know is that the async dispatch process means that postMessage events are marked as trusted; it's not clear whether or not this is a problem.  The one chrome test, however, suggests that it may be -- we currently don't allow non-chrome to call postMessage on chrome, because the created event is marked as untrusted, but with this patch it's marked as trusted because there's no JS on the stack to cause it to be marked untrusted.  Every test that fails (I didn't do a full run, but I did modify all the files a search for "postMessage" on MXR turned up, which should cover everything) does so because the events are marked trusted; either the tests or the implementation will have to be fixed so the tests pass again, before we can go further here.

Since I don't know that having the event be trusted is bad, and since this may or may not even happen anyway depending on spec activity, I'm leaving this as-is for now.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

If you need to bypass the set-event-trusted, you could use
nsEventDispatcher::Dispatch().
nsEventDispatcher::DispatchDOMEvent() is the one which sets event trusted.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Better patch, ready for review if it becomes necessary

This adds logic to make the dispatched event untrusted; no test needed any more changes to work fully.

Astute readers will have noticed that in the previous patch, I grabbed the inner window from |this| in PostMessageMoz and then passed that along with the runnable.  This is probably highly non-kosher and probably isn't what would be specified (particularly given that inner window reuse makes it impossible to actually truly bind the window at postMessage time), so this patch passes the outer window and then computes the inner window at dispatch time.

The only real question as I see it is whether the extra logic in FORWARD_TO_INNER_CREATE that isn't in |GetCurrentInnerWindowInternal| is needed or not.  All tests pass with it this way, so I'm inclined to think the create case doesn't matter.  Further, reading the code, I don't see how _CREATE is any different from the FORWARD_TO_INNER with the right error code -- if we're an outer window with no inner window, calling GetDocument and tracing through the calls that happen by inspection seems to suggest no inner window would be created anyway.  Was _CREATE useful in the past but has since become dead code?

Comment 3

[Jeff Walden [:Waldo]]

It's decided; I'm updating the patch to reflect the last couple nits in the spec changes that aren't in the patch here.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Final patch, updated to very latest spec

Okay, this is completely updated.  Things to note since last:

Apparently postMessage events changed from bubbling to non-bubbling and gained a lastEventId field (for server-sent events; postMessage sets it to "") a few days ago (<http://html5.org/tools/web-apps-tracker?from=1478&to=1480&context=10>).  I updated IDL, implementation, and tests to account for this.

The origin argument to postMessage is non-optional now, and "*" now denotes "don't care".  I could have updated the existing tests that didn't care by using "*", but in the interests of robustness I made the effort to determine the "real" origin for most of them, or used |evt.origin| when sending to |evt.source|.  This took rather longer than I'd anticipated it would take.

There's one bug in here (also present in current tree) that prevents specifying a chrome origin as the explicit required target origin (due to chrome: URIs being made immutable by nsChromeProtocolHandler); this is an edge case that would be difficult to fix now without breaking/slowing down the rest of the code, so I'm ignoring it for now.  Posting to chrome is enough of an edge case that I don't think we should care (at the very least, for now), and you can still use "*" at the cost of identity checking (but then again, you're trusted code, so this doesn't seem like it actually matters).

As it turns out, test_postMessage_onOther.html wasn't actually testing what it was supposed to test after the uri/domain->origin change, so I rewrote it with a dash of document.domain to actually test what it was supposed to test.  Yay for rereading code, I guess.  :-)

I actually have run all Mochitests in all flavors now, and we pass everything.

If there's any way to reliably control origin changes with respect to the event queue, we could add tests for the origin at dispatch rather than the origin at postMessage being the one used, but I don't know if we have anything that can give that kind of control reliably.  If we do, I'll try to spin up a (separate) test for it -- if code inspection suggests we're okay here and we're otherwise well-tested, I don't think a delay is practical or worthwhile given where we are now.

The _CREATE question still stands, so I'm directing this to jst at least for that issue -- beyond that, sicking for the rest seems to make sense since he's reviewed all the other postMessage patches.

Comment 5

[Jeff Walden [:Waldo]]

This wants to make the release, because once it's out and in use it's massively more difficult to change it (particularly since the changes aren't backwards-compatible).

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

How certain can we be that the spec (draft) doesn't change again?

Doesn't Opera have some support for postMessage? That is synchronous?

Comment 7

[Hixie (not reading bugmail)]

We can't never be certain that the spec won't change again. Just look at CSS2, which was "finished" in 1998 and is still undergoing changes now, 10 years later. HTML5 isn't even "finished" yet.

However, at this point the design is pretty stable. The only change that is likely is related to what the events are fired at, and that won't change any time soon.

Comment 8

[Jeff Walden [:Waldo]]

I'm not particularly worried about the properties of postMessage events changing.  We have exactly the details required for same-origin checking in .origin, the message in .data, and .source for replies, and you can't really pare down further than that without sacrificing something obviously necessary for safe two-way communication.  That any such change requires an update to initMessageEvent(NS) I'm not so much a fan of, but I think having numerous different implementations relying on this as it is now will be sufficient to cool any unexpected desires to change where we can't all agree they're absolutely necessary.

I'm also not worried about the details of dispatch changing.  The sync/async is was the biggest outstanding issue, having been around since I first started looking at this (even asked about it on the mailing list as I recall).  Since nobody'd really played with it at that time I only got a sync clarification and no arguments for async -- time and experience have changed that.  In this last update the algorithm itself changed, but it more or less changed exactly as much as was needed to make it async.  The fundamentals are the same, even if they didn't translate unchanged into the new algorithm (or our implementation of it) due to having to split computations between postMessage and dispatch.  So the important thing here isn't how much changed, but how much stayed the same in its essential nature.

When it comes down to it there are only two things that really remain, as I see it -- whether the event bubbles and where it fires.  The former changed in the last few days, and I'm not entirely sure why.  Changing that is about five lines (change a constant, flip meaning of the single method checking for bubblyness in tests), so at least that change would be simple to make.  I don't really understand this question about where the events are fired, tho -- they've fired at the document for as long as I can remember, and I don't really see a good reason to move elsewhere.  Moving beneath document isn't possible when Window and HTMLDocument are non-HTML-specific and must be implemented even for non-HTML content, and I see the choice of one or the other as a tossup.  At that point, we have what we have, and why bother changing?  I don't recall the where-it-fires thing ever being raised before as a serious remaining issue, or I'd have pushed on it further at the time (same for the bubbling thing, for that matter).

Comment 9

[Hixie (not reading bugmail)]

I don't remember changing whether the event bubbles -- which revision did that change in?

The only reason we would dispatch the event at the <body> element is to enable <body onmessage=""> handling. Right now you can't declare your handler in the markup, since there is no "document" node in the markup.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

if <body>'s onfoo attributes worked like in gecko, the event listener would be
registered to |window| and if the event bubbles, then event would be caught.
Not sure what HTML5 currently specifies about onfoo attributes and
|window|'s event handling.

Comment 11

[Hixie (not reading bugmail)]

Right now that part of the spec is a mess (and all the browsers do things slightly differently).

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #9)
> I don't remember changing whether the event bubbles -- which revision did that
> change in?

r1237 (mental mixup over exactly when the change happened, from juggling
commit-watchers and web-apps-tracker tools):

http://lists.whatwg.org/htdig.cgi/commit-watchers-whatwg.org/2008/000436.html

> The only reason we would dispatch the event at the <body> element is to enable
> <body onmessage=""> handling. Right now you can't declare your handler in the
> markup, since there is no "document" node in the markup.

Why is <script>document.addEventListener("message", function(e) { },
false);</script> not declared in the markup?  And really, will people generally
be doing things sufficiently un-complicated with this that they should be
putting it all in an attribute in markup?  You have to check origin at a
minimum, plus do whatever you're going to do with the data, and it's going to
get pretty big pretty quick.

Comment 13

[Damon Sicore (:damons)]

While we MUST make a decision here, the default is to continue with our synchronous implementation.  I say we don't hold the release while we are in limbo making a decision.  If this is ready (i.e., it gets solid reviews and good feedback from the community), then request approval1.9.

In the meantime, -'ing.

Comment 14

[Hixie (not reading bugmail)]

jwalden: Oh yeah, had to make it not bubble for <event-source>. On the other thing, I agree, I was just saying that those were the reasons for considering <body onmessage="">. That whole part of the spec (events targetted at body/document/window, bubbling or being redirected to/from, window.onfoo and body.onfoo and document.onfoo having magical relationships, etc) needs work.

Anyway. Spec is stable. I only made the change because y'all said you could handle it even at this late stage, so it would be sad if the patch wasn't taken. :-)

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Change event target to window, last remaining issue now resolved

A little more discussion in whatwg@ and we've agreed to switch the event's target to be the window upon which postMessage is called.  With that change, everything Hixie wanted for forward-compatibility of a potential onmessage attribute on <body> is there, and I think we've covered everything anybody had any intention of changing with respect to postMessage for the foreseeable future.

Comment 16

[Jeff Walden [:Waldo]]

(For those who might not have thought to consider it, whether the postMessage event bubbles or not is immaterial when the target changes to window.  Why?  That's where bubbling ends -- any event which bubbles goes up to window and after being dispatched at window listeners isn't dispatched anywhere else.  Regardless whether it bubbles or doesn't bubble, as long as the target is window the exact same listeners will be called either way, no matter where you register the listeners.  So in the end, the change of target nullifies any concerns about whether the postMessage event bubbles or not.)

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #13)
> While we MUST make a decision here, the default is to continue with our
> synchronous implementation.  I say we don't hold the release while we are in
> limbo making a decision.  If this is ready (i.e., it gets solid reviews and
> good feedback from the community), then request approval1.9.

No limbo on decision, just waiting for reviews. Are either jst or sicking around today? If not, how about bz?

/be

Comment 18

[Brendan Eich [:brendan]]

Based on Hixie's testimony that the spec is stable now, and the incompatible difference with trunk, we had better ship this in Firefox 3.

/be

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm not likely to have time to adequately review this before May 5, especially since I'd need to go read the spec first.

It's probably worth having smaug look at the event dispatch part; I'm not sure whether it would make more sense to push the right JSContext* on the stack and then dispatchEvent().

I'm also not convinced about the origin comparison; is there a reason we aren't using checkSameOriginURI or some such?  Or nsIScriptSecurityManager::checkSameOrigin if we have a JSContext on one end.?  Or even nsIPrincipal::checkMayLoad?  Perhaps the spec defines the behavior for jar: URIs or something in a way that means we can't use our normal definition of same-origin?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 317935 [details] [diff] [review]
Change event target to window, last remaining issue now resolved

Def want Smaugs review on this given the lateness of the patch.

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Use CheckSameOriginURI

I modeled the async event dispatch off how nsPLDOMEvent works, plus some inline expansion of nsGlobalWindow::DispatchEvent with a tweak to modify the trusted bit, modeled off nsEventDispatcher::DispatchDOMEvent.

CheckSameOriginURI seems like it should work here -- sort of.  The spec doesn't speak to jar: so I think we want to un-nest before comparing; I think postMessage to or from jar: URIs would fail as is now.  file: is a more interesting case, and HTML5 doesn't differentiate it from other URIs (so all files are same-origin).  All of the Check* functions have funky file: handling, so technically we'd be going against the spec using it without fallback logic of some sort; do we care?  This is a rare enough use case I sort of think we don't.

I'll write up a test of jar: functionality to fold in the next iteration of the patch, but given the schedule it's better I post this than wait until the testcase is finished, so reviews can start sooner.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 318379 [details] [diff] [review]
Use CheckSameOriginURI

>+NS_IMETHODIMP
>+PostMessageEvent::Run()
>+{
>+  NS_ABORT_IF_FALSE(mTargetWindow->IsOuterWindow(),
>+                    "should have been passed an outer window!");
>+  NS_ABORT_IF_FALSE(mSource->IsOuterWindow(),
>+                    "should have been passed an outer window!");
>+
>+  nsRefPtr<nsGlobalWindow> targetWindow =
>+    mTargetWindow->GetCurrentInnerWindowInternal();
>+  NS_ABORT_IF_FALSE(targetWindow->IsInnerWindow(),
>+                    "we ordered an inner window!");

What if inner window changed after posting the message event? What should happen?
...
>+    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
>+    nsresult rv =
>+      ssm->CheckSameOriginURI(mProvidedOrigin, targetURI, PR_TRUE);
>+    if (NS_FAILED(rv))
>+      return rv;
>+  }

Should you make security checks when posting the message event, not when
dispatching it? Or perhaps in both cases, if we allow different inner windows.
Or does postmessage already have those checks (/me needs to look at that).
Add some comment if the checks are in both places.

> NS_IMETHODIMP
> nsGlobalWindow::PostMessageMoz(const nsAString& aMessage, const nsAString& aOrigin)
> {
>-  FORWARD_TO_INNER_CREATE(PostMessageMoz, (aMessage, aOrigin));
>+  // NB: Since much of what this method does must happen at event dispatch time,
>+  //     this method does not forward to the inner window, unlike most other
>+  //     methods.
Could you clarify this comment.

>+  // Create and asynchronously dispatch a runnable which will handle actual DOM
>+  // event creation and dispatch.
>+  PostMessageEvent* event =
>+    new PostMessageEvent(nsContentUtils::IsCallerChrome()
>+                         ? nsnull
>+                         : callerInnerWin->GetOuterWindowInternal(),
>+                         NS_ConvertUTF8toUTF16(origin),
>+                         aMessage,
>+                         this,
>+                         providedOrigin,
>+                         nsContentUtils::IsCallerTrustedForWrite());
>+  return NS_DispatchToCurrentThread(event);

use nsCOMPtr<nsIRunnable> so that if NS_DispatchToCurrentThread fails, event
is deleted properly.

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Addressing those comments

(In reply to comment #22)
> What if inner window changed after posting the message event? What should
> happen?

Exactly what happens here.  The location of the document in the window is only compared to |providedOrigin| when the event is about to be dispatched, precisely to avoid time-of-check/time-of-use issues.  If the window changes location and the postMessage caller specified a different location, the event isn't dispatched ("abort these steps silently").

http://www.whatwg.org/specs/web-apps/current-work/multipage/section-crossDocumentMessages.html#processing4


> Should you make security checks when posting the message event, not when
> dispatching it? Or perhaps in both cases, if we allow different inner windows.
> Or does postmessage already have those checks (/me needs to look at that).
> Add some comment if the checks are in both places.

Security checks are only supposed to happen immediately before the event is to be dispatched.  There's not that much gain to checking at time of posting since that location has no relevance to the location when the event is to be dispatched, in the case that this setup is being attacked by a malicious site.  Also, I think it's reasonable to assume that when you call the method you usually "know" where the window is, so it's either wasted effort or "your own problem".


> >+  // NB: Since much of what this method does must happen at event dispatch time,
> >+  //     this method does not forward to the inner window, unlike most other
> >+  //     methods.
> Could you clarify this comment.

Done.  To be honest, I feel like I'm being over-verbose in the explanations in this revised patch, which is a bit odd as I usually find myself on the other side of that argument.


> use nsCOMPtr<nsIRunnable> so that if NS_DispatchToCurrentThread fails, event
> is deleted properly.

I had some weird memory of a thread-related API to which you could dispatch new objects and which would delete on failure, avoiding an addref/release, but on second check here it wasn't this API (if it even exists).  I used an |nsRefPtr<PostMessageEvent>| to avoid having to make virtual calls (possibly even calls, if addref/release can be inlined) to release the pointer, for compilers which don't make that optimization themselves.

Comment 24

[Jeff Walden [:Waldo]]

Hm, I think I forgot to handle a jar: caller in the patch.  Testcasing to verify that I do indeed need to make more tweaks...

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 318409 [details] [diff] [review]
Addressing those comments

>RCS file: /cvsroot/mozilla/dom/public/idl/base/nsIDOMWindowInternal.idl,v
>RCS file: /cvsroot/mozilla/dom/public/idl/events/nsIDOMMessageEvent.idl,v
update uuid when changing interfaces 

>+NS_IMETHODIMP
>+PostMessageEvent::Run()
>+{
>+  NS_ABORT_IF_FALSE(mTargetWindow->IsOuterWindow(),
>+                    "should have been passed an outer window!");
>+  NS_ABORT_IF_FALSE(mSource->IsOuterWindow(),
>+                    "should have been passed an outer window!");
Here you use mSource, which is possibly initialized to null:
...
>+  // Create and asynchronously dispatch a runnable which will handle actual DOM
>+  // event creation and dispatch.
>+  nsRefPtr<PostMessageEvent> event =
>+    new PostMessageEvent(nsContentUtils::IsCallerChrome()
>+                         ? nsnull
>+                         : callerInnerWin->GetOuterWindowInternal(),
>+                         NS_ConvertUTF8toUTF16(origin),
>+                         aMessage,
>+                         this,
>+                         providedOrigin,
>+                         nsContentUtils::IsCallerTrustedForWrite());


> NS_IMETHODIMP
> nsGlobalWindow::PostMessageMoz(const nsAString& aMessage, const nsAString& aOrigin)
> {
>-  FORWARD_TO_INNER_CREATE(PostMessageMoz, (aMessage, aOrigin));
>+  // NB: Since much of what this method does must happen at event dispatch time,
>+  //     this method does not forward to the inner window, unlike most other
>+  //     methods.  We do this because the only time we need to refer to this
>+  //     window, we need a reference to the outer window (the PostMessageEvent
>+  //     ctor call), and we don't want to pay the price of forwarding to the
>+  //     inner window for no actual benefit.
>+  NS_ABORT_IF_FALSE(IsOuterWindow(), "only call this method on outer windows");
You say it isn't possible to call this method on inner window? How is that prevented?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #25)
> You say it isn't possible to call this method on inner window? How is that
> prevented?
Or at least for binary usage it should be documented that window must be the
outer window. Though, why not just fix it to work with inner window too?

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Patch, v5

Sigh, I'm rushing myself too much.  The jar: omission I spotted did indeed turn out to be valid; the testcase in this patch now passes (source for the file in the jar is basically receive the event, check a few properties, and then post back with the results, it's easy enough to mentally fill in the blank).


> >RCS file: /cvsroot/mozilla/dom/public/idl/base/nsIDOMWindowInternal.idl,v
> >RCS file: /cvsroot/mozilla/dom/public/idl/events/nsIDOMMessageEvent.idl,v
> update uuid when changing interfaces 

I could have sworn I updated the second one.  As for the first, it's technically unnecessary because it's not changing binary compatibility (check nsIDOMWindowInternal.h file for ShowModalDialog, for example), but I bumped it anyway.


> >+  NS_ABORT_IF_FALSE(mSource->IsOuterWindow(),
> >+                    "should have been passed an outer window!");
> Here you use mSource, which is possibly initialized to null:

Rushing too fast, added a !mSource || to that.


> You say it isn't possible to call this method on inner window? How is that
> prevented?

Two things: one, compiled code has better ways to do this and already will trigger an assertion in nsGlobalWindow::CallerInnerWindow (so indeed I couldn't fix it for binary usage), and two, script can only get references to outer windows.  I expanded the comment to note this.


I think this addresses everything and doesn't make stupid mistakes like have been in the past couple iterations.  Unfortunately local interdiff croaks on the patch due to the funky line endings in extensions/cookies, so I can't review the deltas between versions of the patch.  :-\

Comment 28

[Boris Zbarsky [:bzbarsky]]

fwiw, it seems like nsIPrincipal::GetOrigin() might more or less do what you want in terms of the innermost uri, no userpass, etc thing.  Not sure about the system principal issues, so this might be fodder for a separate bug.

It just bothers me when I see manual origin-construction like that.  All such code should live in only one place, imo.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 318431 [details] [diff] [review]
Patch, v5

>+  nsresult rv = message->InitMessageEvent(NS_LITERAL_STRING("message"),
>+                                          PR_FALSE /* non-bubbling */,
>+                                          PR_TRUE /* cancelable */,
>+                                          mMessage,
>+                                          mCallerOrigin,
>+                                          NS_LITERAL_STRING(""),
Why not EmptyString() ?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 318431 [details] [diff] [review]
Patch, v5

r=me, but note comment #28

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 318431 [details] [diff] [review]
Patch, v5

Hmm.. rather than the target-origin thing, wouldn't it be better if posted messages got canceled if the window was navigated away from. If a user navigates from one page to another within the same site, it seems unexpected that the message got posted to the new window.

However this part I think is something that is edge-casey enough that we should deal with it after initial release.

>   [binaryname(PostMessageMoz)] void postMessage(in DOMString message,
>-                                                [optional] in DOMString origin);
>+                                                in DOMString providedOrigin);

s/providedOrigin/targetOrigin/ per spec.


>Index: dom/src/base/nsGlobalWindow.cpp

>+PostMessageEvent::Run()
>+    nsIPrincipal* targetPrin = targetWindow->GetPrincipal();
>+    if (!targetPrin)
>+      return NS_OK;
>+    nsCOMPtr<nsIURI> targetURI;
>+    if (NS_FAILED(targetPrin->GetURI(getter_AddRefs(targetURI))))
>+      return NS_OK;
>+    if (!targetURI) {
>+      nsCOMPtr<nsIDocument> targetDoc =
>+        do_QueryInterface(targetWindow->mDocument);
>+      if (!targetDoc)
>+        return NS_OK;
>+      targetURI = targetDoc->GetDocumentURI();
>+      if (!targetURI)
>+        return NS_OK;
>+    }
>+
>+    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
>+    nsresult rv =
>+      ssm->CheckSameOriginURI(mProvidedOrigin, targetURI, PR_TRUE);

This isn't going to work very well if the targetURI is a file-uri, is it? Does the spec specify how those work?

Again, this is something we can deal with in a dot-release IMHO, but please file a bug on it.

>+  nsresult rv = message->InitMessageEvent(NS_LITERAL_STRING("message"),
>+                                          PR_FALSE /* non-bubbling */,
>+                                          PR_TRUE /* cancelable */,
>+                                          mMessage,
>+                                          mCallerOrigin,
>+                                          NS_LITERAL_STRING(""),
>+                                          mSource);

EmptyString()


Looks good otherwise.

Comment 32

[Adam Barth]

(In reply to comment #31)
> Hmm.. rather than the target-origin thing, wouldn't it be better if posted
> messages got canceled if the window was navigated away from. If a user
> navigates from one page to another within the same site, it seems unexpected
> that the message got posted to the new window.

Keep in mind that the window might have been navigated between the time it was created and the time postMessage was called.  It seems reasonable to deal with the two cases uniformly.

Comment 33

[Jeff Walden [:Waldo]]

(In reply to comment #31)
> >   [binaryname(PostMessageMoz)] void postMessage(in DOMString message,
> >-                                                [optional] in DOMString origin);
> >+                                                in DOMString providedOrigin);
> 
> s/providedOrigin/targetOrigin/ per spec.

Okay, as long as we're only doing it here.  I found it confusing to describe this as targetOrigin in the implementation because it's not the target origin -- it's the "required" target origin.  I found myself confusing variable names when I used the spec's naming, particularly when doing the origin comparison, whereas providedOrigin is unambiguously the user-provided argument.


> >Index: dom/src/base/nsGlobalWindow.cpp
> >+    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> >+    nsresult rv =
> >+      ssm->CheckSameOriginURI(mProvidedOrigin, targetURI, PR_TRUE);
> 
> This isn't going to work very well if the targetURI is a file-uri, is it? Does
> the spec specify how those work?
> 
> Again, this is something we can deal with in a dot-release IMHO, but please
> file a bug on it.

It probably isn't a good idea to have two different security policies for how file: URIs are treated.  The spec groups all file URIs into a single origin right now, but I can imagine this is something that it might be possible to get changed.  file: isn't really the target of this API for now anyway, and I don't think it's entirely unreasonable to say (for now at least) that pages on the local file system can't use postMessage for sensitive data (or that if they do, they use some encryption scheme to do it over an "insecure" channel).

I'll try to propose something to whatwg@ when I next have time, and I'll get the bug filed before I commit (not now, both because I probably still want a jst-review and because I don't have time now).

Comment 34

[Brendan Eich [:brendan]]

Agree with Adam that navigation is immaterial, origin is the thing (I think that's his point).

file: as single trust domain is a problem in general. We could be future-proof and only testing-hostile if we made postMessage in file: script illegal for now (pref for testing). This is an extra (apparently unmotivated, by itself) layer of defense, but we've been burned by file: before. Comments?

/be

Comment 35

[Boris Zbarsky [:bzbarsky]]

> file: as single trust domain is a problem in general.

We no longer have this problem.  The spec does, but this patch doesn't follow the spec for file://

Comment 36

[Brendan Eich [:brendan]]

(In reply to comment #35)
> > file: as single trust domain is a problem in general.
> 
> We no longer have this problem.  The spec does, but this patch doesn't follow
> the spec for file://

Ah, bug 230606. Ok, so long as we have one rule for file: trust relations, even if we diverge on this point from the postMessage spec, I'm good.

/be

Comment 37

[Johnny Stenback (:jst)]

Comment on attachment 318431 [details] [diff] [review]
Patch, v5

- In PostMessageEvent::Run():

+    if (!targetURI) {
+      nsCOMPtr<nsIDocument> targetDoc =
+        do_QueryInterface(targetWindow->mDocument);

targetWindow->mDoc is already mDocument QI'ed to nsIDocument, so you can use that here w/o QI or reference counting overhead.

+    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
+    nsresult rv =
+      ssm->CheckSameOriginURI(mProvidedOrigin, targetURI, PR_TRUE);
+    if (NS_FAILED(rv))
+      return rv;

This is in ::Run(), might as well return NS_OK here as well. Having said that, it seems to me (though according to Jonas this isn't what the spec says to do, but still worth thinking about) that if the user of this API explicitly passes in a target URI and we know at the time postMessage() is called that the target window is *not* from the given URI, we should throw there so the caller knows. Obviously this *could* (even though it's pretty unlikely) the target windows origin could change in the time between postMessage() and ::Run() so that the event actually would fire, but depending on that is stretching it pretty far IMO. This is of course material for a different (and post 1.9) bug if we decide to consider this.

+  return nsEventDispatcher::Dispatch(static_cast<nsPIDOMWindow*>(mTargetWindow),
+                                     presContext,
+                                     internalEvent,
+                                     message,
+                                     &status);
+}

Same here, return NS_OK unconditionally IMO.

r=jst with that.

Comment 38

[Hixie (not reading bugmail)]

(The spec now allows Mozilla's behaviour for file://)

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 318431 [details] [diff] [review]
Patch, v5

The patch should be very safe. It only touches postMessage code which is very much removed from the rest of the code so regressions elsewhere should be very unlikely. And we've got good test coverage of postMessage itself too, so regressions there should be unlikely as well.

Comment 40

[Jeff Walden [:Waldo]]

Attachment: Final patch with comments addressed

Incidentally, the interdiff bustage was probably due to manually editing out hunks of the patch in an editor which normalizes line endings, as I realized just before I was going to attempt to apply the latest patch to a clean tree from which it could easily be committed.  interdiff works on this version against one of the earlier ones that didn't touch extensions/cookie.

Comment 41

[Damon Sicore (:damons)]

Comment on attachment 318779 [details] [diff] [review]
Final patch with comments addressed

a1.9+=damons

Comment 42

[Jeff Walden [:Waldo]]

I'll commit this either later this afternoon or late tonight, depending when I have time.

Comment 43

[Jeff Walden [:Waldo]]

Tinderboxen were not green at a time when I could commit, and they still aren't (in fact, Linux hasn't been green for the last twelve hours).  Punting to this afternoon, sadly, when there shall hopefully be more greenness...

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking based on comment 18

Comment 45

[Jeff Walden [:Waldo]]

Landed; bug 431935 as followup for figuring out some nicer behavior for sending from and sending to file:.