Open Bug 1627926 Opened 8 months ago Updated 7 months ago

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

Categories

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

x86
Windows
defect

Tracking

()

People

(Reporter: wsmwk, Unassigned)

Details

(Keywords: crash, Whiteboard: [tbird crash])

Crash Data

Attachments

(1 obsolete file)

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

Flags: needinfo?(mkmelin+mozilla)
Assignee: nobody → mkmelin+mozilla
Component: General → DOM: Workers
Product: Thunderbird → Core

sRemoteWorkerService will be null during shutdown.

Whiteboard: [tbird crash]
Status: NEW → ASSIGNED

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.)

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.

Assignee: mkmelin+mozilla → nobody
Status: ASSIGNED → NEW
Attachment #9142790 - Attachment is obsolete: true
Priority: -- → P3
Summary: Crash in [@ mozilla::dom::RemoteWorkerService::Thread] → SharedWorkers should be forced to shutdown at browser shutdown. Crash in [@ mozilla::dom::RemoteWorkerService::Thread]

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.)

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.