1325253 - Crash in mozilla::dom::HTMLMediaElement::NotifyAudioPlaybackChanged

Status: RESOLVED FIXED

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

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug was filed from the Socorro interface and is 
report bp-c23b0fb0-bdb7-4dea-ac0b-f3d262161221.
=============================================================

There has been a large number of this crash starting with the 12-20 Aurora build (the previous bug, bug 1315521, was closed a month ago). Alastor, do you think this is a regression from bug 1320005, which was uplifted to Aurora in the 12-20 build?

Comment 1

[Alastor Wu [:alwu]]

Yes, it's a regression from bug 1320005, and I'm debugging now.

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1325253 - notify audible state only when agent exists. (aurora)

Hi, Baku,
Could you help me review this patch?
This patch is for aurora-only, because this crash doesn't happen on present Nightly after bug1309162.
Thanks!

Comment 3

[Alastor Wu [:alwu]]

Comment on attachment 8821047 [details] [diff] [review]
Bug 1325253 - notify audible state only when agent exists. (aurora)

># HG changeset patch
># User Alastor Wu <[e-mail]>
># Date 1482392923 -28800
>#      Thu Dec 22 15:48:43 2016 +0800
># Node ID 4998c4ef859b1128471bd15e9384d127b5ef9654
># Parent  40e863f1e8180487fa1011ed6ac807a30905d617
>Bug 1325253 - notify audible state only when agent exists. (aurora)
>
>The old logic is wrong, it would reach the line NotifyStartedAudible() when
>mAudioChannelAgent is nullptr.
>
>diff --git a/dom/html/HTMLMediaElement.cpp b/dom/html/HTMLMediaElement.cpp
>--- a/dom/html/HTMLMediaElement.cpp
>+++ b/dom/html/HTMLMediaElement.cpp
>@@ -6576,27 +6576,25 @@ HTMLMediaElement::SetAudibleState(bool a
>     NotifyAudioPlaybackChanged(
>       AudioChannelService::AudibleChangedReasons::eDataAudibleChanged);
>   }
> }
> 
> void
> HTMLMediaElement::NotifyAudioPlaybackChanged(AudibleChangedReasons aReason)
> {
>-  if (MaybeCreateAudioChannelAgent() &&
>-      !mAudioChannelAgent->IsPlayingStarted()) {
>-    return;
>-  }
>-
>   if (mAudible == IsAudible()) {
>     return;
>   }
> 
>   mAudible = IsAudible();
>-  mAudioChannelAgent->NotifyStartedAudible(mAudible, aReason);
>+
>+  if (mAudioChannelAgent && mAudioChannelAgent->IsPlayingStarted()) {
>+    mAudioChannelAgent->NotifyStartedAudible(mAudible, aReason);
>+  }
> }
> 
> AudibleState
> HTMLMediaElement::IsAudible() const
> {
>   // Muted or the volume should not be ~0
>   if (Muted() || (std::fabs(Volume()) <= 1e-7)) {
>     return AudioChannelService::AudibleState::eNotAudible;

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1325253 - notify audible state only when agent exists. (aurora)

re-upload the patch.

Comment 5

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=131c847b0adb404c06a1a98393192512ed349683

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8821050 [details] [diff] [review]
Bug 1325253 - notify audible state only when agent exists. (aurora)

Approval Request Comment
[Feature/Bug causing the regression]: Fix the crash
[User impact if declined]: Browser would crash
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, it doesn't happen on Nightly, only on Aurora
[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?]: The change is to avoid calling dangling pointer 
[String changes made/needed]: No

Comment 7

[Julien Cristau [:jcristau]]

Comment on attachment 8821050 [details] [diff] [review]
Bug 1325253 - notify audible state only when agent exists. (aurora)

fix media crash/regression in aurora52

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a571b48128eb

Comment 9

[Andrei Vaida [:avaida]]

qe-verify- per Comment 6

Comment 10

[Blake Wu [:bwu][:blakewu]]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #9)
> qe-verify- per Comment 6
Could you elaborate it more? "qe-verify-" is due to comment6?

Comment 11

[Andrei Vaida [:avaida]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #10)
> (In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #9)
> > qe-verify- per Comment 6
> Could you elaborate it more? "qe-verify-" is due to comment6?

The "qe-verify" flag is used to show whether a bug needs manual testing. 

Manual testing is not required for this specific fix, according to Comment 6:

> [Needs manual test from QE? If yes, steps to reproduce]: No

Comment 12

[Blake Wu [:bwu][:blakewu]]

Ah, I see. Thanks.