Open Bug 1440754 Opened 4 years ago Updated 2 years ago

Delay cross-docgroup postMessage by a tick

Categories

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

enhancement

Tracking

()

Fission Milestone Future

People

(Reporter: bzbarsky, Unassigned, NeedInfo)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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...
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.
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.)
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...
This just does an extra trip through the event loop.  We can add "run at end of task" complexity later if it becomes needed..
Attachment #8953703 - Flags: review?(nika)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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?
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.
What does Chrome do here? It seems we want consistency with Chrome and eventually standardize this?
I filed https://github.com/whatwg/html/issues/3506 to make progress on those questions.
> 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 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.
Attachment #8953703 - Flags: review?(nika) → review+
Priority: -- → P2
Component: DOM → DOM: Core & HTML
No longer blocks: oop-frames

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?

Flags: needinfo?(bzbarsky)

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

Flags: needinfo?(bzbarsky) → needinfo?(sledru)

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

Flags: needinfo?(sledru)

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

Fission Milestone: --- → Future

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.

Assignee: bzbarsky → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.