1268233 - crash in @0x0 | mozilla::widget::AudioSession::StopInternal

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Jim Mathies [:jimm]]

This bug was filed from the Socorro interface and is 
report bp-bc79325b-82a8-4cb7-8271-4ba952160427.
=============================================================

Comment 1

[Jim Mathies [:jimm]]

Attachment: avoid unregistering before registering

This isn't very common but appears easy to avoid - we call StopInternal [1] in multiple places here for unexpected failure prior to registering a session notification callback at the end of Start [2], which tries to unregister for callbacks [3]. Window audio apis don't seem to like this.

[1] https://hg.mozilla.org/releases/mozilla-beta/annotate/5bbf2e7c2fc6/widget/windows/AudioSession.cpp#l269
[2] https://hg.mozilla.org/releases/mozilla-beta/annotate/5bbf2e7c2fc6/widget/windows/AudioSession.cpp#l285
[3] https://hg.mozilla.org/releases/mozilla-beta/annotate/5bbf2e7c2fc6/widget/windows/AudioSession.cpp#l300

Comment 2

[Jim Mathies [:jimm]]

Hmm, I guess I didn't need to move mAudioSessionControl = nullptr. :) I'll touch that up locally.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8746217 [details] [diff] [review]
avoid unregistering before registering

actually this isn't right yet. I still need to set mAudioSessionControl to null on failure here.

Comment 4

[Jim Mathies [:jimm]]

Attachment: avoid unregistering before registering

Ok, here we go. We still call StopInternal to clear the pointer, but only unregister if mState indicates registration has occurred and succeeded.

Comment 5

[Jim Mathies [:jimm]]

Poking through crashstats, it looks like bug 910813 is the same problem, although that bug happens when an audio device disconnects.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0006e319e776

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/0006e319e776

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8746222 [details] [diff] [review]
avoid unregistering before registering

Approval Request Comment
[Feature/regressing bug #]:
original win7 audio code had this bug.
[User impact if declined]:
crashy browser
[Describe test coverage new/current, TreeHerder]:
This landed 4-29, last crash in nightly was 4-27. seems safe to assume this is fixed. Wasn't a high volume crash so I'd like to get it uplifted to aurora for more coverage.
[Risks and why]:
low, well understood changes.
[String/UUID change made/needed]:
none

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Sounds like it may be good to uplift as far as beta, let's see how it does on aurora first though.

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8746222 [details] [diff] [review]
avoid unregistering before registering

Crash fix, let's uplift and verify that it goes away on aurora.

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/2804f926fe77

Comment 12

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

Hi Jim, this one is very low volume on Beta47. Is it still worth uplifting or should we just wontfix it? Thanks!