1837736 - Crash in [@ mozilla::dom::ThreadSafeWorkerRef::Private]

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

5 crashes from 5 different installations of Firefox 116.0a1, build IDs 20230607033033 and 20230609093445. No crashes for a few versions before.

Could this be from the changes in bug 1800659?

Crash report: https://crash-stats.mozilla.org/report/index/e7cc52c9-6b92-4442-8fb7-0aa440230610

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  RefPtr<mozilla::dom::StrongWorkerRef>::get const  mfbt/RefPtr.h:325
0  xul.dll  RefPtr<mozilla::dom::StrongWorkerRef>::operator-> const  mfbt/RefPtr.h:355
0  xul.dll  mozilla::dom::ThreadSafeWorkerRef::Private const  dom/workers/WorkerRef.cpp:212
1  xul.dll  mozilla::dom::WorkerFetchResolver::GetAbortSignalForTargetThread  dom/fetch/Fetch.cpp:263
1  xul.dll  mozilla::dom::WorkerFetchResponseRunnable::WorkerRun  dom/fetch/Fetch.cpp:816
2  xul.dll  mozilla::dom::WorkerRunnable::Run  dom/workers/WorkerRunnable.cpp:372
3  xul.dll  nsThread::ProcessNextEvent  xpcom/threads/nsThread.cpp:1193
4  xul.dll  NS_ProcessNextEvent  xpcom/threads/nsThreadUtils.cpp:479
5  xul.dll  mozilla::dom::WorkerPrivate::DoRunLoop  dom/workers/WorkerPrivate.cpp:3341
6  xul.dll  mozilla::dom::workerinternals::  dom/workers/RuntimeService.cpp:2141

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Bug 1837736 - Don't execute WorkerFetchResponseRunnable::WorkerRun when Worker is shutting down r=asuth

This is a case that Worker is shutting down when WorkerFetchResponseRunnable is in queue.

When Worker is getting into "Canceling" all WorkerRefs are notified, then queued WorkerRunnable are executed.
And this causes PromiseWorkerProxy::mWorkerRef be released before the WorkerRunnable execution.
Before Fx 116, there is no this problem since the queued WorkerFetchResponseRunnable is canceled by WorkerPrivate::ClearMainEventQueue() mechanism.

However, WorkerFetchResponseRunnable needs WorkerRef to keep Worker alive for its WorkerRun execution.

Here we just check if WorkerFetchResolver::mWorkerRef is notified or not to judge if the Worker is in "Canceling." Once the Worker is in "Canceling," return true in WorkerFetchResponseRunnable::WorkerRun directly.

We might need a mechanism for PromiseWorkerProxy to notify the proxy owner worker is shutting down for proxy owner has a way to release resources.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/140d21dede36
Don't execute WorkerFetchResponseRunnable::WorkerRun when Worker is shutting down r=asuth

Comment 3

[Eden Chuang[:edenchuang]]

Comment on attachment 9344372 [details]
Bug 1837736 - Don't execute WorkerFetchResponseRunnable::WorkerRun when Worker is shutting down r=asuth

Beta/Release Uplift Approval Request

• User impact if declined: Users could meet UAFs when chrome worker is shutting down during a resource fetching.
• 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 risk is low since we only add a status checking for Worker to ensure that the runnable would not execute while Worker shutting down.
• String changes made/needed: No
• Is Android affected?: Yes

Comment 4

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/140d21dede36

Comment 5

[Dianna Smith [:diannaS]]

Comment on attachment 9344372 [details]
Bug 1837736 - Don't execute WorkerFetchResponseRunnable::WorkerRun when Worker is shutting down r=asuth

Approved for 116.0b8

Comment 6

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b8a4837bb60