Closed Bug 1347758 Opened 7 years ago Closed 7 years ago

HTML5 video/audio doesn't play if node was removed in background tab

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
55 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed
firefox55 --- verified

People

(Reporter: 684sigma, Assigned: alwu)

References

Details

(Keywords: regression)

Attachments

(8 files, 2 obsolete files)

video doesn't play, by reporter.html
437 bytes, text/html
Details
Bug 1347758 - part1 : window should know whehter there is any alive media component and decide to resume the tab or not.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1347758 - part2 : remove add/removeMediaContent() and related codes.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1347758 - part3 : should notify media block for audible agent in the first time.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1347758 - part4 : add audio channel log.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1347758 - part5 : add tests.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1347758 - part6 : reorder tests according to alphabet order.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1347758 - let outer window decides when need to resume the tab. (aurora)
42.07 KB, patch
Details | Diff | Splinter Review 
I have a problem with Firefox Nightly 55. It doesn't happen in Firefox Beta 53.
Sometimes when I click play button on a web page, audio doesn't play. I don't remember where I saw it (probably on Soundcloud or Facebook) because I immediately created working html page that reproduces the bug.
I noticed one specific scenario when it happens

1. Open attached html page in background tab
2. When that tab displays title "removed", switch to that tab and click "Play" button on the page

Result: Nothing happens
Expected: Audio should play
Has STR: --- → yes
Keywords: regression
Hi Kaku,
Please take this bug.
Flags: needinfo?(kaku)
The issue belongs to the block-background-audio feature, ni Alastor for more information.
Flags: needinfo?(alwu)
Assignee: nobody → alwu
Blocks: delay-autoplay
Flags: needinfo?(kaku)
Flags: needinfo?(alwu)
Priority: -- → P1
Status: UNCONFIRMED → NEW
Ever confirmed: true
Writing tests...
Attachment #8848000 - Flags: review?(amarchesini)
Attachment #8849448 - Flags: review?(amarchesini)
Attachment #8849449 - Flags: review?(amarchesini)
Attachment #8849450 - Flags: review?(amarchesini)
Attachment #8849884 - Flags: review?(amarchesini)
Attachment #8849885 - Flags: review?(amarchesini)
Attachment #8848000 - Flags: review?(amarchesini) → review?(ehsan)
Attachment #8849448 - Flags: review?(amarchesini) → review?(ehsan)
Attachment #8849449 - Flags: review?(amarchesini) → review?(ehsan)
Attachment #8849450 - Flags: review?(amarchesini) → review?(ehsan)
Attachment #8849884 - Flags: review?(amarchesini) → review?(ehsan)
Attachment #8849885 - Flags: review?(amarchesini) → review?(ehsan)
Hi, Ehsan,
Since baku said he's busy with fixing blockers for multi-e10s, could you help me review these patches?
Thanks!
I'll try to review tomorrow, sorry for the delay!
(In reply to :Ehsan Akhgari (busy) from comment #27)
> I'll try to review tomorrow, sorry for the delay!

Thank you very much!
Comment on attachment 8848000 [details]
Bug 1347758 - part1 : window should know whehter there is any alive media component and decide to resume the tab or not.

https://reviewboard.mozilla.org/r/120956/#review125560

::: dom/base/nsGlobalWindow.cpp:4254
(Diff revision 3)
>    nsCOMPtr<nsPIDOMWindowInner> inner = GetCurrentInnerWindow();
>    if (!inner) {
>      return;
>    }
>  
> +  // If the document is still in the background, don't need to resume it.

nsIDocument::Hidden() is technically not the same thing as the doc being in the background, it also precludes the doc being prerendered.  If you reword the comment to say the doc is not visible, that would be more accurate.

::: dom/base/nsPIDOMWindow.h:743
(Diff revision 3)
>  
>    mozilla::dom::LargeAllocStatus mLargeAllocStatus; // Outer window only
> +
> +  // When there is any created alive media component, we can consider to resume
> +  // the media content in the window.
> +  bool mShouldActiveMediaComponents;

Let's rename this to something slightly better, like mShouldResumeOnFirstActiveMediaComponent.
Attachment #8848000 - Flags: review?(ehsan) → review+
Comment on attachment 8849448 [details]
Bug 1347758 - part2 : remove add/removeMediaContent() and related codes.

https://reviewboard.mozilla.org/r/122210/#review125562
Attachment #8849448 - Flags: review?(ehsan) → review+
Comment on attachment 8849449 [details]
Bug 1347758 - part3 : should notify media block for audible agent in the first time.

https://reviewboard.mozilla.org/r/122212/#review125576
Attachment #8849449 - Flags: review?(ehsan) → review+
Comment on attachment 8849450 [details]
Bug 1347758 - part4 : add audio channel log.

https://reviewboard.mozilla.org/r/122214/#review125580

::: dom/media/webaudio/AudioDestinationNode.cpp:512
(Diff revision 2)
>      return NS_OK;
>    }
>  
> +  MOZ_LOG(AudioChannelService::GetAudioChannelLog(), LogLevel::Debug,
> +         ("AudioDestinationNode, WindowVolumeChanged, "
> +          "this = %p, aVolume = %f, aMuted = %d\n", this, aVolume, aMuted));

