1768930 - RemoteWorkerService crashes if it is shut down too quickly

Status: RESOLVED FIXED

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

Description

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

(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

[Jens Stutte [:jstutte]]

Hi Nick, are you going to upload that/a patch to phabricator here? It looks definitely reasonable to me.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1768930 - Shutdown Remote Worker Service thread only after initialization. r?jstutte

This avoids a crash when the main process exits very quickly, as is
usual in --backgroundtask ... mode.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aac927e5a353
Shutdown Remote Worker Service thread only after initialization. r=jstutte

Comment 5

[Iulian Moraru]

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Landed so clearing NI. Thanks for your help, :nika and :jstutte!