1800659 - WorkerRunnable/WorkerDebuggeeRunnable refactoring (eliminate cancellation and therefore ClearMainEventQueue)

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P2)

Description

[Eden Chuang[:edenchuang]]

This is a bug for tracking our WorkRunnable refactoring.
According to https://mozilla-hub.atlassian.net/browse/FFXP-1949, the followings are the targets for this bug.

• Eliminate cancellation from WorkerRunnables

• Eliminate busy count in favor of just letting workerrefs and the existence of IPC actors keep a worker alive. Simplifies worker runnable too.

• Distinguish WorkerRunnables meant to run on the worker thread from those meant to run on the main/parent thread by having a different base class (like WorkerParentRunnable), eliminating the need to have a (still) living worker when executed

https://bugzilla.mozilla.org/show_bug.cgi?id=1799173 shows the defects of our current implementation. And Worker's lifecycle would be much simpler after this refactoring.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Bug 1800659 - P2 remove nsICancelableRunnable inheriting form WorkerRunnable. r=asuth

WorkerRunnable needs not to inherit nsICancelRunnable.

Without Cancel(),WorkerRunnable::Run() would be executed even the WorkerPrivate is getting to closing/cancling/killing to release its corresponding resource.
However, WorkerRunnable::WorkerRun() is not assured can be executed as normal during the WorkerPrivate's shutting down, WorkerRunnable::Discard() is introduced to perform the specific resource releasing work.

Comment 2

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

As part of the changes here, we should also make sure that dispatch guards are relaxed. Some of this has already been explicitly mentioned in related docs, but just for clarity:

• As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1777921#c10 the guard about not dispatching to workers beyond Running should be relaxed to beyond Canceling. It should always be okay to dispatch a runnable to a Worker in Canceling.
• We should remove all guards about what threads are allowed to dispatch to a worker.
• We should remove all concept of wrapping runnables that aren't worker runnables (because it will no longer be necessary).

It's quite possible that we can potentially expedite work on this bug by taking the above steps and removing all calls to ClearMainEventQueue as a first step. Like, it's potentially okay for the cancel methods to be dead code for non-control-runnables, with us always running the runnables.

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1800659 - P1 Adding log for Worker. r=#dom-worker-reviewers

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: WIP: Bug 1800659 - P3 Remove WorkerPrivate::ClearMainEventQueue. r=#dom-worker-reviewers

Depends on D173850

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1800659 - P3 Remove WorkerPrivate::ClearMainEventQueue() r=asuth

Some behavior for WorkerStatus is changed since removing the ClearMainEventQueue.
When entering “Closing”

1. Keeping receives normal WorkerRunnables
2. Cancel all timeout
3. Don’t clear the main event queue anymore. But we still wait for all SyncLoops be completed.
4. Switching to “Canceling” by asking parent thread to call WorkerPrivate::Cancel()

When entering “Canceling”

1. Notify WorkerScope, it should start dying and stop the WebTaskScheduler
2. Notify StrongWorkerRefs, worker is in “Canceling”, send and complete the corresponding shutdown work right now.
3. Executing all runnables until no normal WorkerRunnables in queue and no WorkerRefs, SyncLoops and children workers
   WorkerRefs could be created in this status, but it would be notified immediatly to start shutdown behavior.
4. Stop receiving normal WorkerRunnables and DisconnectEventTargetObjects of WorkerScope.
5. Entering “Killing”

When entering “Killing”

1. We would not notify WorkerRefs anymore. Logically all WorkerRefs should be released in “Canceling”
2. Executing all remaining ControlRunnables
3. Release corresponding resources of Worker on worker thread.

Depends on D173850

Comment 6

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

Marking as leave-open since this bug currently also is defined to include some other enhancements, in particular removing busy count. That said, it might make sense to create spin-off bugs for those and close this once those are filed.

Comment 7

[Jens Stutte [:jstutte]]

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

Marking as leave-open since this bug currently also is defined to include some other enhancements, in particular removing busy count. That said, it might make sense to create spin-off bugs for those and close this once those are filed.

Yes, please create spin-off bugs, thanks.

Comment 8

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

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

Yes, please create spin-off bugs, thanks.

(In reply to Eden Chuang[:edenchuang] from comment #0)

• Eliminate busy count in favor of just letting workerrefs and the existence of IPC actors keep a worker alive. Simplifies worker runnable too.

I have filed bug 1836700 to spin this off to a follow-up.

• Distinguish WorkerRunnables meant to run on the worker thread from those meant to run on the main/parent thread by having a different base class (like WorkerParentRunnable), eliminating the need to have a (still) living worker when executed

I believe bug 1769913 makes sense to track this after bug 1836700 is addressed, since the elimination of the busy count can potentially eliminate the need to have worker-to-main-thread runnables be a special class at all. (As long as there is no syncloop involved.) I'm going to update that bug.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/910dabb8545d
P1 Adding log for Worker. r=asuth
https://hg.mozilla.org/integration/autoland/rev/32515f9b41c7
P2 remove nsICancelableRunnable inheriting form WorkerRunnable. r=asuth
https://hg.mozilla.org/integration/autoland/rev/b1ab6c750d04
P3 Remove WorkerPrivate::ClearMainEventQueue() r=asuth,smaug

Comment 10

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/910dabb8545d
https://hg.mozilla.org/mozilla-central/rev/32515f9b41c7
https://hg.mozilla.org/mozilla-central/rev/b1ab6c750d04