1811442 - 110 crashes on OpenBSD in mozilla::dom::MIDIPlatformService::Get

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Landry Breuil (:gaston)]

since 110 beta, firefox crashes when left unattended after some minutes.

backtraces seem to point at bug #1806495:

(gdb) bt
#0  thrkill () at /tmp/-:3
#1  0xb8975294cb4afb60 in ?? ()
#2  0x00000a97738fc695 in nsProfileLock::FatalSignalHandler (signo=11, info=0xa972b8bee50, context=0xa972b8bed60) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/toolkit/profile/nsProfileLock.cpp:174
#3  0x00000a9773c49527 in WasmTrapHandler (signum=11, info=0xa972b8bee50, context=0xa972b8bed60) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/js/src/wasm/WasmSignalHandlers.cpp:783
#4  <signal handler called>
#5  mozilla::dom::MIDIPlatformService::Get () at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/dom/midi/MIDIPlatformService.cpp:210
#6  0x00000a9771c6aba9 in mozilla::ipc::BackgroundParentImpl::RecvCreateMIDIManager(mozilla::ipc::Endpoint<mozilla::dom::PMIDIManagerParent>&&)::$_5::operator()() (this=<optimized out>) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/ipc/glue/BackgroundParentImpl.cpp:1268
#7  mozilla::detail::RunnableFunction<mozilla::ipc::BackgroundParentImpl::RecvCreateMIDIManager(mozilla::ipc::Endpoint<mozilla::dom::PMIDIManagerParent>&&)::$_5>::Run() (this=<optimized out>) at /usr/obj/ports/firefox-110.0beta2/build-amd64/dist/include/nsThreadUtils.h:546
#8  0x00000a97719876ed in mozilla::TaskQueue::Runner::Run (this=0xa97b2dc15a0) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/xpcom/threads/TaskQueue.cpp:259
#9  0x00000a977198ee9c in nsThreadPool::Run (this=0xa97500ca400) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/xpcom/threads/nsThreadPool.cpp:310
#10 0x00000a976df81c01 in nsThread::ProcessNextEvent (this=0xa97425b5200, aMayWait=<optimized out>, aResult=<optimized out>) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/xpcom/threads/nsThread.cpp:1191
#11 0x00000a976e5c42b2 in NS_ProcessNextEvent (aThread=0x0, aMayWait=false) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/xpcom/threads/nsThreadUtils.cpp:473
#12 0x00000a9771c78034 in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0xa979f727a40, aDelegate=0xa972b8bf478) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/ipc/glue/MessagePump.cpp:300
#13 0x00000a976e70ba4b in MessageLoop::RunInternal (this=0xa972b8bf0d0) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/ipc/chromium/src/base/message_loop.cc:381
#14 MessageLoop::RunHandler (this=0xa972b8bf0d0) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/ipc/chromium/src/base/message_loop.cc:374
#15 MessageLoop::Run (this=0xa972b8bf0d0) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/ipc/chromium/src/base/message_loop.cc:356
#16 0x00000a977198b69e in nsThread::ThreadFunc (aArg=0xa97425aa700) at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/xpcom/threads/nsThread.cpp:383
#17 0x00000a975f02fca5 in _pt_root (arg=0xa972fd8b800) at ptthread.c:201
#18 0x00000a97e7564731 in _rthread_start (v=<optimized out>) at /usr/src/lib/librthread/rthread.c:96
#19 0x00000a974aac9e9a in __tfork_thread () at /usr/src/lib/libc/arch/amd64/sys/tfork_thread.S:86

(gdb) frame 5
#5  mozilla::dom::MIDIPlatformService::Get () at /usr/obj/ports/firefox-110.0beta2/firefox-110.0/dom/midi/MIDIPlatformService.cpp:210
210     /usr/obj/ports/firefox-110.0beta2/firefox-110.0/dom/midi/MIDIPlatformService.cpp: No such file or directory.
(gdb) p gMIDIPlatformService
$1 = {mRawPtr = 0x0}

https://searchfox.org/mozilla-central/source/dom/midi/MIDIPlatformService.cpp#210 that will be null

10:12 <tb> this will be NULL if midi_testing is not set
10:13 <tb> and MOZ_WEBMID_MIDIR_IMPL is not defined.
10:13 <tb> looks like it need a NULL check.

Comment 1

[Landry Breuil (:gaston)]

