1315803 - don't let postMessage() flood the main thread

Status: RESOLVED INCOMPLETE

(Core :: DOM: postMessage, defect, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This builds on the timer ThrottledEventQueue work in bug 1300659.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c36464ef409498f90ad4ee2b4c232137086b561e

The back pressure part of this patch is not quite right.  I may not implement back pressure immediately since we never stop message events today.

Comment 1

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

One thing people have asked is as fast postMessage (between windows) as possible, not being blocked but other stuff.
Just something to keep in mind.

Comment 2

[violet.bugreport]

It's very similar to bug 1514413, this one is postMessage, that bug is Http request. If the parent gets flooded by those events, the browser will completely hang, some extreme case may even force user to reboot the machine.

Chrome doesn't have this problem.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The existing two dependencies were resolved. What work is left here?

Comment 4

[Simon Giesecke [:sg] [he/him]]

When trying to apply the patch to the current head revision, I found that

• Bug 1321903 resp. https://hg.mozilla.org/mozilla-central/rev/9439982efdc3a72db5ae3f798649fe558eea71c6 moved the back-pressure mechanism to TimeoutManager
• Bug 1363829 resp. https://hg.mozilla.org/mozilla-central/rev/bd452eda2e83eea0cabb28259106a854d8dcf847 then removed the back-pressure mechanism from TimeoutManager
• Bug 1453925 resp. https://hg.mozilla.org/mozilla-central/rev/71b23fab4c0b8a450cf906a725f55171c0e81638 removed the dispatching of PostMessage, since it already is asynchronous via the IPC mechanism

The last point removes the ability to directly put the ThrottledEventQueue, but this change might be reverted.

Andrew, could you provide some guidance if implementing this still makes sense? I can supply a rebased patch if this makes answering this easier.

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1315803 - Add DispatchToCurrentThread function that uses ThrottledEventQueue on the main thread. r=#dom-workers-and-storage

Depends on D57799

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1315803 - Use DispatchToCurrentThread in Close and Shutdown, and make use of convenience functions. r=#dom-workers-and-storage

Depends on D57800

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1315803 - Use DispatchToCurrentThread in PostMessage. r=#dom-workers-and-storage

Depends on D57801

Comment 8

[Simon Giesecke [:sg] [he/him]]

I now adapted the patch, split up into 3 patches, together with some cleanup, so maybe comment on Phabricator directly.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

Andrew, could you provide some guidance if implementing this still makes sense? I can supply a rebased patch if this makes answering this easier.

I think we should abandon the patchset and close the bug in favor of other bugs that are about the problem than a patch holding bug. This is a real problem, but is likely best considered at a higher level more comprehensively as part of the various scheduler changes coming up and including the performance team. I'm planning to attend https://berlinallhandsjanuary2020.sched.com/event/YwoR/workshop-scheduler-design-update-and-next-steps where I'm interested in understanding if that work will interact with this problem space.

In particular:

• Various worker debugger changes have been happening to better support worker debugging and I wouldn't want to land random changes and risk complicating things for :bhackett.
• Throttling that doesn't actually provide some variant of backpressure or throttling (ex: token bucket) results in a memory leak, which isn't necessarily an improvement, and there's no analysis on this here. (Which is part of my prior paragraph point.)

Comment 10

[Jens Stutte [:jstutte]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #9)

I think we should abandon the patchset and close the bug in favor of other bugs that are about the problem than a patch holding bug. This is a real problem, but is likely best considered at a higher level more comprehensively as part of the various scheduler changes coming up and including the performance team.

Hi Andrew, to be more concrete: Do we have a bug we can dupe this on then?

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jens Stutte [:jstutte] from comment #10)

Hi Andrew, to be more concrete: Do we have a bug we can dupe this on then?

No, we would need to file new bugs. And this would also entail figuring out what we want our policy to be. This may be better spun off in favor of a product-level decision/investigation into whether there are cases where we should be throttling workers or the impact of workers. There's real potential for cross-browser compat issues here.

That said, e10s and Fission help alleviate much of the original concern here. Specifically:

1. e10s means that workers aren't harming the general responsiveness as the browser as a whole because the parent process's main thread is not interfered with.
2. Fission means thats the performance implications of spammed postMessage on windows should only be meaningfully impact that origin's main thread.
   • That said, for BroadcastChannel (which this patch touched) and MessageChannel (related), those messages still do get routed through the PBackground thread so there is some potential concern there. But this patch did nothing to address that.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Restating:

• :bkelly's changes to BroadcastChannel and Window(Proxy).postMessages in the patch were to use ThrottledEventQueue or otherwise defer delivery of postMessage in order to avoid having the postMessage tasks dominating the main-thread run loop.
• This is a tricky scenario because doing something like this can potentially result in OOM without a capability to throttle the source of the postMessage calls. This stack did nothing to throttle the source; we only have such a mechanism in place for Workers using WorkerGlobalScope.postMessage, and it does accomplish that. This makes this a much harder problem.
• As I said in comment 11, in e10s and fission have largely mitigated concerns related to this.

From triage discussion:

• We are overhauling MessagePort in bug 1752287 which creates some new potential for backpressure, or at least in terms of allowing us to ensure that the OOM could be localized to the sender which is failing to utilize backpressure.
• We are investigating / working on worker throttling in bug 1818463 that could potentially integrate with backpressure signals, and this could potentially be hooked up to the changes above. But this would be done as new follow-ups.
• It would be desirable to have new real-world scenarios / profiling that shows that we are experiencing real problems here.