1783190 - Worker Cancellation doesn't wait for cleanup

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Yulia Startsev [:yulia]]

This was raised in bug 1675068 -- With the changes in bug 1742438, cancellation was turned into a flag, as opposed to iterating over all requests and cancelling them one by one.

The main issue is deleting the cache. We don't have a pointer to the cache creator because we are no longer tracking where we are in terms of processed requests on the main thread. There are four ways to solve this, I think:

• The original method: Keep a pointer on WorkerScriptLoader. If we keep a pointer to the cache creator on the WorkerScriptLoader, we will leak memory. This means we need some form of iteration, or a signal that all of the requests have been sent back to the worker to do clean up.
• Make CacheCreator initialize on the Worker side. The issue is we have some cross thread data used in the cache creator. This is likely doable, but will be more invasive.
• Track the ScriptLoaderRunnable, and store it there. The problem with this, is we would need the WorkerScriptLoader to be aware of the ScriptLoaderRunnable, and this doesn't work for modules.
• The final option is to iterate over the ScriptLoadRequests on the worker side, and dispatch a runnable that deletes the cache and clears the promises.

I will probably go with option 4 here, as it seems least invasive.

Comment 1

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

I doubt this impacts the current decision here, but I do want to reiterate that the Cache API is available on worker threads, so we could (eventually) use it there, but we would need to use a SimpleGlobalObject as the sandbox we create it in probably, since we don't have access to the main-thread-only sandboxy compartments from the worker thread and using the content global or the debugger global seems like it would really confuse things.

Comment 2

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

Also just for extra context, if we're not still holding things alive via a syncloop at that time, holding a StrongWorkerRef that doesn't have its last refcount dropped until we're ready for the worker to finish shutdown is the way to go. This can also be wrapped into a ThreadSafeWorkerRef to be held off the main thread if needed and it will handle proxying the release, etc. etc.

Comment 3

[Yulia Startsev [:yulia]]

Attachment: Bug 1783190 - Hold a ThreadSafeWorkerRef on WorkerScriptLoader; r=asuth

This addresses part of the issue, by holding a strong ref until we shutdown, so that we do not end
up in a situation where the worker closes before we finish cleanup.

Comment 4

[Yulia Startsev [:yulia]]

Attachment: Bug 1783190 - In the case of cancellation, cleanup cache; r=asuth

In this patch, we bring back a pointer to the CacheCreator on the WorkerScriptLoader. Previously, we
iterated over all of the load requests to cancel the cache promise. However, we can cancel all
outstanding requests by deleting the cache instead.

If the WeakPtr is still present when we cancel on the main thread, then we clear the cache and
cancel any outstanding promises related to loading.

Depends on D154381

Comment 5

[Yulia Startsev [:yulia]]

Attachment: Bug 1783190 - Replace WorkerPrivate usage with ThreadSafeWorkerRef usage; r=asuth

Cleanup, optional. It seems strange to have two ways to access the WorkerPrivate, and
ThreadSafeWorkerRef seems like the more reasonable choice.

Depends on D154382

Comment 6

[Yulia Startsev [:yulia]]

Attachment: Bug 1783190 - Replace WorkerPrivate usage on NetworkLoadHandler with ThreadSafeWorkerRef; r=asuth

Cleanup, optional. Repeating the same work on the NetWorkLoaderHandler as on the WorkerScriptLoader.

Previously, this was considered "safe" because the assumption is that the NetworkLoadHandler is
shorter lived than the WorkerScriptLoader. Rather than assuming this, if we end up in a situation
where this does out-live the WorkerScriptLoader, then we will end up leaking.

Depends on D154383

Comment 7

[Yulia Startsev [:yulia]]

Attachment: Bug 1783190 - Replace WorkerPrivate usage on CacheLoadHandler with ThreadSafeWorkerRef; r=asuth

Cleanup, Optional, Same as the NetworkLoadHandler.

Depends on D154384

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0850d8dd77ac
Hold a ThreadSafeWorkerRef on WorkerScriptLoader; r=asuth
https://hg.mozilla.org/integration/autoland/rev/7d1db2c5d45b
Replace WorkerPrivate usage with ThreadSafeWorkerRef usage; r=asuth
https://hg.mozilla.org/integration/autoland/rev/0be030b098ed
Replace WorkerPrivate usage on NetworkLoadHandler with ThreadSafeWorkerRef; r=asuth
https://hg.mozilla.org/integration/autoland/rev/ac34c5b93635
Replace WorkerPrivate usage on CacheLoadHandler with ThreadSafeWorkerRef; r=asuth

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0850d8dd77ac
https://hg.mozilla.org/mozilla-central/rev/7d1db2c5d45b
https://hg.mozilla.org/mozilla-central/rev/0be030b098ed
https://hg.mozilla.org/mozilla-central/rev/ac34c5b93635

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/593c1d517530
In the case of cancellation, cleanup cache; r=asuth

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b0be0b91aca
Backed out 6 changesets (bug 1783190, bug 1784482) for causing crashes at mozilla::dom::Promise::MaybeReject(nsresult)]. CLOSED TREE

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81341ff7a484
In the case of cancellation, cleanup cache; r=asuth

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/81341ff7a484

Comment 14

[Yulia Startsev [:yulia]]

Attachment: Bug 1783190 - Introduce lock for cancellation in case of race;r=jstutte

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6bb705b5ad9
Introduce lock for cancellation in case of race;r=jstutte

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/c6bb705b5ad9