according to searchfox (https://searchfox.org/mozilla-central/search?q=MIDIPlatformService%3A%3AGet%28%29&path=&case=false&regexp=false) it seems all callers of MIDIPlatformService::Get() dont check for NULL return values, so something should be returned anyway, just adding a NULL check wont help..

Comment 2

[Landry Breuil (:gaston)]

i guess we crash because MOZ_WEBMIDI_MIDIR_IMPL is undef for us, because https://searchfox.org/mozilla-central/source/toolkit/moz.configure#181

Comment 3

[Landry Breuil (:gaston)]

see also bug #1747196 for previous related failures; trying a build of 110.0b3 with MOZ_WEBMIDI_MIDIR_IMPL force-defined to TRUE.. who knows.

Comment 4

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

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

:bholley, since you are the author of the regressor, bug 1806495, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 5

[Landry Breuil (:gaston)]

petr, since you had filed bug #1747196 i guess you're also seeing this ?

Comment 6

[Landry Breuil (:gaston)]

poking at totally random things, i toggled midi.testing to true in my profile currently used w/ 110.0b2 to see if i'd get crashes.

Comment 7

[Landry Breuil (:gaston)]

there's a null check in https://searchfox.org/mozilla-central/source/dom/midi/MIDIPlatformService.cpp#176 so if all callers of MIDIPlatformService::Get() are only called if MIDIPlatformService::IsRunning() is checked first we might get away with a null check inside MIDIPlatformService::Get()

Comment 8

[Landry Breuil (:gaston)]

force-defining MOZ_WEBMIDI_MIDIR_IMPL to true puts us back with the build failures seen in bug #1747196. So far havent had crashes of 110.0b2 with midi.testing set to true.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Bobby I'm wondering what would be the right approach here. I'd say we should keep exposing the MIDI API even if we don't have an underlying implementation to prevent fingerprinting based on that. Maybe we could simply reject the access request like we do for users who don't have any devices?

Comment 10

[Petr Sumbera]

I can confirm I see the issue. I do just regular builds so I haven't really spot it as I concentrate on ESR versions.

Assertion failure: mRawPtr, at /builds/psumbera/mozilla-central-build/obj-x86_64-pc-solaris2.11/dist/include/mozilla/StaticPtr.h:155
#01: mozilla::StaticRefPtr<mozilla::dom::MIDIPlatformService>::operator->() const[/root/INSTALLED/lib/firefox/libxul.so +0x1cc31e9b]
#02: mozilla::dom::MIDIPlatformService::Get()[/root/INSTALLED/lib/firefox/libxul.so +0x1cc29762]
#03: mozilla::ipc::BackgroundParentImpl::RecvCreateMIDIManager(mozilla::ipc::Endpoint<mozilla::dom::PMIDIManagerParent>&&)::{lambda()#1}::operator()()[/root/INSTALLED/lib/firefox/libxul.so +0x190b709d]
#04: mozilla::detail::RunnableFunction<mozilla::ipc::BackgroundParentImpl::RecvCreateMIDIManager(mozilla::ipc::Endpoint<mozilla::dom::PMIDIManagerParent>&&)::{lambda()#1}>::Run()[/root/INSTALLED/lib/firefox/libxul.so +0x190bcaaa]
#05: mozilla::TaskQueue::Runner::Run()[/root/INSTALLED/lib/firefox/libxul.so +0x1831e19c]
#06: nsThreadPool::Run()[/root/INSTALLED/lib/firefox/libxul.so +0x18330cc0]
#07: nsThread::ProcessNextEvent(bool, bool*)[/root/INSTALLED/lib/firefox/libxul.so +0x1832a5c0]
#08: NS_ProcessNextEvent(nsIThread*, bool)[/root/INSTALLED/lib/firefox/libxul.so +0x1833444a]
#09: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)[/root/INSTALLED/lib/firefox/libxul.so +0x1911e97c]
#10: MessageLoop::RunInternal()[/root/INSTALLED/lib/firefox/libxul.so +0x190572d5]
#11: MessageLoop::RunHandler()[/root/INSTALLED/lib/firefox/libxul.so +0x19057258]
#12: MessageLoop::Run()[/root/INSTALLED/lib/firefox/libxul.so +0x1905721d]
#13: nsThread::ThreadFunc(void*)[/root/INSTALLED/lib/firefox/libxul.so +0x18326f04]
#14: _pt_root[/root/INSTALLED/lib/firefox/libnspr4.so +0x4b0a6]
#15: _thrp_setup[/lib/amd64/libc.so.1 +0x258b03]
#16: _lwp_start[/lib/amd64/libc.so.1 +0x258e30]

Comment 11

[Ryan VanderMeulen [:RyanVM]]

109 isn't actually affected - the regressing change only landed in 110.

Comment 12

[Bobby Holley (:bholley)]

Yeah, I guess we should check for the build flag and bail out somewhere around here.

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Bug 1811442 - Automatically reject requests for MIDI access when no implementation is present r=bholley

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01632825ec98
Automatically reject requests for MIDI access when no implementation is present r=bholley

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/01632825ec98

Comment 16

[Landry Breuil (:gaston)]

Comment on attachment 9313362 [details]
Bug 1811442 - Automatically reject requests for MIDI access when no implementation is present r=bholley

Beta/Release Uplift Approval Request

• User impact if declined: crashes on tier3
• Is this code covered by automated tests?: No
• 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): tier3 only
• String changes made/needed:
• Is Android affected?: No

Comment 17

[Landry Breuil (:gaston)]

thanks :gsvelto, i've had multiple users reporting that my build of 110.0b4 with the above patch backported fixed all the crashes we were seeing... hence requesting the backport to beta :)

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9313362 [details]
Bug 1811442 - Automatically reject requests for MIDI access when no implementation is present r=bholley

Approved for 110 beta 5, thanks.

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/30bc9941b691

Comment 20

[Landry Breuil (:gaston)]

can confirm that 110b5 works fine !