1339259 - Crash in mozilla::widget::AudioSession::OnSessionDisconnectedInternal

Status: RESOLVED FIXED

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

Description

[[:philipp]]

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

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Jim, any thoughts on who might be able to investigate?

Comment 2

[Jim Mathies [:jimm]]

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.

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

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 '--'.

Comment 4

[David Parks [:handyman]]

Attachment: Hold AudioSession with KungFuGrip when cycling its registration

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b80368bd2236
Hold Windows AudioSession object while recycling registration. r=jimm

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please request Beta & ESR52 approval on this when you get a chance.

Comment 8

[Jim Mathies [:jimm]]

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

Comment 9

[Jim Mathies [:jimm]]

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.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

Does match the esr criteria (sec crit/high only)

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 13

[Daniel Veditz [:dveditz]]

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.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks for the ping Randell. I chatted with Dan and given the sec rating + low risk, we'll take it in 57.

Comment 15

[Daniel Veditz [:dveditz]]

I don't know why this was tagged as a "regression": there are crashes going back to Firefox 12.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

sec-approval=dveditz

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8913430 [details] [diff] [review]
Hold AudioSession with KungFuGrip when cycling its registration

Sec-approved, Beta57+, ESR52.5+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37d735e5ed3b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/865f7e9b208d