RemoteWorkerService crashes if it is shut down too quickly
Categories
(Core :: DOM: Workers, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox102 | --- | fixed |
People
(Reporter: nika, Assigned: nalexander)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
This was noticed by :nalexander (https://matrix.to/#/!ykdkAGURCpjeYhwLFB:mozilla.org/$7IsoT4jUdN7iNMLbnELd06M70g3AiaC1J1yy03UX1n0?via=mozilla.org&via=matrix.org&via=mattn.ca) in a try push which creates a large number of firefox --backgroundtask
instances (https://treeherder.mozilla.org/logviewer?job_id=377708202&repo=try&lineNumber=10531). Apparently these instances of Firefox will start, run a small amount of JS, and then shut down almost immediately.
The issue here appears to be that we don't wait for the background thread to actually finish initializing before proceeding to the next shutdown phase, which can lead to issues. When the service is initialized, it starts a background thread and dispatches a startup task to it which creates the actor managed by PBackground
(https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/dom/workers/remoteworkers/RemoteWorkerService.cpp#97-101, https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/dom/workers/remoteworkers/RemoteWorkerService.cpp#117). If this occurs after the xpcom-shutdown-threads
phase, this will assert as PBackground will already be shut down, meaning new actors cannot be started (https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/ipc/glue/BackgroundImpl.cpp#458).
The service is set up to observe xpcom-shutdown
, which it will dispatch an event to the background thread from to ask it to shut down (https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/dom/workers/remoteworkers/RemoteWorkerService.cpp#165-169). It however does not wait for the shutdown task to be run, so there is no guarantee that the background thread ever finished initializing before it returns, potentially leading to this bug. The only thing which synchronizes with the thread shutting down after this point to ensure it's dead before the process exits is the forced-shutdown of the thread in nsThreadManager::ShutdownNonMainThreads()
.
An easy fix here might be to remove the second dispatch from the target thread back to the main thread, and to instead call mThread->Shutdown();
immediately after dispatching the event to it from the main thread, which will spin a nested event loop waiting for the thread to exit after processing all posted events, and will also wait for it to finish initializing as a side-effect.
Assignee | ||
Comment 1•2 years ago
|
||
(In reply to Nika Layzell [:nika] (ni? for response) from comment #0)
An easy fix here might be to remove the second dispatch from the target thread back to the main thread, and to instead call
mThread->Shutdown();
immediately after dispatching the event to it from the main thread, which will spin a nested event loop waiting for the thread to exit after processing all posted events, and will also wait for it to finish initializing as a side-effect.
FWIW, I hacked the easy fix in and my previously unhappy try builds are no longer crashing. See all the green TV jobs here and the trivial patch.
Thanks so much for this easy-to-test suggestion, Nika!
Comment 2•2 years ago
|
||
Hi Nick, are you going to upload that/a patch to phabricator here? It looks definitely reasonable to me.
Assignee | ||
Comment 3•2 years ago
|
||
This avoids a crash when the main process exits very quickly, as is
usual in --backgroundtask ...
mode.
Updated•2 years ago
|
Comment 5•2 years ago
|
||
bugherder |
Assignee | ||
Comment 6•2 years ago
|
||
Landed so clearing NI. Thanks for your help, :nika and :jstutte!
Description
•