1436972 - Properly maintain ref count of AudioSession when changing audio devices

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Julien Cristau [:jcristau] (back August 18)]

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.

Comment 1

[[:philipp]]

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

Comment 2

[Marcia Knous [:marcia]]

Many of the URLs appear to be video chats and webcams.

Comment 3

[Jim Mathies [:jimm]]

Hey David, can you poke at this? Another audio interface issue,s this time with plugins.

Comment 4

[David Parks [:handyman]]

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

Comment 5

[David Parks [:handyman]]

Attachment: Properly maintain kung-fu grip on AudioSession while reStarting

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

Comment 6

[David Parks [:handyman]]

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fbc30501e6a11973011a56ede466bd5276afe69

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80a4001d266
Hold ref counted AudioSession for proper lifetime when re-initializing. r=jimm

Comment 8

[Stefan Hindli [:stefan_hindli]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Looks like this is still happening on Nightly, too :(

Comment 11

[Calixte Denizet (:calixte)]

There still are 49 crashes in nightly 61 with buildid >= 20180325100345.

Comment 12

[David Parks [:handyman]]

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.

Comment 13

[David Parks [:handyman]]

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.

Comment 14

[David Parks [:handyman]]

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.

Comment 15

[David Parks [:handyman]]

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

Comment 16

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 8961500 [details] [diff] [review]
Properly maintain kung-fu grip on AudioSession while reStarting

plugin-related crash fix, approved for 60.0b10

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4c10eb56d460

Comment 18

[[:philipp]]

hi, the patch landed in 60.0b10, but the crashes are continuing:
https://crash-stats.mozilla.com/signature/?build_id=>%3D20180404171943&signature=CLockedList<T>%3A%3AForEachEntry#reports

should we reopen this bug or file a new one?

Comment 19

[[:philipp]]

sorry, didn't catch your comment #14