Closed Bug 1873573 Opened 9 months ago Closed 9 months ago

crash near null in [@ mozilla::dom::(anonymous namespace)::PermissionResultRunnable::WorkerRun()]

Categories

(Core :: DOM: Service Workers, defect)

Product:
Core
Component:
DOM: Service Workers
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
124 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox121 --- wontfix
firefox122 --- wontfix
firefox123 --- fixed
firefox124 --- verified

People

(Reporter: tsmith, Assigned: edenchuang)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords, Whiteboard: [bugmon:bisected,confirmed])

Crash Data

Attachments

(2 files)

testcase.zip
965 bytes, application/x-zip-compressed
Details
Bug 1873573 - Rename PromiseWorkerProxy::WorkerPromise() to GetWorkerPromise(). r=#dom-worker-reviewers
48 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
Details | Review
Attached file testcase.zip —

Found while fuzzing m-c 20231108-f1fb5f0afb58 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -a --fuzzing -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

==145056==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x7f081557b8c4 bp 0x7f07f89026a0 sp 0x7f07f89024a0 T20)
==145056==The signal is caused by a READ memory access.
==145056==Hint: address points to the zero page.
    #0 0x7f081557b8c4 in get /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:314:27
    #1 0x7f081557b8c4 in operator nsIGlobalObject * /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:327:12
    #2 0x7f081557b8c4 in MaybeSomething<mozilla::dom::PermissionState &> /builds/worker/workspace/obj-build/dist/include/mozilla/dom/Promise.h:426:25
    #3 0x7f081557b8c4 in void mozilla::dom::Promise::MaybeResolve<mozilla::dom::PermissionState&>(mozilla::dom::PermissionState&) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/Promise.h:94:5
    #4 0x7f081557b6da in mozilla::dom::(anonymous namespace)::PermissionResultRunnable::WorkerRun(JSContext*, mozilla::dom::WorkerPrivate*) /builds/worker/checkouts/gecko/dom/push/PushManager.cpp:314:16
    #5 0x7f08166a7b8d in mozilla::dom::WorkerRunnable::Run() /builds/worker/checkouts/gecko/dom/workers/WorkerRunnable.cpp:342:12
    #6 0x7f080bed1846 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1193:16
    #7 0x7f080bedf19a in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
    #8 0x7f0816686733 in mozilla::dom::WorkerPrivate::DoRunLoop(JSContext*) /builds/worker/checkouts/gecko/dom/workers/WorkerPrivate.cpp:3341:7
    #9 0x7f081664dd09 in mozilla::dom::workerinternals::(anonymous namespace)::WorkerThreadPrimaryRunnable::Run() /builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp:2106:42
    #10 0x7f080bed1846 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1193:16
    #11 0x7f080bedf19a in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
    #12 0x7f080db6b721 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:300:20
    #13 0x7f080d991a6a in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370:10
    #14 0x7f080d991a6a in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
    #15 0x7f080d991a6a in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
    #16 0x7f080bec8060 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:370:10
    #17 0x7f0833ceb11f in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #18 0x5626cbc7e3aa in asan_thread_start(void*) /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:225:31
    #19 0x7f0833a94ac2 in start_thread nptl/pthread_create.c:442:8
    #20 0x7f0833b2665f  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:314:27 in get
Thread T20 created by T0 (Isolated Servic) here:
    #0 0x5626cbc67b4d in pthread_create /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:237:3
    #1 0x7f0833cd9844 in _PR_CreateThread /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:458:14
    #2 0x7f0833cc743e in PR_CreateThread /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:533:12
    #3 0x7f080becbca9 in nsThread::Init(nsTSubstring<char> const&) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:620:20
    #4 0x7f08166bb09a in mozilla::dom::WorkerThread::Create(mozilla::dom::WorkerThreadFriendKey const&) /builds/worker/checkouts/gecko/dom/workers/WorkerThread.cpp:101:7
    #5 0x7f081661cc27 in mozilla::dom::workerinternals::RuntimeService::ScheduleWorker(mozilla::dom::WorkerPrivate&) /builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp:1312:37
    #6 0x7f081661afd5 in mozilla::dom::workerinternals::RuntimeService::RegisterWorker(mozilla::dom::WorkerPrivate&) /builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp:1194:19
    #7 0x7f081667e0c8 in mozilla::dom::WorkerPrivate::Constructor(JSContext*, nsTSubstring<char16_t> const&, bool, mozilla::dom::WorkerKind, mozilla::dom::RequestCredentials, mozilla::dom::WorkerType, nsTSubstring<char16_t> const&, nsTSubstring<char> const&, mozilla::dom::WorkerLoadInfo*, mozilla::ErrorResult&, nsTString<char16_t>, std::function<void (bool)>&&, std::function<void ()>&&) /builds/worker/checkouts/gecko/dom/workers/WorkerPrivate.cpp:2612:24
    #8 0x7f08166d6632 in mozilla::dom::RemoteWorkerChild::ExecWorkerOnMainThread(mozilla::dom::RemoteWorkerData&&) /builds/worker/checkouts/gecko/dom/workers/remoteworkers/RemoteWorkerChild.cpp:350:41
    #9 0x7f081671a2d7 in operator() /builds/worker/checkouts/gecko/dom/workers/remoteworkers/RemoteWorkerChild.cpp:197:29
    #10 0x7f081671a2d7 in mozilla::detail::RunnableFunction<mozilla::dom::RemoteWorkerChild::ExecWorker(mozilla::dom::RemoteWorkerData const&)::$_0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:548:5
    #11 0x7f080bea13ea in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:568:16
    #12 0x7f080be8726b in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:895:26
    #13 0x7f080be83e48 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:718:15
    #14 0x7f080be84549 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:504:36
    #15 0x7f080bea9514 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:225:37
    #16 0x7f080bea9514 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_1>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
    #17 0x7f080bed145f in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1199:16
    #18 0x7f080bedf19a in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
    #19 0x7f080db69f63 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:107:5
    #20 0x7f080d991a6a in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370:10
    #21 0x7f080d991a6a in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
    #22 0x7f080d991a6a in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
    #23 0x7f0817304b99 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
    #24 0x7f0817509672 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:470:33
    #25 0x7f081c3bc81e in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:721:20
    #26 0x7f080d991a6a in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370:10
    #27 0x7f080d991a6a in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
    #28 0x7f080d991a6a in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
    #29 0x7f081c3bbdc3 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:656:34
    #30 0x5626cbcc1d9c in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #31 0x5626cbcc1d9c in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:375:18
    #32 0x7f0833a29d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

