Closed Bug 1339230 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::AudioChannelAgent::WindowID

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 + fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: alwu)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(5 files)

This bug was filed from the Socorro interface and is 
report bp-3b264f60-07c8-4264-b8d9-8c9772170213.
=============================================================

It seems we get a nullptr mAudioChannelAgent here in spite of the MOZ_ASSERT() assuming we don't:

https://hg.mozilla.org/releases/mozilla-beta/annotate/f0c7307f124e/dom/html/HTMLMediaElement.cpp#l6663

Seems about 50 of these crashes a day.
Flags: needinfo?(alwu)
Component: Audio/Video → Audio/Video: Playback
Assignee: nobody → alwu
Flags: needinfo?(alwu)
Looks like this is the #4 top browser crash on beta for fennec.
Comment on attachment 8839413 [details]
Bug 1339230 - part1 : only need to do audio competing for active agent.

https://reviewboard.mozilla.org/r/114088/#review115868

Can you add a test for this?
Attachment #8839413 - Flags: review?(amarchesini) → review+
Priority: -- → P1
Attachment #8840286 - Flags: review?(amarchesini)
Attachment #8840287 - Flags: review?(amarchesini)
Comment on attachment 8840286 [details]
Bug 1339230 - part2 : should be non-audible when media is suspended.

https://reviewboard.mozilla.org/r/114782/#review116666
Attachment #8840286 - Flags: review?(amarchesini) → review+
Comment on attachment 8840287 [details]
Bug 1339230 - part3 : add test.

https://reviewboard.mozilla.org/r/114784/#review116670

::: dom/audiochannel/AudioChannelService.cpp:174
(Diff revision 2)
>    // Second reason is to reduce the bandwidth, avoiding to play any non-audible
>    // media in background which user doesn't notice about.
>  #ifdef MOZ_WIDGET_ANDROID
>    return true;
>  #else
> -  return false;
> +  return Preferences::GetBool("dom.audiochannel.audioCompeting.allAgents", false);

Use boolVarCache instead
Attachment #8840287 - Flags: review?(amarchesini) → review-
Comment on attachment 8840287 [details]
Bug 1339230 - part3 : add test.

https://reviewboard.mozilla.org/r/114784/#review116674
Attachment #8840287 - Flags: review?(amarchesini) → review+
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf5321028381
part1 : only need to do audio competing for active agent. r=baku
https://hg.mozilla.org/integration/autoland/rev/20809c1055e5
part2 : should be non-audible when media is suspended. r=baku
https://hg.mozilla.org/integration/autoland/rev/541cb234a772
part3 : add test. r=baku
Set NI as reminder for uplift the change.
Flags: needinfo?(alwu)
Approval Request Comment
[Feature/Bug causing the regression]: Fix the crash
[User impact if declined]: Will crash, it seems about 50 of these crashes a day.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: doesn't happen on Nightly
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's about changing some audio competing stuffs, and it's only used on Fennec (other platforms is default off)
[String changes made/needed]: No
Flags: needinfo?(alwu)
Attachment #8842274 - Flags: review+
Attachment #8842274 - Flags: approval-mozilla-beta?
Approval Request Comment
[Feature/Bug causing the regression]: Fix the crash
[User impact if declined]: Might crash, we don't see the crash on Aurora, but lots crashes on Beta
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: doesn't happen on Nightly
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's about changing some audio competing stuffs, and it's only used on Fennec (other platforms is default off)
[String changes made/needed]: No
Attachment #8842276 - Flags: review+
Attachment #8842276 - Flags: approval-mozilla-aurora?
Attachment #8842274 - Attachment is patch: true
Attachment #8842276 - Attachment is patch: true
Probably too late to get into Fx52 at this point, but definitely want to keep it on the radar for ESR52 still.
Whoopsie, I missed that this was Android-only.
Comment on attachment 8842274 [details] [diff] [review]
bug1339230 - only need to do audio competing for active agent. r=baku. (for-beta)

this crash looks bad enough in beta that I think we should take this still, even if it's really late.
Attachment #8842274 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8842276 [details] [diff] [review]
bug1339230 - only need to do audio competing for active agent. r=baku. (for-aurora)

fennec crash fix for aurora53
Attachment #8842276 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.