Closed Bug 1268233 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 + wontfix
firefox48 + fixed
firefox49 + fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Keywords: crash, Whiteboard: btpp-active)

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-bc79325b-82a8-4cb7-8271-4ba952160427.
=============================================================
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
Attachment #8746217 - Flags: review?(aklotz)
Hmm, I guess I didn't need to move mAudioSessionControl = nullptr. :) I'll touch that up locally.
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.
Attachment #8746217 - Flags: review?(aklotz)
Ok, here we go. We still call StopInternal to clear the pointer, but only unregister if mState indicates registration has occurred and succeeded.
Attachment #8746217 - Attachment is obsolete: true
Attachment #8746222 - Flags: review?(aklotz)
Poking through crashstats, it looks like bug 910813 is the same problem, although that bug happens when an audio device disconnects.
Blocks: 910813
Attachment #8746222 - Flags: review?(aklotz) → review+
Keywords: checkin-needed
Whiteboard: btpp-active
https://hg.mozilla.org/mozilla-central/rev/0006e319e776
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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
Attachment #8746222 - Flags: approval-mozilla-aurora?
Sounds like it may be good to uplift as far as beta, let's see how it does on aurora first though.
Comment on attachment 8746222 [details] [diff] [review]
avoid unregistering before registering

Crash fix, let's uplift and verify that it goes away on aurora.
Attachment #8746222 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Jim, this one is very low volume on Beta47. Is it still worth uplifting or should we just wontfix it? Thanks!
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
See Also: → 1295917
You need to log in before you can comment on or make changes to this bug.