Properly maintain ref count of AudioSession when changing audio devices

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jcristau, Assigned: handyman)

Tracking

({crash, regression})

Trunk
mozilla61
x86_64
Windows 10
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60+ fixed, firefox61+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is
report bp-cbb91443-6d95-4a6a-a823-c5edd0180208.
=============================================================

Top 8 frames of crashing thread:

0 audioses.dll CLockedList<ATL::CComPtr<IAudioSessionEvents>, 0, 1>::ForEachEntry 
1 audioses.dll CAudioSessionControl::OnAudioSessionEvent 
2 audioses.dll CAudioSessionControl::CAudioSessionNotificationDelegator::OnMediaNotification 
3 mmdevapi.dll CMediaNotifications::OnMediaNotificationWorkerHandler 
4 ntdll.dll TppSimplepExecuteCallback 
5 ntdll.dll TppWorkerThread 
6 kernel32.dll BaseThreadInitThunk 
7 ntdll.dll RtlUserThreadStart 

=============================================================

This looks like a new plugin process crash signature in nightly, starting Feb 6.
this is the pushlog for 60.a1 build 20180206220102 where the crash first showed up on nightly:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f1a4b64f19b0e93c49492735db30a5023e624ae7&tochange=0790ec12200d5f663dd000a6ef3c72c795ca4ead

only 64bit builds on windows 10 seem affected - ja locale builds seem to hit this quite frequently
Keywords: regression
Hardware: Unspecified → x86_64
Many of the URLs appear to be video chats and webcams.
Hey David, can you poke at this? Another audio interface issue,s this time with plugins.
Flags: needinfo?(davidp99)
I suspect that the dates are a bit off and this is due to patches from bug 1358372. It may also be due to the sandbox changes and the fact that Adobe themselves maintain an IAudioSessionEvents object (seen in debugger).
Assignee: nobody → davidp99
Flags: needinfo?(davidp99)
Priority: -- → P2
So the issue fixed by the patch is pretty obvious -- I had introduced the mutex scope without any consideration for the scope of the kung-fu grip.  This fixes that.

What I can't do is tie that directly to this crash signature.  Of course, its Windows internal behavior so its hard to know but this doesn't look like the crash we got when I fixed this race condition the first time.  I'm tying this patch to this crash based largely on the alignment of their dates in the crash history.

(The Windows code around the crash pointed me to a different part of my old patch but I can find no error there so...)
Attachment #8961500 - Flags: review?(jmathies)
Priority: P2 → P1
Attachment #8961500 - Flags: review?(jmathies) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80a4001d266
Hold ref counted AudioSession for proper lifetime when re-initializing. r=jimm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f80a4001d266
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please request Beta approval on this when you get a chance.
Flags: needinfo?(davidp99)
Looks like this is still happening on Nightly, too :(
There still are 49 crashes in nightly 61 with buildid >= 20180325100345.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yeah...

It turns out that changing the audio device doesn't work in plugins, although it doesn't crash for me.  That _feels_ due to a related issue to this one.  It also gave me something I could look at with mozregression... and the result suggests I wasn't even looking at the right bug as cause.  I now think this is due to my hardening of the plugin sandbox in bug 1426733 -- that's the restricting SIDs, not the USER_LIMITED access token strengthening that we did around the same time.

OTOH, the fix in this bug is legit and should be important (we'd probably start seeing crashes after the release).  So I'm going to leave this bug as is and open a new one for the crash.
Flags: needinfo?(davidp99)
See Also: → 1449388
FYI, I still think this should get uplifted but I'm going to hold off a bit while I work on bug 1449388.  I'm feeling gun shy.
Repeating some of the above history:

This bug was originally about the crash in Windows audio that spiked in the plugin process.  The patch in this bug fixes a race condition -- it was initially thought that it might fix the crash but it didn't.  The crash is being handled in bug 1449388.  This bug is now just about the race condition.  We want to get this fix uplifted.
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Summary: Crash in CLockedList<T>::ForEachEntry → Properly maintain ref count of AudioSession when changing audio devices
Comment on attachment 8961500 [details] [diff] [review]
Properly maintain kung-fu grip on AudioSession while reStarting

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1358372 (patch #2)

[User impact if declined]:
We haven't been seeing crashes in nightly/beta but the crash in bug 1339259 should return without it.

[Is this code covered by automated tests?]:
No.  Its an occasional race condition that I've never even personally seen.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
N/A

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Codewise, this just increases the scope of a ref-counted object beyond the spot where it might result in a crash.  It should only make a difference to cases where the ref-count would have dropped to 0, which would've resulted in a crash anyway.

[String changes made/needed]:
None
Attachment #8961500 - Flags: approval-mozilla-beta?
Comment on attachment 8961500 [details] [diff] [review]
Properly maintain kung-fu grip on AudioSession while reStarting

plugin-related crash fix, approved for 60.0b10
Attachment #8961500 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.