1849269 - Null deref crash in [@ mozilla::dom::RemoteWorkerController::PendingSharedWorkerOp::MaybeStart]

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/73fce024-e007-4b10-9be2-93a260230817

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  mozilla::dom::RemoteWorkerController::PendingSharedWorkerOp::MaybeStart  dom/workers/remoteworkers/RemoteWorkerController.cpp:360
1  xul.dll  mozilla::dom::RemoteWorkerController::MaybeStartSharedWorkerOp<mozilla::dom::RemoteWorkerController::PendingSharedWorkerOp::Type>  dom/workers/remoteworkers/RemoteWorkerController.cpp:224
2  xul.dll  mozilla::dom::RemoteWorkerController::Terminate  dom/workers/remoteworkers/RemoteWorkerController.cpp:253
3  xul.dll  mozilla::dom::SharedWorkerManager::Terminate  dom/workers/sharedworkers/SharedWorkerManager.cpp:162
3  xul.dll  mozilla::dom::SharedWorkerManager::UnregisterHolder::<lambda_1>::operator const  dom/workers/sharedworkers/SharedWorkerManager.cpp:312
3  xul.dll  mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/dom/workers/sharedworkers/SharedWorkerManager.cpp:312:11'>::Run  xpcom/threads/nsThreadUtils.h:548
4  xul.dll  nsThread::ProcessNextEvent  xpcom/threads/nsThread.cpp:1193
4  xul.dll  NS_ProcessNextEvent  xpcom/threads/nsThreadUtils.cpp:480
5  xul.dll  mozilla::ipc::MessagePumpForNonMainThreads::Run  ipc/glue/MessagePump.cpp:300
6  xul.dll  MessageLoop::RunInternal  ipc/chromium/src/base/message_loop.cc:370

This looks like a null deref. Is it this that is null? That doesn't make much sense.

Comment 1

[Jens Stutte [:jstutte]]

Looks as if mRemoteWorkerController is nullptr ?

We clear it only on Terminate, AFAICS. There is the theoretical possibility to end without if we fail to allocate it but that allocation should be infallible and thus crash on OOM, IIUC.

The lambda dispatched as runnable captures a RefPtr to the containing SharedWorkerManager, keeping it alive, but we could end up with mRemoteWorkerController == nullptr at the second dispatch of this runnable, if that was possible. We could add a diagnostic assert before dispatching to see, if that happens and who is causing this.

Comment 2

[Jens Stutte [:jstutte]]

Hi Eden, would you mind to take a look if I miss something obvious?

Comment 3

[Eden Chuang[:edenchuang]]

Here comes a possible might cause the crash stack.

1. RemoteWorkerController creation fails. If it is Shutdown phase or OOM. The point is ServiceWorkerManager::mRemoteWorkerController is kept as nullptr.

2. WorkerManagerCreatedRunnable::mManagerWrapper is the only holder of the ServiceWorkerManager. And it would be released once the runnable finished.

3. When releasing WorkerManagerCreatedRunnable::mManagerWrapper, ServiceWorkerManager::UnregisterHolder() is called. In the end SharedWorkerManager::Terminate() is triggered to release the SharedWorker by calling SharedWorkerManager::mRemoteWorkerController::Terminate. However, SharedWorkerManager::mRemoteWorkerController is reasonably a nullptr here, then we meet a de-reference on nullptr.

I will suggest checking whether SharedWorkerManager::mRemoteWorkerController is null in ServiceWorkerManager::Terminate() since it is reasonable to be a nullptr.

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Bug 1849269 - Add SharedWorkerManager::mRemoteWorkerController nullptr checking in SharedWorkerManager::Terminate(). r=#dom-worker-reviewers

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3bb72fd36fc
Add SharedWorkerManager::mRemoteWorkerController nullptr checking in SharedWorkerManager::Terminate(). r=dom-worker-reviewers,smaug,asuth

Comment 6

[Narcis Beleuzu [:NarcisB]]

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

Comment 7

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

The patch landed in nightly and beta is affected.
:edenchuang, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Comment 8

[Eden Chuang[:edenchuang]]

Comment on attachment 9351198 [details]
Bug 1849269 - Add SharedWorkerManager::mRemoteWorkerController nullptr checking in SharedWorkerManager::Terminate(). r=#dom-worker-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Users would meet Firefox crash once creating SharedWorker during shutdown.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch adds a nullptr checking when terminating the SharedWorker. SharedWorkerManager::mRemoteWorkerController could be a nullptr reasonably if creating the SharedWorker when Firefox shutdown.
• String changes made/needed: No
• Is Android affected?: Yes

Comment 9

[Eden Chuang[:edenchuang]]

Comment on attachment 9351198 [details]
Bug 1849269 - Add SharedWorkerManager::mRemoteWorkerController nullptr checking in SharedWorkerManager::Terminate(). r=#dom-worker-reviewers

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: If creating a SharedWorker when Firefox is shutting down, SharedWorkerManager::mRemoteWorkerController could be a nullptr reasonably.
  Because Firefox is shutting down, SharedWorkerManager::Terminate would try to access SharedWorkerManager::mRemoteWorkerController to shut down the SharedWorker. Then, Nullptr de-reference happens for the case. Since nullptr is a reasonable value, what the patch did is adding a nullptr check in SharedWorkerManager::Terminate, and stop the termination once SharedWorkerManager::mRemoteWorkerController is a nullptr.
• User impact if declined: Users would meet Firefox crash once creating SharedWorker during shutdown.
• Fix Landed on Version: 119
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch adds a nullptr checking when terminating the SharedWorker. SharedWorkerManager::mRemoteWorkerController could be a nullptr reasonably if creating the SharedWorker when Firefox shutdown.

Comment 10

[Pascal Chevrel:pascalc]

Comment on attachment 9351198 [details]
Bug 1849269 - Add SharedWorkerManager::mRemoteWorkerController nullptr checking in SharedWorkerManager::Terminate(). r=#dom-worker-reviewers

Approved for 118.0b8, thanks.

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/8cb7862e7ce9

Comment 12

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/8cb7862e7ce9

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9351198 [details]
Bug 1849269 - Add SharedWorkerManager::mRemoteWorkerController nullptr checking in SharedWorkerManager::Terminate(). r=#dom-worker-reviewers

Approved for ESR 115.3, thanks.

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/74134705e36a

Comment 15

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-esr115/rev/74134705e36a