1345251 - MozPromise is not usable on worker threads

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

MozPromise needs to dispatch runnables at a thread target for Then(), etc.  Currently this cannot be done on Worker threads because the Worker thread target requires nsICancelableRunnable to be implemented.

Also, we should probably assert in the MozPromise runnables that the WorkerPrivate has a holder keeping the thread alive.  Otherwise MozPromise is pretty unsafe there.

I'd like to use MozPromise in the Clients API, so I need this for bug 1293277.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Please be careful with that:

Some asserts for "reliable dispatch" were recently removed (bug 1343337) because we got rid of FlushableTaskQueue's in media code (bug 1270039). These asserts were there because (I believe) MozPromise expects all dispatched tasks to always run to completion, unless the users explicitly Disconnect()'s them through the MozPromise interface.

So re-introducing disappearing tasks may be dangerous!

JW, you know much more about that aspect of MozPromise and task queues, what do you think?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

My plan is to make the Cancel() method on MozPromise runnables just call Run().  That is what we do for things like IPC runnables which must be executed.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

AbstractThread does its own wrapper runnable, though.  I need to make that cancelable as well.

Once again I am really frustrated we decided to make a second parallel thread ecosystem separate from nsIThread.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make MozPromise usable on worker threads. r=gerald

This patch solves the problems with a few changes:

1) Make all MozPromise nsICancelableRunnable objects.  They are implemented to always execute their Run(), so current invariants should not change.
2) Make AbstractThread's Runner implement nsICancelableRunnable interface.  It will attempt to proxy the Cancel() to its inner runnable.
3) Make the XPCOM wrapper avoid dispatching a non-cancelable runnable to set its TLS if its already on the target thread.
4) Make worker's RuntimeService create an AbstractThread at worker startup so AbstractThread::GetCurrent() functions.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8844655 [details] [diff] [review]
Make MozPromise usable on worker threads. r=gerald

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

Sorry.  Included some different files there.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make MozPromise usable on worker threads. r=gerald

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

And please request the review from :jwwang, he's the better expert for MozPromise & threads.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make MozPromise usable on worker threads. r=gerald

Will I get the right patch this time?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8844658 [details] [diff] [review]
Make MozPromise usable on worker threads. r=gerald

Please see comment 4 for the patch description.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #2)
> My plan is to make the Cancel() method on MozPromise runnables just call
> Run().  That is what we do for things like IPC runnables which must be
> executed.

Can you give some code examples? It is hard to me to think Cancel() means "To run" in the IPC context. Also I think Cancel() should mean "Not to run" in most cases.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #10)
> Can you give some code examples? It is hard to me to think Cancel() means
> "To run" in the IPC context. Also I think Cancel() should mean "Not to run"
> in most cases.

In most cases it does.  But my understanding is MozPromise is designed around the idea that once Dispatch() returns success then the runnable will always run.  The way this achieved on worker threads is to call Run() in Cancel().

Existing example:

https://dxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundImpl.cpp#1791

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

To clarify, I'm happy to ignore MozPromise runnables if they are canceled, but comment 1 suggested that was not advisable.  I don't have time to add some new cancelation facility to MozPromise, though.  I just need to be able to use MozPromise on worker threads safely and this seems the best approach at the moment.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Also, I'm open to other suggestions or patches.  This is blocking bug 1293277 which is in turn blocking multi-e10s which is a high priority at the moment.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #11)
> In most cases it does.  But my understanding is MozPromise is designed
> around the idea that once Dispatch() returns success then the runnable will
> always run.

There are some noticeable details in the statement "once Dispatch() returns success then the runnable will always run".

http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/xpcom/threads/MozPromise.h#422
The resolve/reject function will not be invoked when the ThenValue is disconnected even if Dispatch() has succeeded and the runnable is running on the target thread which in a sense means "Not to run" to a MozPromise client.

However, I think it should be fine to call Run() in Cancel() because we require the client to disconnect the ThenValue explicitly if it doesn't want resolve/reject function to be run.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8844658 [details] [diff] [review]
Make MozPromise usable on worker threads. r=gerald

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

::: xpcom/threads/AbstractThread.cpp
@@ +172,5 @@
>  
> +    nsresult Cancel() override
> +    {
> +      // Set the TLS during Cancel() just in case it calls Run().
> +      class MOZ_STACK_CLASS AutoTaskGuard final {

We have the same one in Run(). Please extract the common code.

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you for helping, JW.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1345251_workermozpromise.patch

Thanks for the review!

Update patch to share the AutoTaskGuard code between the two uses.

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f760f5997f1
Make MozPromise usable on worker threads. r=gerald

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6f760f5997f1