Closed Bug 1783190 Opened 2 years ago Closed 1 year ago

Worker Cancellation doesn't wait for cleanup

Categories

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

Firefox 105
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox105 --- affected

People

(Reporter: yulia, Assigned: yulia)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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.

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.

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.

Severity: -- → S3

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.

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

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

Depends on D154382

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

Cleanup, Optional, Same as the NetworkLoadHandler.

Depends on D154384

Blocks: 1784457
Attachment #9289440 - Attachment description: Bug 1783190 - add WeakPtr to CacheCreator for cleanup; r=asuth → Bug 1783190 - Track CacheCreator for cleanup; r=asuth
Keywords: leave-open
Pushed by ystartsev@mozilla.com:
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
Attachment #9289440 - Attachment description: Bug 1783190 - Track CacheCreator for cleanup; r=asuth → Bug 1783190 - In the case of cancellation, cleanup cache; r=asuth
Depends on: 1784482
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/593c1d517530
In the case of cancellation, cleanup cache; r=asuth
Backout by abutkovits@mozilla.com:
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
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81341ff7a484
In the case of cancellation, cleanup cache; r=asuth
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c6bb705b5ad9
Introduce lock for cancellation in case of race;r=jstutte
Status: NEW → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.