Crash in mozilla::widget::AudioSession::OnSessionDisconnectedInternal

RESOLVED FIXED in Firefox -esr52

Status

()

P1
critical
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: philipp, Assigned: handyman)

Tracking

(Blocks: 1 bug, {crash, csectype-uaf, sec-high})

52 Branch
mozilla58
x86
Windows 7
crash, csectype-uaf, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(thunderbird_esr52 fixed, firefox51 unaffected, firefox52 wontfix, firefox-esr5257+ fixed, firefox53 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57+ fixed, firefox58 fixed)

Details

(Whiteboard: tpi:+, win7only[tbird crash][adv-main57+][adv-esr52.5+][post-critsmash-triage], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-b2ba2be5-fd5e-462e-9aa4-442592170213.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 		@0x65a7d2c5 	
1 	xul.dll 	mozilla::widget::AudioSession::OnSessionDisconnectedInternal() 	widget/windows/AudioSession.cpp:428
2 	xul.dll 	mozilla::detail::RunnableMethodImpl<void ( mozilla::AbstractMirror<bool>::*)(void), 1, 0>::Run() 	obj-firefox/dist/include/nsThreadUtils.h:810
3 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1216
4 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:361
5 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:124
6 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
7 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:225
8 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:205
9 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
10 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:262
11 	xul.dll 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp:869
12 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:269
13 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:225
14 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:205
15 	xul.dll 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp:701
16 	firefox.exe 	content_process_main(int, char** const) 	ipc/contentproc/plugin-container.cpp:197
17 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
18 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
19 	kernel32.dll 	BaseThreadInitThunk 	
20 	ntdll.dll 	__RtlUserThreadStart 	
21 	ntdll.dll 	_RtlUserThreadStart

this crash signature has been present in older builds for a while, but it seems to have sprung up again on 52.0b and subsequent beta builds in 32bit builds on windows 7. 
so maybe there's a clue in the pushlog link for changes between beta 1 & beta 2: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_52_0b1_RELEASE&tochange=FIREFOX_52_0b2_RELEASE
Jim, any thoughts on who might be able to investigate?
Flags: needinfo?(jmathies)
I've been trying to reproduce on win7 this morning, no luck. afaict these events are working generally. Whatever causes this must be some sort of corner case.
Flags: needinfo?(jmathies)
Priority: -- → P2
Whiteboard: tpi:+, win7only
status-firefox52: affected → fix-optional

Updated

2 years ago
See Also: → bug 1338504
(Reporter)

Updated

2 years ago
Crash Signature: [@ mozilla::widget::AudioSession::OnSessionDisconnectedInternal] → [@ mozilla::widget::AudioSession::OnSessionDisconnectedInternal] [@0x0 | mozilla::widget::AudioSession::OnSessionDisconnectedInternal]

Updated

2 years ago
Priority: P2 → P1
status-firefox52: fix-optional → wontfix
status-firefox53: affected → wontfix
status-firefox54: ? → fix-optional
status-firefox55: --- → affected
status-firefox-esr52: --- → affected

Updated

2 years ago
Blocks: 731812

Updated

2 years ago
status-thunderbird_esr52: --- → affected
Whiteboard: tpi:+, win7only → tpi:+, win7only[tbird topcrash]
This is a P1 bug without an assignee. 

P1 are bugs which are being worked on for the current release cycle/iteration/sprint. 

If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.
Keywords: stale-bug

Updated

2 years ago
Blocks: 1394024
status-firefox55: affected → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → affected
(Assignee)

Comment 4

2 years ago
Although I'm still unable to cause this, I'm pretty certain that its due to the refcount issue spelled out in the patch.  I've commented on the places where we use the APIs that change the IAudioSessionEvents refcount so that its more clear what's happening.  At the very least, the KungFu grip shouldn't harm anything so its worth a shot.

Tests:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f098c7d2eb4835459d62bf8a74a164437ca6880
Attachment #8913430 - Flags: review?(jmathies)
(Assignee)

Updated

2 years ago
Assignee: nobody → davidp99

Updated

2 years ago
Attachment #8913430 - Flags: review?(jmathies) → review+

Updated

2 years ago
Keywords: checkin-needed
status-firefox54: fix-optional → wontfix
status-firefox58: --- → affected
Keywords: stale-bug

Comment 5

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b80368bd2236
Hold Windows AudioSession object while recycling registration. r=jimm
Keywords: checkin-needed

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b80368bd2236
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Please request Beta & ESR52 approval on this when you get a chance.
Flags: needinfo?(davidp99)
Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

Approval Request Comment
[Feature/Bug causing the regression]:
long standing crash issue with native audio api interfacing.
[User impact if declined]:
crashy browser
[Is this code covered by automated tests?]:
no
[Has the fix been verified in Nightly?]:
yes - on crash stats, crashes stop on the 2nd.
[Needs manual test from QE? If yes, steps to reproduce]: 
no
[List of other uplifts needed for the feature/fix]:
none
[Is the change risky?]:
no
[Why is the change risky/not risky?]:
well understood code
[String changes made/needed]:
none
Attachment #8913430 - Flags: approval-mozilla-beta?
Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
crashy browser
Fix Landed on Version:
58, 57 soon
Risk to taking this patch (and alternatives if risky): 
pretty low, no issues have shown up from the fix thus far and the change is well understood. crash is low volume though, so this isn't critical.
String or UUID changes made by this patch: 
none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8913430 - Flags: approval-mozilla-esr52?

Updated

2 years ago
Flags: needinfo?(davidp99)
Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

This is not a new crash and neither is it a top crasher on beta57 or release56. I was not able to confirm whether this helps as I didn't see any crash reports from 58. We are getting too many beta uplifts and trying to minimize churn by only taking fixes for severe recent regressions/crashes/perf issues introduced by new features in 57. Recommend letting this one ride the 58 train.
Attachment #8913430 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox57: affected → wontfix

Updated

2 years ago
Whiteboard: tpi:+, win7only[tbird topcrash] → tpi:+, win7only[tbird crash]
Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

Does match the esr criteria (sec crit/high only)
Attachment #8913430 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
FYI - this was turned down for uplift - but this should have been a sec-critical or at least high, with a TON of EXEC wildptr crashes.  I'll note that none of e5e5, but these may not be going through our jemalloc impl, and so may not get poisoned.

In fact, almost all of these are EXEC except for the odd ILLEGAL or PRIV, but there are some READ and WRITEs

At minimum it should be taken for ESR52.

I suggest re-nominating with the new info (jimm)
Group: core-security
status-firefox57: wontfix → affected
Flags: needinfo?(rkothari)
Flags: needinfo?(jmathies)
Keywords: csectype-uaf, sec-critical
This is crash #54 and #158 in the content process and crash #135+#170 in the browser process (#101 overall in 56.0) so it can make a reasonable "common crash" claim. It's a safe kungFuDeathGrip that can't introduce new crashes: worst case would be leaks, but in this case it's local to a function so that's not an issue either.
tracking-firefox57: --- → ?
tracking-firefox-esr52: --- → 57+
Thanks for the ping Randell. I chatted with Dan and given the sec rating + low risk, we'll take it in 57.
tracking-firefox57: ? → +
Flags: needinfo?(rkothari)
I don't know why this was tagged as a "regression": there are crashes going back to Firefox 12.
Keywords: regression
Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

sec-approval=dveditz
Attachment #8913430 - Flags: sec-approval+
Keywords: sec-critical → sec-high
Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

Sec-approved, Beta57+, ESR52.5+
Attachment #8913430 - Flags: approval-mozilla-esr52-
Attachment #8913430 - Flags: approval-mozilla-esr52+
Attachment #8913430 - Flags: approval-mozilla-beta-
Attachment #8913430 - Flags: approval-mozilla-beta+
Group: core-security → core-security-release
https://hg.mozilla.org/releases/mozilla-esr52/rev/865f7e9b208d
status-firefox-esr52: affected → fixed
status-thunderbird_esr52: affected → fixed
Flags: needinfo?(jmathies)
Whiteboard: tpi:+, win7only[tbird crash] → tpi:+, win7only[tbird crash][adv-main57+][adv-esr52.5+]
Flags: qe-verify-
Whiteboard: tpi:+, win7only[tbird crash][adv-main57+][adv-esr52.5+] → tpi:+, win7only[tbird crash][adv-main57+][adv-esr52.5+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.