1631304 - Run the TailDispatcher off an nsIThreadObserver

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Bobby Holley (:bholley)]

I'm going to provide a lot of context here, because I think this machinery is not well-understood by people other than me. :-)

In the old days, anything that needed to use MozPromise needed to use AbstractThread, so AbstractThread began to propagate throughout the tree. We then switched MozPromise over to nsISerialEventTarget in bug 1366072, which means that much of the AbstractThread usage still in the tree is vestigial. However, there is one piece of special functionality you still need AbstractThread for, which is TailDispatch [1].

The basic idea of tail dispatch is to delay the dispatching of all tasks from thread A to thread B until the end of the current task. The reason for this is the StateMirroring machinery [2]. The idea of state mirroring is to accumulate state updates in thread A and send them to thread B atomically once all the mutations have finished. This abstraction only holds if thread A doesn't share memory with thread B, and also doesn't have any asynchronous communication with thread B while state updates are accumulating. That is to say, if thread A is queuing up state updates to send to thread B later, but is intermittently firing runnables at thread B, the whole thing falls apart. Thread B might see a runnable that assumes the new state, but not have received the new state yet.

So the idea of tail dispatch is that it's global relationship between two threads - at least for the code using AbstractThread, which is why it's important to use AbstractThread consistently within a module. If thread A and thread B both support tail dispatch, then all runnables between them are queued until the end of the task, and dispatched atomically (with state updates going out first, and regular tasks going out second). So far this is all just queuing tasks, no arbitrary code execution.

However, in addition to StateMirroring, there is also a companion mechanism called StateWatching [3]. StateWatching is basically a quick-and-dirty version of reactive programming in Gecko, wherein you can tweak a bunch of values, and then run some dependent code like (i.e. StateDidChange()) once the dust has settled. StateWatching also piggy-backs off the TailDispatcher, but needs to run code immediately. So it has a mechanism called "Direct Tasks", which do that [4].

On the main thread, the TailDispatcher registers itself as a stable-state callback, to run at the very end of the current task in the event loop. This is conceptually what we want, but suboptimal for a few reasons:

• The concept of "stable state" is vestigial from the spec's perspective, and has been merged with the microtask infrastructure. TailDispatcher's current usage of it is very weird and not well-understood.
• Triggering script from within a stable-state callback is disallowed. This is sometimes happens accidentally, and triggers crashes (see bug 1443429).

Strictly speaking, the direct tasks don't need to be coupled with the rest of the tail dispatch machinery. However, the AbstractThread abstraction allows StateWatching and StateMirroring to be used transparently across TaskQueues and XPCOM threads, which is a nice thing to preserve.

Given all the considerations, I think the right thing to do here is to run the tail dispatcher off the microtask queue. This will make it less weird, and allow Direct Tasks to trigger script (since microtasks are totally allowed to run script). The primary disadvantage is that the system might be less "settled" when the tail dispatcher fires, because MicroTasks run at the end of topmost script execution. So in this example:

void SomeRunnable() {
MutateSomeMirroredState();
TriggerSomeJS();
MutateSomeMoreMirroredState();
}

Today's Gecko would deliver the state updates as one atomic unit, whereas the MicroTask version would deliver two state updates. That could cause problems if the intermediate state is not coherent, but I think that's unlikely in practice. After all, if you're about to run JS, the modified state is probably going to be observable, so it's necessary anyway to make it coherent.

I prototyped this, and it appears to be green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf56740ae55cdbae7f89ee56000708677b05d59c

[1] https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/xpcom/threads/AbstractThread.h#79
[2] https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/xpcom/threads/StateMirroring.h
[3] https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/xpcom/threads/StateWatching.h
[4] https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/xpcom/threads/TaskDispatcher.h#44
[5] https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/xpcom/threads/AbstractThread.cpp#80

Comment 2

[Bobby Holley (:bholley)]

Attachment: Bug 1631304 - Drill a fast path to accessing the main thread.

I'm surprised we don't have this already.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Bug 1631304 - Add a mechanism to dispatch runnables as microtasks.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Bug 1631304 - Run the TailDispatcher off the Microtask queue.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/700adf5d3779
Drill a fast path to accessing the main thread. r=erahm
https://hg.mozilla.org/integration/autoland/rev/9ffa1843a04b
Add a mechanism to dispatch runnables as microtasks. r=smaug
https://hg.mozilla.org/integration/autoland/rev/54e27b897b3a
Run the TailDispatcher off the Microtask queue. r=jya

Comment 6

[Andrei Ciure[:aciure]]

Backed out 3 changesets (bug 1631304) for causing MediaStream-MediaElement-srcObject.https.html wpt failures
https://hg.mozilla.org/integration/autoland/rev/96dbee93d251770c63224e6b34a81d795853947a

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=54e27b897b3abd422eda4e4520ea7d29e5c22e90&searchStr=wpt4

log: https://treeherder.mozilla.org/logviewer.html#?job_id=298541802&repo=autoland

Comment 7

[Bobby Holley (:bholley)]

Hm, that's a pretty nice and easy to read test, and is very clearly testing for precisely the behavior that we're changing [1]. Kudos to Andreas.

So the test is verifying that changes to HTMLMediaElement.ended only take effect when the event loop reaches step 1 [2]. This is a new spec phrasing since last I checked, but seems to pretty clearly codify the old TailDispatcher behavior (which runs off microtasks), at least for anything depending on StateWatching.

I think I can still improve the situation a bit by making the whole thing explicit though. I have the vestiges of a patch.

[1] https://searchfox.org/mozilla-central/rev/d2cec90777d573585f8477d5170892e5dcdfb0ab/testing/web-platform/tests/mediacapture-streams/MediaStream-MediaElement-srcObject.https.html#380
[2] https://html.spec.whatwg.org/#dom-media-ended

Comment 8

[Bobby Holley (:bholley)]

Attachment: Bug 1631304 - Replace EventTargetWrapper with XPCOMThreadWrapper.

This is how it used to be, before the Quantum DOM stuff. We use
nsIThreadInternal because that supports thread observers, which we
leverage in the next patch.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Bug 1631304 - Use thread observers for the tail dispatcher.

Comment 10

[Bobby Holley (:bholley)]

Updating the title to reflect the new approach.

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Bobby Holley (:bholley)]

Attachment: Bug 1631304 - Don't lazily create a TailDispatcher from MaybeDrainDirectTasks.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Bug 1631304 - Reject AbstractThread dispatches after the main thread has ceased event proccessing.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Bug 1631304 - Mark the tail dispatcher as unavailable outside of event dispatch.

This is in preparation for running the tail dispatcher off
nsIThreadObserver callbacks, which only work during regular
event processing.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63aa05c6c9b0
Drill a fast path to accessing the main thread. r=erahm
https://hg.mozilla.org/integration/autoland/rev/7d0595a40d4e
Don't lazily create a TailDispatcher from MaybeDrainDirectTasks. r=jya
https://hg.mozilla.org/integration/autoland/rev/a33a94d66233
Replace EventTargetWrapper with XPCOMThreadWrapper. r=jya
https://hg.mozilla.org/integration/autoland/rev/bad4d84f41ca
Reject AbstractThread dispatches after the main thread has ceased event proccessing. r=erahm
https://hg.mozilla.org/integration/autoland/rev/05287026736e
Mark the tail dispatcher as unavailable outside of event dispatch. r=jya
https://hg.mozilla.org/integration/autoland/rev/658d78ebfd03
Use thread observers for the tail dispatcher. r=jya

Comment 17

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/63aa05c6c9b0
https://hg.mozilla.org/mozilla-central/rev/7d0595a40d4e
https://hg.mozilla.org/mozilla-central/rev/a33a94d66233
https://hg.mozilla.org/mozilla-central/rev/bad4d84f41ca
https://hg.mozilla.org/mozilla-central/rev/05287026736e
https://hg.mozilla.org/mozilla-central/rev/658d78ebfd03