Nit: because of the %d, please cast the aMuted to int here.

::: dom/media/webaudio/AudioDestinationNode.cpp:533
(Diff revision 2)
>      return NS_OK;
>    }
>  
> +  MOZ_LOG(AudioChannelService::GetAudioChannelLog(), LogLevel::Debug,
> +         ("AudioDestinationNode, WindowSuspendChanged, "
> +          "this = %p, aSuspend = %d\n", this, aSuspend));

Similarly here.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp:1743
(Diff revision 2)
>  NS_IMETHODIMP
>  nsNPAPIPluginInstance::WindowVolumeChanged(float aVolume, bool aMuted)
>  {
> +  MOZ_LOG(AudioChannelService::GetAudioChannelLog(), LogLevel::Debug,
> +         ("nsNPAPIPluginInstance, WindowVolumeChanged, "
> +          "this = %p, aVolume = %f, aMuted = %d\n", this, aVolume, aMuted));

And here.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp:1766
(Diff revision 2)
>  NS_IMETHODIMP
>  nsNPAPIPluginInstance::WindowSuspendChanged(nsSuspendedTypes aSuspend)
>  {
> +  MOZ_LOG(AudioChannelService::GetAudioChannelLog(), LogLevel::Debug,
> +         ("nsNPAPIPluginInstance, WindowSuspendChanged, "
> +          "this = %p, aSuspend = %d\n", this, aSuspend));

Same.
Attachment #8849450 - Flags: review?(ehsan) → review+
Comment on attachment 8849884 [details]
Bug 1347758 - part5 : add tests.

https://reviewboard.mozilla.org/r/122604/#review125582

::: toolkit/content/tests/browser/browser_block_notInTreeAudio.js:41
(Diff revision 2)
> +  content.document.body.removeChild(audio);
> +  // Add timeout to ensure the audio is removed from DOM tree.
> +  setTimeout(function() {
> +    info("Prepare to start playing audio.");
> +    audio.play();
> +  }, 2000);

Why do we need to wait for 2 seconds here?  Can we do something better here?  r+ becuase I don't want to block the landing of this patch on this, but please file a follow-up to make this better.
Attachment #8849884 - Flags: review?(ehsan) → review+
Comment on attachment 8849885 [details]
Bug 1347758 - part6 : reorder tests according to alphabet order.

https://reviewboard.mozilla.org/r/122624/#review125592

r+ based on the commit message.  :-)
Attachment #8849885 - Flags: review?(ehsan) → review+
Comment on attachment 8849450 [details]
Bug 1347758 - part4 : add audio channel log.

https://reviewboard.mozilla.org/r/122214/#review125580

> Nit: because of the %d, please cast the aMuted to int here.

I'll change it to %s and print "true" or "false" according to the value of boolean.
Comment on attachment 8849884 [details]
Bug 1347758 - part5 : add tests.

https://reviewboard.mozilla.org/r/122604/#review125582

> Why do we need to wait for 2 seconds here?  Can we do something better here?  r+ becuase I don't want to block the landing of this patch on this, but please file a follow-up to make this better.

Because media element would send async runnable to pause itself when it was unbind from tree [1].

The ideal way is split "unbind from tree" and "play not-in-tree media element" in two different tasks, but I have no way to access the not-in-tree media element in the different task. (can't get that element from document)

Therefore, I need to wait for a while to ensure pause() has been called, before I call play(). If you think 2 seconds is too long, I can change it to 1 second.

[1] http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/dom/html/HTMLMediaElement.cpp#4557
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/85846edfe957
part1 : window should know whehter there is any alive media component and decide to resume the tab or not. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/cafb3c12027b
part2 : remove add/removeMediaContent() and related codes. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/1fd084ec34d4
part3 : should notify media block for audible agent in the first time. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/80232d4c85d5
part4 : add audio channel log. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/2abce19f5001
part5 : add tests. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/9becd55242c1
part6 : reorder tests according to alphabet order. r=Ehsan
Add NI as a reminder for uplift.
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de28756d3b2b
part1 : window should know whehter there is any alive media component and decide to resume the tab or not. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/c8cb2fcd9f01
part2 : remove add/removeMediaContent() and related codes. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/3e7df5c0637c
part3 : should notify media block for audible agent in the first time. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/e373c012833a
part4 : add audio channel log. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/2321624fb1b2
part5 : add tests. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/04911f2670f1
part6 : reorder tests according to alphabet order. r=Ehsan
Flags: needinfo?(alwu)
Approval Request Comment
[Feature/Bug causing the regression]: This regression is caused by bug1338137.
[User impact if declined]: Affect lots of website using webAudio/Flash plugIn, the video would not have audio after it was resumed.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see the duplicated bugs for the details, most of those bugs have the STR.
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Because the most risky thing is keep this regression alive, and this changes was passed all previous test cases (and the test I added in this patch)
[String changes made/needed]: No
Flags: needinfo?(alwu)
Attachment #8851454 - Flags: approval-mozilla-aurora?
I have verified that this issue is fixed (the video has audio after focusing an opened tab) in the latest Nightly 55.0a1 using macOS 10.11.6, Ubuntu 16.04 64bit, Windows 10 64bit and Windows 7 64bit.
status-firefox55: fixed → verified
Comment on attachment 8851454 [details] [diff] [review]
Bug 1347758 - let outer window decides when need to resume the tab. (aurora)

Fix a regression caused by block-background-audio. Aurora54+.
Attachment #8851454 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Aurora patch needs rebasing.
Flags: needinfo?(alwu)
Rebase.
Attachment #8851454 - Attachment is obsolete: true
Flags: needinfo?(alwu)
Comment on attachment 8854679 [details] [diff] [review]
Bug 1347758 - let outer window decides when need to resume the tab. (aurora)

Rebase the patch in comment55.
Attachment #8854679 - Flags: approval-mozilla-aurora?
Attachment #8854679 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
patching file dom/base/nsDocument.cpp
Hunk #1 FAILED at 1331
1 out of 4 hunks FAILED -- saving rejects to file dom/base/nsDocument.cpp.rej
patching file dom/base/nsIDocument.h
Hunk #2 FAILED at 3167
1 out of 2 hunks FAILED -- saving rejects to file dom/base/nsIDocument.h.rej
patching file dom/html/HTMLMediaElement.cpp
Hunk #3 FAILED at 877
1 out of 5 hunks FAILED -- saving rejects to file dom/html/HTMLMediaElement.cpp.rej
abort: patch failed to apply

Your parent rev is from Wed, 22 Mar 2017. Can you please make sure you're rebasing on top of the tip revision in the future please? It would save us going around in circles like this.
Flags: needinfo?(alwu)
Ah, sorry, It seems I uploaded the wrong patch.
Flags: needinfo?(alwu)
Rebase the patch in comment55.
Attachment #8854679 - Attachment is obsolete: true
Attachment #8855142 - Flags: approval-mozilla-aurora?
Comment on attachment 8855142 [details] [diff] [review]
Bug 1347758 - let outer window decides when need to resume the tab. (aurora)

You can carry over the previous approval if you're just rebasing.
Attachment #8855142 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: