Closed Bug 1387058 Opened 7 years ago Closed 7 years ago

Crash in mozilla::audio::AudioNotificationReceiver::NotifyDefaultDeviceChanged

Categories

(Core :: Audio/Video: Playback, defect, P1)

Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: marcia, Assigned: chunmin)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-9e4d2982-1df2-4c90-b9aa-d14360170803.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170803100352

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=52285ea5e54c73d3ed824544cef2ee3f195f05e6&tochange=63e261ce8cb04c913d2e6b19ea451b7078d24dc1

Possibly bug 1361336? ni :chunmin
Flags: needinfo?(cchang)
Priority: -- → P1
Crash Signature: [@ mozilla::audio::AudioNotificationReceiver::NotifyDefaultDeviceChanged] → [@ mozilla::audio::AudioNotificationReceiver::NotifyDefaultDeviceChanged] [@ mozilla::dom::ContentChild::RecvAudioDefaultDeviceChange]
I hit this unplugging a USB hub from my laptop.
I can replicate this bug by changing my active audio device through Windows sound settings. The active tab crashes after the change.
Assignee: nobody → cchang
Flags: needinfo?(cchang)
Like :mismith said in bug 1387297 comment 0, it can be reproduced by changing the default device when there is no AudioStream. The crash is caused by dereference the sSubscribers since sSubscribers is nullptr then[0].

[0] http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/dom/media/AudioNotificationReceiver.cpp#67
Comment on attachment 8893688 [details]
Bug 1387058 - part2: Early return in AudioNotificationReceiver::NotifyDefaultDeviceChanged if there is no AudioStream;

https://reviewboard.mozilla.org/r/164804/#review170194

::: dom/media/AudioNotificationReceiver.cpp:71
(Diff revision 2)
>  AudioNotificationReceiver::NotifyDefaultDeviceChanged()
>  {
>    MOZ_ASSERT(XRE_IsContentProcess());
>  
>    StaticMutexAutoLock lock(sMutex);
> +  

space.
Attachment #8893688 - Flags: review?(jwwang) → review+
Comment on attachment 8893687 [details]
Bug 1387058 - part1: Clear the static pointer when no more AudioStreams;

https://reviewboard.mozilla.org/r/164802/#review170196
Attachment #8893687 - Flags: review?(jwwang) → review+
The signature "mozilla::audio::AudioNotificationReceiver::NotifyDefaultDeviceChanged" is ranked #2 in content topcrashes.
Keywords: topcrash
Keywords: checkin-needed
Submitted an autoland request to move fast.
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/357a94f6484f
part1: Clear the static pointer when no more AudioStreams; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/cde7d3637eee
part2: Early return in AudioNotificationReceiver::NotifyDefaultDeviceChanged if there is no AudioStream; r=jwwang
This is Windows topcrash #1 by a wide margin, in the nightly of 20170803134456.
Could you please retriger a nightly once it landed? Thanks
Flags: needinfo?(ryanvm)
(In reply to Sylvestre Ledru [:sylvestre] from comment #15)
> Could you please retriger a nightly once it landed? Thanks

Triggered.
Flags: needinfo?(ryanvm)
Verified fixed in Build 20170805100334.
Status: RESOLVED → VERIFIED
Would it be possible to create a test to catch this type of issue in the future? This signature ended up quite high, in the range of 3188 installs, but almost 15K crashes in 7 days.
Flags: needinfo?(cchang)
For now, there is no way to change the audio devices in our tests. We already plan to do that(Bug 1360165) later to extend our test coverage.
Flags: needinfo?(cchang)
Could we add manual QA testing for this kind of "hardware change" when smoke testing future releases?
We are reviewing SV tests and will ask them to add this kind of test.
Depends on: 1387537
Marking this affected for 56 since it is a regression from bug 1361336 and we are about to uplift patches from that bug for beta 11.
Comment on attachment 8893687 [details]
Bug 1387058 - part1: Clear the static pointer when no more AudioStreams;

This bug is needed to properly uplift bug 1361336. This comment applies to both patches in this bug (both are needed to properly uplift).

Approval Request Comment
[Feature/Bug causing the regression]: Higher sandbox level in 55 on Windows
[User impact if declined]: Crash if bug 1361336 is uplifted.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, crash is now gone from crash-stats.
[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?]: The patches are extremely simple, the cause and the fix for this crash are well understood, it's been in Nightly for some time, no complaints.
[String changes made/needed]: None.
Attachment #8893687 - Flags: approval-mozilla-beta?
Comment on attachment 8893687 [details]
Bug 1387058 - part1: Clear the static pointer when no more AudioStreams;

Both patches please for uplift to beta, to help with audio issues fixed in bug 1361336.  These should land for beta 11.
Attachment #8893687 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.