1592488 - Don't return a MozPromise when using async IPDL methods, it leads to out of order processing of events

Status: RESOLVED FIXED

(Core :: IPC, task, P3)

Description

[Jean-Yves Avenard [:jya]]

SendXXX(...) can return a MozPromise. However, MozPromise::Then() always dispatch a task to run the callback.

This can lead to processing events out of order and intermittent issues due to races.

There's an additional binding that instead takes a resolve/reject callback instead to get around this issue.

I propose that we remove the MozPromise binding as it makes it easy to shoot yourself in the foot, and transition to the resolve/reject callback instead to make it easier to detect incorrect usage.

Additionally, I propose that we add an extra declaration similar to MozPromise::Then(..., [](MozPromise::ResolveOrRejectValue&& aValue) { ... }); which is useful when using move semantics or dealing with capturing non-refcounted objects

So you could do something like:

auto blah = MakeUnit<T1>(...);
SendBlah(..., [blah = std::move(blah)](Result<T2, nsresult>&& aValue) { ... });

Comment 1

[Jean-Yves Avenard [:jya]]

Another alternative would be, if the target thread is the same as the IPDL thread, to dispatch a task via a microtask or tail dispatching

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

There was some discussion of the microtask option in bug 1514328.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

The status of this bug is basically “patches welcome” — converting all of the existing promise-returning call sites to use callbacks is believed to be a nontrivial amount of work, none of the IPC peers have much time to spare, and there are things that are more urgently broken.

As far as anyone who's still here and working on IPC can recall, there doesn't seem to have been any deeply strategic reason to use promises vs. callbacks, other than that they seemed to be the popular way to express an eventually available value.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P1. Cleanup AbstractThread TLS lookup. r?gerald

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P13. Add TailDispatchingTarget class. r?bholley

This wraps a main thread such that a call to Dispatch will be done as Tail Dispatch instead so that the task will be run in the current event loop.

Depends on D69994

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P3. Fix race with MozPromise IPDL. r?mattwoodrow

Depends on D69995

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P2. Do not use AbstractThread too early. r?padenot

AbstractThread::MainThread() requires TailDispatcher to work which itself relies on the nsCycleCollectors to have been initialised.

This early in the XPCOM initilization, it doesn't work.

A bug was hiding the fact that the tail dispatcher wasn't available on the main thread by default.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P3. Make AbstractThread::GetCurrent() return MainThread by default. r?bholley

prior bug 1364821, AbstractThread::GetCurrent() would always return AbstractThread::MainThread() when called from the main thread.
After this change, we had to run within an AutoEnter scope.

A hidden side effect of this change was that under most cases AbstractThread::MainThread::Dispatch() would no longer use the tail dispatcher to dispatch a task.

It can be safely assume that whenever you're on the main thread, the equivalent AbstractThread is usable.
As such, we revert this change, making AbstractThread::GetCurrent() always returning the main thread unless overridden locally.

Depends on D71147

Comment 9

[Phabricator Automation]

Comment on attachment 9140954 [details]
Bug 1592488 - P3. Make AbstractThread::GetCurrent() return MainThread by default. r?bholley

Revision D71148 was moved to bug 1630802. Setting attachment 9140954 [details] to obsolete.

Comment 10

[Phabricator Automation]

Comment on attachment 9140953 [details]
Bug 1592488 - P2. Do not use AbstractThread too early. r?padenot

Revision D71147 was moved to bug 1630802. Setting attachment 9140953 [details] to obsolete.

Comment 11

[Phabricator Automation]

Comment on attachment 9138810 [details]
Bug 1592488 - P1. Cleanup AbstractThread TLS lookup. r?gerald

Revision D69994 was moved to bug 1630802. Setting attachment 9138810 [details] to obsolete.

Comment 12

[Jean-Yves Avenard [:jya]]

With bug 1631304, we'll be able to add an automatic dispatch via micro-task that wouldn't make us have to explicitly prevent the race from occurring.

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P1. Add MozPromise::Private::UseSynchronousTaskDispatch. r?bholley

If set, the callback will be run synchronously when the promise is resolved or rejected.

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P2. Add MozPromise::Private::UseDirectTaskDispatch. r?bholley

If set, the callback will be run synchronously when the promise is resolved or rejected.

Will dispatch a task via the microtask queue.
This mechanism is only available if both the caller and the target are on the main thread.

Depends on D71591

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P12. Run MozPromise's IPDL callbacks via direct tasks when the promise is resolved. r?nika

Fix intermittent issues due to races.

We now run the MozPromise generated by the IPDL bindings to run synchronously their callback.
This avoids a full trip to the back of the event queue for each additional asynchronous step when using MozPromise

The prevent unexpected racy behaviours when combining MozPromise with the other Resolve/Reject IPDL async declaration which was have lead to processing the events out of order.

Depends on D71592

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P4. Ensure an AbstractThread exists for the Compositor Thread. r?mattwoodrow

This is required for MozPromise's direct task dispatch to work.
It only needs to exist in the nsIThread.

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P3. Update AbstractThread comment to reflect current mode of operation. r?mattwoodrow

