1440754 - Delay cross-docgroup postMessage by a tick

Status: NEW

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

Description

[Boris Zbarsky [:bzbarsky]]

Consider this testcase, where "win" is a cross-docgroup, hence possibly cross-process, iframe:

  win.postMessage("hey", "*");
  win.focus();

Per spec, focus() is synchronous.  So when the message event fires in "win", it can expect to be focused.

But in the cross-process case, if we convert both of these into async cross-process calls, we have a problem: the postMessage gets queued before the focus().  We have a few options here:

1)  Have the focus() call post some kind of high-priority runnable.  But there's no real guarantee that this will work.  For example, we could lose the timeslice between the two calls, such that by the time we're doing win.focus() the other process has received the postMessage runnable and run the message event.

2)  Delay posting of the postMessage runnable until end of "something" (current script execution, microtask checkpoint, etc, etc).  In theory it should be "end of run-to-completion".

Nika suggested we do #2 and have "something" be "a task" in this case.  That way we ensure that we do not actually send the message until our run-to-completion finishes, insofar as we do run-to-completion at all.  For example, if you do sync XHR between those lines above right now, you are already out of luck...

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

We talked about this on IRC, but just noting for the bug.  MessageChannel postMessage can go cross-origin and likely has a similar issue.  Its a separate task source from win.postMessage(), though.

Comment 2

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

So could we move all postMessages to dispatch-at-the-end-of-task model?
window.postMessage uses 'posted message task source' and MessagePort "port message queue".
(Nice inconsistency in the naming.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

The "port message queue" is defined to be a task source.  That is, each port has its own separate task source in the spec.

Dispatching at end of task for all postMessages, including same-origin ones, has compat worries, even if it _is_ allowed by spec...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: When doing cross-doc-group postMessage, delay the actual dispatch of the runnable by one event loop trip

This just does an extra trip through the event loop.  We can add "run at end of task" complexity later if it becomes needed..

Comment 5

[Boris Zbarsky [:bzbarsky]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ea97b6629d72f63aea08d6f89925ddee77ef360

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

There are other postMessage() methods that are not affected by the issue here because they are same-origin.  BroadcastChannel, ServiceWorker, Client, etc.

Olli, are you proposing to change them all to be consistent?

Comment 7

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

It would be nice to have consistency.

Adding extra trip through event loop might be bad for performance. We've got complains about postMessage latency for ages. Anything else dispatched to the event loop will increase the latency.
So at least in same-doc-group case we should try to avoid extra trip.

Comment 8

[Anne (:annevk)]

What does Chrome do here? It seems we want consistency with Chrome and eventually standardize this?

Comment 9

[Anne (:annevk)]

I filed https://github.com/whatwg/html/issues/3506 to make progress on those questions.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> What does Chrome do here?

I put this in https://github.com/whatwg/html/issues/3506#issuecomment-368510907 but briefly in --site-per-process mode if the postMessage() is before focus() the message recipient is not focused when it gets the message, but if you reverse the order of those calls it is.  That is, it has exactly the spec violation this discussion started with.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8953703 [details] [diff] [review]
When doing cross-doc-group postMessage, delay the actual dispatch of the runnable by one event loop trip

Review of attachment 8953703 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM but it might be good to figure out the performance and spec implications of this change before we merge it as mentioned in the other comments.

::: dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html
@@ +177,1 @@
>      

trailing whitespace.

@@ +182,2 @@
>  
> +  SimpleTest.finish();

So much cleaner. Wild. :p

@@ +184,4 @@
>  }
>  
>  window.addEventListener("message", receiveMessage, false);
>  window.addEventListener("load", run, false);

It might just be cleaner to use add_task here with the async function, but it's not a big deal so I'd just leave it as-is.

Comment 12

[Release mgmt bot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:bzbarsky, could you have a look please?

Comment 13

[Boris Zbarsky [:bzbarsky]]

This is somewhat blocked on the spec issue mentioned in comment 9. How do I annotate that?

Comment 14

[Sylvestre Ledru [:Sylvestre]]

The bot won't bother you anymore :)
So, clearing the need info (+your comment) will be enough

Comment 15

[Chris Peterson [:cpeterson]]

Tracking for Fission Future. Since this change is blocked on a spec issue, it doesn't need to block Fission MVP.

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is going to need a different owner....

As far as MVP or not, this could be a web compat issue. Chrome does NOT implement what the spec says, so we might need to do something here even without the spec issue being resolved. We can avoid it for the moment, but should be ready for possible site compat problems.

Comment 17

[demiobenour]

(In reply to Boris Zbarsky [:bzbarsky] from comment #16)

This is going to need a different owner....

As far as MVP or not, this could be a web compat issue. Chrome does NOT implement what the spec says, so we might need to do something here even without the spec issue being resolved. We can avoid it for the moment, but should be ready for possible site compat problems.

I suggest just doing what Chrome does here. Sites already have to support that.