1930717 - Hit MOZ_CRASH(ServiceWorkerManager not thread-safe) at /builds/worker/checkouts/gecko/xpcom/base/nsISupportsImpl.cpp:43

Status: RESOLVED INVALID

(Core :: DOM: Service Workers, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.zip

Found while fuzzing 20241109-bcec5f42cb2e (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

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

Hit MOZ_CRASH(ServiceWorkerManager not thread-safe) at /builds/worker/checkouts/gecko/xpcom/base/nsISupportsImpl.cpp:43

#0 0x7125ef04f9b6 in MOZ_Crash /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:324:3
#1 0x7125ef04f9b6 in nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const /builds/worker/checkouts/gecko/xpcom/base/nsISupportsImpl.cpp:43:5
#2 0x7125f9484f31 in AssertOwnership<37> /builds/worker/workspace/obj-build/dist/include/nsISupportsImpl.h:59:5
#3 0x7125f9484f31 in AddRef /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerManager.cpp:405:1
#4 0x7125f9484f31 in AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:48:39
#5 0x7125f9484f31 in AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:408:35
#6 0x7125f9484f31 in RefPtr /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:108:7
#7 0x7125f9484f31 in mozilla::dom::ServiceWorkerManager::GetInstance() /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerManager.cpp:1501:39
#8 0x7125f9497ea7 in mozilla::dom::ServiceWorkerManager::LocalizeAndReportToAllClients(nsTString<char> const&, char const*, nsTArray<nsTString<char16_t>> const&, unsigned int, nsTString<char> const&, nsTString<char16_t> const&, unsigned int, unsigned int) /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerManager.cpp:1520:38
#9 0x7125f8e2b9a9 in mozilla::dom::(anonymous namespace)::ReportFetchListenerWarningRunnable::Run() /builds/worker/checkouts/gecko/dom/workers/WorkerScope.cpp:1240:5
#10 0x7125ef206aa9 in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() /builds/worker/checkouts/gecko/xpcom/threads/ThrottledEventQueue.cpp:254:22
#11 0x7125ef200bbf in mozilla::ThrottledEventQueue::Inner::Executor::Run() /builds/worker/checkouts/gecko/xpcom/threads/ThrottledEventQueue.cpp:81:15
#12 0x7125ef1bdcea in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:618:16
#13 0x7125ef1a9f9e in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:945:26
#14 0x7125ef1a77b8 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:768:15
#15 0x7125ef1a7dd6 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:554:36
#16 0x7125ef1c4fe4 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:271:37
#17 0x7125ef1c4fe4 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_1>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
#18 0x7125ef1e54df in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1155:16
#19 0x7125ef1f0238 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#20 0x7125f07c0663 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:107:5
#21 0x7125f06a51e4 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:369:10
#22 0x7125f06a51e4 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:362:3
#23 0x7125f06a51e4 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:344:3
#24 0x7125f97899b9 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#25 0x7125f992c29a in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:469:33
#26 0x7125fb5d19dd in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:651:20
#27 0x7125f06a51e4 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:369:10
#28 0x7125f06a51e4 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:362:3
#29 0x7125f06a51e4 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:344:3
#30 0x7125fb5cfe8c in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:586:34
#31 0x613ba57a88f9 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:397:22

Comment 1

[Tyson Smith [:tsmith]]

Debug builds report:

Assertion failure: NS_IsMainThread(), at /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerManager.cpp:1495

#0 0x7fffed5c2ac5 in mozilla::dom::ServiceWorkerManager::GetInstance() /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerManager.cpp:1495:5
#1 0x7fffe7e6e3c5 in mozilla::xpcom::CreateInstanceImpl(mozilla::xpcom::ModuleID, nsID const&, void**) /builds/worker/workspace/obj-build/xpcom/components/StaticComponents.cpp:11783:57
#2 0x7fffe7e8190e in CreateInstance /builds/worker/checkouts/gecko/xpcom/components/nsComponentManager.cpp:189:46
#3 0x7fffe7e8190e in nsComponentManagerImpl::GetServiceLocked(mozilla::Maybe<mozilla::detail::BaseMonitorAutoLock<mozilla::Monitor>>&, (anonymous namespace)::EntryWrapper&, nsID const&, void**) /builds/worker/checkouts/gecko/xpcom/components/nsComponentManager.cpp:986:17
#4 0x7fffe7e81f38 in nsComponentManagerImpl::GetService(mozilla::xpcom::ModuleID, nsID const&, void**) /builds/worker/checkouts/gecko/xpcom/components/nsComponentManager.cpp:1076:10
#5 0x7fffe7e7723e in mozilla::xpcom::GetServiceHelper::operator()(nsID const&, void**) const /builds/worker/workspace/obj-build/xpcom/components/StaticComponents.cpp:13542:50
#6 0x7fffecaabd15 in assign_from_helper /builds/worker/workspace/obj-build/dist/include/nsCOMPtr.h:901:7
#7 0x7fffecaabd15 in nsCOMPtr<nsIServiceWorkerManager>::nsCOMPtr(nsCOMPtr_helper const&) /builds/worker/workspace/obj-build/dist/include/nsCOMPtr.h:537:5
#8 0x7fffed5a1b6b in mozilla::dom::ServiceWorkerContainer::GetScopeForUrl(nsTSubstring<char16_t> const&, nsTString<char16_t>&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerContainer.cpp:614:7
#9 0x7fffea7ae9c4 in mozilla::dom::ServiceWorkerContainer_Binding::getScopeForUrl(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/./ServiceWorkerContainerBinding.cpp:1015:24
#10 0x7fffeb0e87c7 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3290:13
#11 0x7fffee8a6ff4 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:532:13
#12 0x7fffee8a67d8 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:628:12
#13 0x7fffef3f70ef in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jit/BaselineIC.cpp:1683:10
#14 0x38077fb2bd5e  ([anon:js-executable-memory]+0x1bd5e)

Comment 2

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/UP6e7eNwcuQCTrR9eiUhYg/index.html

Comment 3

[Bugmon [:jkratzer for issues]]

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

Start: 2b86d382dab2e182131315e79fa26bb32affa790 (20241024013316)
End: 7936ca01a900402531a01de23474744fde9bbe1c (20241024040951)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2b86d382dab2e182131315e79fa26bb32affa790&tochange=7936ca01a900402531a01de23474744fde9bbe1c

Comment 4

[Andrew McCreight [:mccr8]]

asuth has a big pile of patches in that regression range.

Comment 5

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

The fuzzer is using a testing-only method here that it should not be using:

// Testing only.
partial interface ServiceWorkerContainer {
  [Throws,Pref="dom.serviceWorkers.testing.enabled"]
  DOMString getScopeForUrl(DOMString url);
};

This is the SW script:

(async () => {
    await self.navigator.storage.getDirectory()
    self.navigator.serviceWorker.getScopeForUrl("missing.js")
    self.addEventListener("fetch", async () => {}, { })
})()

Comment 6

[Tyson Smith [:tsmith]]

asuth: I will remove the pref from prefpicker so it will not be set for browser fuzzing. Is that ok?

Comment 7

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

(In reply to Tyson Smith [:tsmith] from comment #6)

asuth: I will remove the pref from prefpicker so it will not be set for browser fuzzing. Is that ok?

Probably. The test pref does a few things:

• make ServiceWorkers work without being in a SecureContext
• enables some testing flags

Presumably the fuzzer reliably either uses localhost which is a securecontext or dom.securecontext.allowlist which can make anything a SecureContext.

You might want to disable dom.caches.testing.enabled too since it basically has the same securecontext bypass rationale, although I don't think it has any weird testing functions hooked up (and in general we want to move most of these pref checks to be is-in-automation checks via the official API). The other dom testing prefs I see in there like dom.push.testing.ignorePermission, dom.storageManager.prompt.testing, and dom.storageManager.prompt.testing.allow all seem okay because they let the fuzzer gain access to content APIs that would otherwise be hard to get to although I haven't validated they are hooked up still (there are various changes happening with the push impl and this file is not in-tree). (Although maybe the fuzzer already has other permission-granting magic?)

Comment 8

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

Note that I have filed bug 1930983 on removing the test method in response to this bug since I believe it turns out we don't use this WebIDL method at all; as briefly discussed in bug 1930983, we only seem to use the underlying XPIDL method in the parent process main thread via specialpowers (which is the only place it's safe to be doing this anyway). I haven't added a see-also for security bug hygiene reasons, but this bug I guess also isn't a security bug after all?

Comment 9

[Bugmon [:jkratzer for issues]]

No valid actions for resolution (INVALID).
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.