Debug builds also report: Assertion failure: mWorkerPromise, at /builds/worker/checkouts/gecko/dom/promise/Promise.cpp:889

Verified bug as reproducible on mozilla-central 20240108213208-fa142c3f71b8.
The bug appears to have been introduced in the following build range:

Start: 8ca41fa1fe7f665f3fb17e1ee1ecb9dc533e3970 (20230606015752)
End: 7974ac7e053aa00b02c6b9405b4fe87e6729456d (20230606074519)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8ca41fa1fe7f665f3fb17e1ee1ecb9dc533e3970&tochange=7974ac7e053aa00b02c6b9405b4fe87e6729456d

Keywords: regression
Whiteboard: [bugmon:bisected,confirmed]

That seems to point to bug 1800659.

Flags: needinfo?(echuang)
Keywords: pernosco-wanted
Regressed by: 1800659

Set release status flags based on info from the regressing bug 1800659

status-firefox121: --- → affected
status-firefox122: --- → affected
status-firefox-esr115: --- → unaffected
Assignee: nobody → echuang
Flags: needinfo?(echuang)
Crash Signature: [@ mozilla::dom::(anonymous namespace)::PermissionResultRunnable::WorkerRun()]
status-firefox121: affected → wontfix
status-firefox122: affected → fix-optional

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Keywords: pernosco-wanted → pernosco

(In reply to Bugmon [:jkratzer for issues] from comment #5)

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

It seems, bugmon encountered a problem here?

Flags: needinfo?(jkratzer)

Tyson, can you try and get a pernosco session of this locally? I'm unable to reproduce it under rr.

Flags: needinfo?(jkratzer) → needinfo?(twsmith)

I finally managed to get a working trace. This should be uploaded here shortly. Taking the NI back.

Flags: needinfo?(twsmith) → needinfo?(jkratzer)

A pernosco session for this bug can be found here.

(In reply to Bugmon [:jkratzer for issues] from comment #9)

A pernosco session for this bug can be found here.

I put a few notes.
PromiseWorkerProxy::CleanUp is called via the WorkerRef::Notify, nulls out mWorkerPromise and immediately also the worker ref, such that the worker shutdown is not blocked anymore. But we still have a PermissionResultRunnable in the queue which then gets executed and tries to resolve the promise but finds it null. IIUC we have this pattern in more places.

The question would be: What should PromiseWorkerProxy actually do on WorkerRef::Notify ? Should it just mark itself notified, such that subsequent uses from already scheduled runnables can check that if they want ? Or should each usage of PromiseWorkerProxy::WorkerPromise() check for a nullptr ? But I fear the promise might outlive the PromiseWorkerProxy::CleanUp and just remain pending ? When should we remove the worker ref ?

BTW, thanks!

Flags: needinfo?(jkratzer)
Flags: needinfo?(echuang)

Set release status flags based on info from the regressing bug 1800659

status-firefox124: --- → affected
Attachment #9375946 - Attachment description: Bug 1873573 - Don't touch PromiseWorkerProxy::mWorkerPromise if the PromiseWorkerProxy had already CleanedUp in PermissionResultRunnable. r=#dom-worker-reviewers → Bug 1873573 - Rename PromiseWorkerProxy::WorkerPromise() to GetWorkerPromise(). r=#dom-worker-reviewers
Pushed by echuang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a8c5441fd896 Rename PromiseWorkerProxy::WorkerPromise() to GetWorkerPromise(). r=dom-worker-reviewers,smaug

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

Status: NEW → RESOLVED
Closed: 9 months ago
status-firefox124: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch

Verified bug as fixed on rev mozilla-central 20240126214724-19005661ad78.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Status: RESOLVED → VERIFIED
status-firefox124: fixed → verified
Keywords: bugmon

:echuang should this get a beta uplift request for Fx123 or should it ride the train with Fx124?
It's a fuzzing bug, but is it also something users could hit?

status-firefox122: fix-optional → wontfix

Comment on attachment 9375946 [details]
Bug 1873573 - Rename PromiseWorkerProxy::WorkerPromise() to GetWorkerPromise(). r=#dom-worker-reviewers

Beta/Release Uplift Approval Request

  • User impact if declined: The tab might crash when the Worker is shutting down.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • 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): This patch makes accessing promises much safer in Workers. It doesn't change the accessing behavior, but checks if the promise is nullptr before accessing.
  • String changes made/needed: No
  • Is Android affected?: Yes
Flags: needinfo?(echuang)
Attachment #9375946 - Flags: approval-mozilla-beta?

Comment on attachment 9375946 [details]
Bug 1873573 - Rename PromiseWorkerProxy::WorkerPromise() to GetWorkerPromise(). r=#dom-worker-reviewers

Approved for 123 beta 6, thanks.

Attachment #9375946 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: