1627926 - SharedWorkers should be forced to shutdown at browser shutdown. Crash in [@ mozilla::dom::RemoteWorkerService::Thread]

Status: RESOLVED WORKSFORME

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

Description

[Wayne Mery (:wsmwk)]

All Thunderbird crashes. Earliest is 68.1.0. No beta or nightly crashes.

This bug is for crash report bp-50254efe-713e-4371-8968-13f7f0200407.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::RemoteWorkerService::Thread dom/workers/remoteworkers/RemoteWorkerService.cpp:76
1 xul.dll mozilla::dom::RemoteWorkerChild::ShutdownOnWorker dom/workers/remoteworkers/RemoteWorkerChild.cpp:393
2 xul.dll mozilla::dom::WeakWorkerRef::Notify dom/workers/WorkerRef.cpp:128
3 xul.dll mozilla::dom::WorkerRef::Holder::Notify dom/workers/WorkerRef.cpp:65
4 xul.dll mozilla::dom::WorkerPrivate::NotifyHolders dom/workers/WorkerPrivate.cpp:3507
5 xul.dll mozilla::dom::WorkerPrivate::NotifyInternal dom/workers/WorkerPrivate.cpp:4016
6 xul.dll bool mozilla::dom::`anonymous namespace'::NotifyRunnable::WorkerRun dom/workers/WorkerPrivate.cpp:428
7 xul.dll mozilla::dom::WorkerRunnable::Run dom/workers/WorkerRunnable.cpp:363
8 xul.dll mozilla::dom::WorkerPrivate::ProcessAllControlRunnables dom/workers/WorkerPrivate.h:908
9 xul.dll mozilla::dom::WorkerPrivate::InterruptCallback dom/workers/WorkerPrivate.cpp:3111

Comment 1

[Magnus Melin [:mkmelin]]

https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/dom/workers/remoteworkers/RemoteWorkerService.cpp#76 sRemoteWorkerService would be null. But no idea why.

Comment 2

[Magnus Melin [:mkmelin]]

Oh, it can be null during shutdown: https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/dom/workers/remoteworkers/RemoteWorkerService.cpp#172

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: Bug 1627926 - Avoid Crash in [@ mozilla::dom::RemoteWorkerService::Thread]. r=nika

sRemoteWorkerService will be null during shutdown.

Comment 4

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

I commented on the review at https://phabricator.services.mozilla.com/D72189#2196279 but the meta-issue here is that the SharedWorker shouldn't be alive at the time the runtime service is shutting down the workers. There's some extenuating Thunderbird stuff going on here, though, namely it's a SharedWorker created by a legacy extension in the parent process which means the normal mechanism by which SharedWorkers are torn down don't come into play. Those factors are:

• The SharedWorker itself would live in a content process and the content processes would already be terminated.
• The SharedWorker bindings that keep the worker wanting to be running would be gone because of:
  • The content global would have been torn down.
  • The content process would have been terminated.

All that said, it seems like the SharedWorker logic in the parent is perhaps depending too much on these clean shutdowns above and it in fact needs to be adding a ShutdownBlocker like the ServiceWorkerManager does, and it similarly needs to be forcing all SharedWorkers to be torn down at the same time the ServiceWorkerManager is forcing them to be torn down. Also, it might want to complain loudly about the existence of any content global sharedworkers it encounters. (Also maybe we want to forbid non-content-global sharedworkers? They're somewhat nonsensical for system principaled code which I think we actually want to be using ChromeWorkers as we improve their shutdown.)

Comment 5

[Magnus Melin [:mkmelin]]

Thanks for the analysis. I don't know this code enough to do those changes, so I think I'll leave this for now. I'll contact signal-spam and refer them to this comment though.

Comment 6

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.