1892644 - Crash in [@ mozilla::ThreadBound<T>::IsCorrectThread]

Status: RESOLVED FIXED

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

Description

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

Crash report: https://crash-stats.mozilla.org/report/index/43854620-3d14-4f12-b016-315670240421

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  std::_Atomic_storage<const PRThread*, 8>::load const  /builds/worker/fetches/vs/VC/Tools/MSVC/14.39.33519/include/atomic:1121
0  xul.dll  mozilla::detail::IntrinsicMemoryOps<const PRThread*, 1>::load  mfbt/Atomics.h:194
0  xul.dll  mozilla::detail::AtomicBaseIncDec<const PRThread*, 1>::operator const PRThread* const  mfbt/Atomics.h:339
0  xul.dll  mozilla::ThreadBound<mozilla::dom::WorkerPrivate::WorkerThreadAccessible>::IsCorrectThread const  xpcom/threads/ThreadBound.h:128
0  xul.dll  mozilla::ThreadBound<mozilla::dom::WorkerPrivate::WorkerThreadAccessible>::AssertIsCorrectThread const  xpcom/threads/ThreadBound.h:135
0  xul.dll  mozilla::ThreadBound<mozilla::dom::WorkerPrivate::WorkerThreadAccessible>::Accessor<1>::Accessor  xpcom/threads/ThreadBound.h:100
0  xul.dll  mozilla::ThreadBound<mozilla::dom::WorkerPrivate::WorkerThreadAccessible>::Access const  xpcom/threads/ThreadBound.h:125
0  xul.dll  mozilla::dom::WorkerPrivate::CancelBeforeWorkerScopeConstructed const  dom/workers/WorkerPrivate.h:1144
0  xul.dll  mozilla::dom::WorkerThreadRunnable::Run  dom/workers/WorkerRunnable.cpp:375
1  xul.dll  nsThread::ProcessNextEvent  xpcom/threads/nsThread.cpp:1193

By querying Nightly crashes reported within the last 2 months, here are some insights about the signature:

• First crash report: 2024-04-21
• Process type: Multiple distinct types
• Is startup crash: No
• Has user comments: No
• Is null crash: Yes - all crashes happened on null or near null memory address

By analyzing the backtrace, the regression may have been introduced by a patch [1] to fix Bug 1769913.

[1] https://hg.mozilla.org/mozilla-central/rev?node=ffc69aa8f2c3

:edenchuang, since you are the author of the potential regressor, could you please take a look?

Comment 1

[Eden Chuang[:edenchuang]]

In bug 1769913, we remove the WorkerThreadRunnable's raw pointer to the corresponding WorkerPrivate and expect the corresponding WorkerPrivate to be obtained by GetCurrentThreadWorkerPrivate() when WorkerThreadRunnable::Run executing.

In general, the assumption is correct. However, it could be violated in the following two situations.

1. WorkerJSContext initialization fails.
2. WorkerThreadRunnable dispatching after CycleCollector shutdown.
   In both cases, there is no corresponding WorkerJSContext to get its mWorkerPrivate by GetCurrentThreadWorkerPrivate(), which causes WorkerThreadRunnable to have no WorkerPrivate for executing.

For case 1, the WorkerThreadRunnables are all from WorkerPrivate::mPreStartRunnables. These runnables need a WorkerPrivate to execute WorkerRun(). However, I am not sure the current behavior, which executing WorkerRun() with an invalid WorkerPrivate, is a good way to do. But if we want to keep current behavior, we must to find a way to get WorkerPrivate instead of calling GetCurrentThreadWorkerPrivate().

For case 2, the solution could be much easier since the WorkerThreadRunnable is not really a WorkerThreadRunnable. Because the Worker is in the "Dead" state, the WorkerThreadRunnable could not be dispatched normally, which means by WorkerThreadRunnable::Dispatch(WorkerPrivate). So it must be from NS_DispatchToCurrentThread(), WorkerThread::Dispatch(), or other ways which treat WorkerThread as nsIThread/nsISerialEventTarget, where the runnable is wrapped as a WorkerThreadRunnable and call nsThread::Dispatch() directly. In this case, the runnable does not need to be WorkerThreadRunnable, so we should be able to call the runnable's Run() directly.

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug 1892644 - Handling WorkerThreadRunnable::Run() after Worker is "Dead". r=#dom-worker-reviewers

In bug 1769913, we remove the WorkerThreadRunnable's raw pointer to the corresponding WorkerPrivate and expect the corresponding WorkerPrivate to be obtained by GetCurrentThreadWorkerPrivate() when WorkerThreadRunnable::Run executing.

In general, the assumption is correct. However, it could be violated in the following two situations.

1. WorkerJSContext initialization fails.
2. WorkerThreadRunnable dispatching after CycleCollector shutdown.
   In both cases, there is no corresponding WorkerJSContext to get its mWorkerPrivate by GetCurrentThreadWorkerPrivate(), which causes WorkerThreadRunnable to have no WorkerPrivate for executing.

For case 1, the WorkerThreadRunnables are all from WorkerPrivate::mPreStartRunnables. These runnables need a WorkerPrivate to execute WorkerRun(). To cleanup the resources through WorkerRun() if Worker initialization fails or shutdown before DoRunLoop() starts, WorkerThreadRunnable saves a CheckedUnsafePtr<WorkerPrivate> when the runnable is dispacthed to WorkerPrivate::mPreStartRunnables.

For case 2, the solution could be much easier since the WorkerThreadRunnable is not really a WorkerThreadRunnable. Because the Worker is in the "Dead" state, the WorkerThreadRunnable could not be dispatched normally, which means by WorkerThreadRunnable::Dispatch(WorkerPrivate). So it must be from NS_DispatchToCurrentThread(), WorkerThread::Dispatch(), or other ways which treat WorkerThread as nsIThread/nsISerialEventTarget, where the runnable is wrapped as a WorkerThreadRunnable and call nsThread::Dispatch() directly. In this case, the runnable does not need to be WorkerThreadRunnable, so we should be able to call the runnable's Run() directly.

Comment 3

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

The bug is marked as tracked for firefox127 (nightly). However, the bug still has low severity.

:jstutte, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e3ed11f4e8a
Handling WorkerThreadRunnable::Run() after Worker is "Dead". r=asuth

Comment 5

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/9e3ed11f4e8a