Closed Bug 1339259 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Widget: Win32, defect, P1)

52 Branch
x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
thunderbird_esr52 --- fixed
firefox51 --- unaffected
firefox52 --- wontfix
firefox-esr52 57+ fixed
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: philipp, Assigned: handyman)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: tpi:+, win7only[tbird crash][adv-main57+][adv-esr52.5+][post-critsmash-triage])

Crash Data

Attachments

(1 file)

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
See Also: → 1338504
Crash Signature: [@ mozilla::widget::AudioSession::OnSessionDisconnectedInternal] → [@ mozilla::widget::AudioSession::OnSessionDisconnectedInternal] [@0x0 | mozilla::widget::AudioSession::OnSessionDisconnectedInternal]
Priority: P2 → P1
Blocks: 731812
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
Blocks: 1394024
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: nobody → davidp99
Attachment #8913430 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/b80368bd2236
Status: NEW → RESOLVED
Closed: 3 years ago
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?
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-
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
Flags: needinfo?(rkothari)
Flags: needinfo?(jmathies)
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.
Thanks for the ping Randell. I chatted with Dan and given the sec rating + low risk, we'll take it in 57.
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+
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
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.