1931391 - Assertion failure: IsServiceWorker(), at /builds/worker/workspace/obj-build/dist/include/mozilla/dom/WorkerPrivate.h:822

Status: VERIFIED FIXED

(Core :: DOM: Notifications, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.zip

Found while fuzzing m-c 20241109-bcec5f42cb2e (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

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

Assertion failure: IsServiceWorker(), at /builds/worker/workspace/obj-build/dist/include/mozilla/dom/WorkerPrivate.h:822

#0 0x72fef47da4e8 in GetServiceWorkerDescriptor /builds/worker/workspace/obj-build/dist/include/mozilla/dom/WorkerPrivate.h:822:5
#1 0x72fef47da4e8 in ServiceWorkerID /builds/worker/workspace/obj-build/dist/include/mozilla/dom/WorkerPrivate.h:801:45
#2 0x72fef47da4e8 in mozilla::dom::CheckLoadRunnable::MainThreadRun() /builds/worker/checkouts/gecko/dom/notification/Notification.cpp:1859:59
#3 0x72fef4f8aed3 in mozilla::dom::WorkerMainThreadRunnable::Run() /builds/worker/checkouts/gecko/dom/workers/WorkerRunnable.cpp:649:20
#4 0x72feefbf5707 in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() /builds/worker/checkouts/gecko/xpcom/threads/ThrottledEventQueue.cpp:254:22
#5 0x72feefbf2941 in mozilla::ThrottledEventQueue::Inner::Executor::Run() /builds/worker/checkouts/gecko/xpcom/threads/ThrottledEventQueue.cpp:81:15
#6 0x72feefbc9557 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:618:16
#7 0x72feefbbedb9 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:945:26
#8 0x72feefbbd7f7 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:768:15
#9 0x72feefbbdc75 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:554:36
#10 0x72feefbccf99 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:271:37
#11 0x72feefbccf99 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_1>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
#12 0x72feefbe085b in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1155:16
#13 0x72feefbe753f in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#14 0x72fef0778733 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:107:5
#15 0x72fef06ca441 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:362:3
#16 0x72fef06ca441 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:344:3
#17 0x72fef5486188 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#18 0x72fef5539068 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:469:33
#19 0x72fef642101b in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:651:20
#20 0x72fef07795d6 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:235:9
#21 0x72fef06ca441 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:362:3
#22 0x72fef06ca441 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:344:3
#23 0x72fef642043a in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:586:34
#24 0x5c5725f8cede in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:397:22

Comment 1

[Mayank Bansal]

Got a crash from the testcase: https://crash-stats.mozilla.org/report/index/c2fcb54f-42e4-418b-8516-7255f0241115

Comment 2

[Mayank Bansal]

Bisection:

Bug 1544232 - Limit lifetime extension of SWs by SWs to the sender's lifetime. r=edenchuang

Differential Revision: https://phabricator.services.mozilla.com/D180915

Comment 3

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

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

Comment 4

[Bugmon [:jkratzer for issues]]

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

Start: 7936ca01a900402531a01de23474744fde9bbe1c (20241024094434)
End: 1fd874fb28a2db5c84df8126967b1d668856cbb9 (20241024082130)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7936ca01a900402531a01de23474744fde9bbe1c&tochange=1fd874fb28a2db5c84df8126967b1d668856cbb9

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

So this check seems to be enforcing a constraint that's not in the spec[1] that you can only call ServiceWorkerRegistration.showNotification() from the active ServiceWorker's global. This implementation constraint was initially added in bug 1114554.

This is a newly possible crash since previously it was not possible to access a ServiceWorkerRegistration on a worker that wasn't a ServiceWorker before bug 1131324 from the stack referenced in comment 2. We expect this test case to trigger this release assert on builds where we aren't diagnostic asserting here (in content processes) (because ServiceWorkerID() is only safe to call if you are sure you're on a ServiceWorker; there is a fallible version for cases where we aren't).

:saschanaz, do you read the spec the same way about not needing the active check?

Test-wise, it seems like we could probably add WPT coverage to call SWR.showNotification from a Worker or SharedWorker (the test case uses SharedWorker but I don't think that actually matters) if we wanted to get coverage without adding a crash test that is unlikely to ever be relevant again.

1: Step 3 of SWR.showNotification is the only constraint related to the active worker and it doesn't include constraining the caller to need to be the active worker:

If this’s active worker is null, then reject promise with a TypeError and return promise.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

That check kinda looks like it tried to implement step 3 in a wrong way. Agreed for the test coverage, we need showNotification-from-worker tests.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

(Sounds like a notification thing)

Comment 8

[Tyson Smith [:tsmith]]

This issue is frequently reported by fuzzers, please prioritize it accordingly.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1931391 - Simplify ServiceWorkerRegistration.showNotification steps. r=saschanaz!

Changes in Bug 1131324 made it possible for
ServiceWorkerRegistration.showNotification to be invoked from Dedicated
and Shared Workers where previously it could only be invoked from
Service Workers. The fuzzer discovered this new possibility for which
we did not have test coverage for and revealed that the code would call
a WorkerPrivate method that required the global be a ServiceWorker which
causes a diagnostic assert or a release assert depending on the build
type.

It turns out the check in question was enforcing a constraint not
required by the spec that only the active worker associated with the
registration can cause a notification to be shown associated with the
registration. All that is required is that the registration have some
active worker.

When addressing this it also turned out that since the initial
implementation of the notifications API there has been an unneccessary
check that the principal associated with the global is same-origin to
the scope. This check additionally required a sync runnable to be
dispatched to the main thread. This check was unnecessary and has been
removed. All ServiceWorkerRegistrations exposed to a global are
inherently same-origin to the global and the check as implemented was
not aware of the effective storage principal and was just using the
(node) principal. (And any checks concerned about dealing with a rogue
content process would need to take place in the parent process, not the
potentially rogue content process.)

For testing, the recently updated test
test_notification_serviceworker_show.html has been forked to provide
dedicated worker coverage. If the test is run without the fix in this
patch applied, the expected assertion trips.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2dc4a6011b7
Simplify ServiceWorkerRegistration.showNotification steps. r=saschanaz

Comment 11

[Sandor Molnar[:smolnar]]

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

Comment 12

[Bugmon [:jkratzer for issues]]

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