Depends on D71592

Comment 18

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P5. Ensure an AbstractThread exists with BackgroundThread. r?nika

This is necessary to ensure we can use IPC MozPromise that requires direct tasks dispatch.

Depends on D74165

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P6. Ensure an AbstractThread exists with RemoteWorkerService thread. r?nika

This is necessary to ensure we can use IPC MozPromise that requires direct tasks dispatch.

Depends on D74592

Comment 20

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P7. Ensure an AbstractThread exists with RemoteDecoder manager thread. r?mattwoodrow

This is necessary to ensure we can use IPC MozPromise that requires direct tasks dispatch.

Depends on D74593

Comment 21

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P8. Ensure AbstractThread exists with STS thread. r?valentin

This is necessary to ensure we can use IPC MozPromise with this target thread that requires direct tasks dispatch.

Depends on D74594

Comment 22

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P9. Make a DOMCacheThread an AbstractThread. r?baku

This is a step towards bug 1119864. However, at this stage we only need the cache thread to support direct task dispatch so that we can have IPDL MozPromise acting in a similar fashion to JS promise.

Comment 23

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P10. Add NonOwningXPCOMThreadWrapper. r?gerald

This will be useful to create a nsThread that provides an AbstractThread interface without creating a cycle.

Comment 24

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P11. Ensure an AbstractThread exists for each WorkerThread. r?baku

As with P9; in order to be able to use MozPromise and direct task dispatching we need an AbstractThread to exist.

Depends on D74674

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74ff142216da
P1. Add MozPromise::Private::UseSynchronousTaskDispatch. r=bholley
https://hg.mozilla.org/integration/autoland/rev/09f0c7ef9cd3
P2. Add MozPromise::Private::UseDirectTaskDispatch. r=bholley
https://hg.mozilla.org/integration/autoland/rev/49ca05bab7e0
P3. Update AbstractThread comment to reflect current mode of operation. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/b703165595f6
P4. Ensure an AbstractThread exists for the Compositor Thread. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/0220b0ec625b
P5. Ensure an AbstractThread exists with BackgroundThread. r=nika
https://hg.mozilla.org/integration/autoland/rev/828152dfaed5
P6. Ensure an AbstractThread exists with RemoteWorkerService thread. r=nika
https://hg.mozilla.org/integration/autoland/rev/ad41d4a3dd8d
P7. Ensure an AbstractThread exists with RemoteDecoder manager thread. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2ad003a3a954
P8. Ensure AbstractThread exists with STS thread. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/11deef3eafe8
P9. Make a DOMCacheThread an AbstractThread. r=baku
https://hg.mozilla.org/integration/autoland/rev/773d2ea52765
P11. Ensure an AbstractThread exists for each WorkerThread. r=baku
https://hg.mozilla.org/integration/autoland/rev/78b86c92bedc
P12. Run MozPromise's IPDL callbacks via direct tasks when the promise is resolved. r=nika
https://hg.mozilla.org/integration/autoland/rev/4b0f7ee4f17d
P13. Add TailDispatchingTarget class. r=bholley

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P11.1 Add missing header. r?mattwoodrow

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63abdefd8f5b
P11.1 Add missing header. r=mattwoodrow

Comment 28

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/74ff142216da
https://hg.mozilla.org/mozilla-central/rev/09f0c7ef9cd3
https://hg.mozilla.org/mozilla-central/rev/49ca05bab7e0
https://hg.mozilla.org/mozilla-central/rev/b703165595f6
https://hg.mozilla.org/mozilla-central/rev/0220b0ec625b
https://hg.mozilla.org/mozilla-central/rev/828152dfaed5
https://hg.mozilla.org/mozilla-central/rev/ad41d4a3dd8d
https://hg.mozilla.org/mozilla-central/rev/2ad003a3a954
https://hg.mozilla.org/mozilla-central/rev/11deef3eafe8
https://hg.mozilla.org/mozilla-central/rev/773d2ea52765
https://hg.mozilla.org/mozilla-central/rev/78b86c92bedc
https://hg.mozilla.org/mozilla-central/rev/4b0f7ee4f17d
https://hg.mozilla.org/mozilla-central/rev/63abdefd8f5b

Comment 29

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1592488 - P9.1 Ensure thread gets shutdown when DOMCache manager gets destroyed. r=baku

This is required to break the cycle between the nsThread and the nsThreadManager and prevent leaks.

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 9158182 [details]
Bug 1592488 - P9.1 Ensure thread gets shutdown when DOMCache manager gets destroyed. r=baku

Beta/Release Uplift Approval Request

• User impact if declined: hundreds of unused threads leaking
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We revert to the original code; only creating an associated AbstractThread to allow for direct task dispatching to work.
• String changes made/needed: none

Comment 31

[Phabricator Automation]

Comment on attachment 9158182 [details]
Bug 1592488 - P9.1 Ensure thread gets shutdown when DOMCache manager gets destroyed. r=baku

Revision D80457 was moved to bug 1643690. Setting attachment 9158182 [details] to obsolete.