1213154 - tab-sound-icon does not display after back/forward navigation

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

Steps to reproduce:
1. Open https://videos.cdn.mozilla.net/uploads/mozillaorg/
2. Click "A different kind of browser.webm" link 
   --- observe, tab-sound-icon display as expected
3. Click navigation back button in location bar
4. Click navigation forward button in location bar
--- observe, tab-sound-icon display or not

Actual results.
tab-sound-icon does not display

Expected results:
tab-sound-icon dhould display

Comment 1

[(no longer active)]

Andrea, can you take a look, please?  Thanks!

Comment 2

[Andrea Marchesini [:baku]]

I'm not able to reproduce this issue. Following the STR from the description, I see the icon when I load the video for the first time, then going back the icon disappears, but clicking the forward button, the video and the icon appear again.

Comment 3

[Alice0775 White]

(In reply to Andrea Marchesini (:baku) from comment #2)
> I'm not able to reproduce this issue. Following the STR from the
> description, I see the icon when I load the video for the first time, then
> going back the icon disappears, but clicking the forward button, the video
> and the icon appear again.


You should wait to stop buffering, then back and forward

Comment 4

[Andrea Marchesini [:baku]]

Attachment: bfcache.patch

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8673654 [details] [diff] [review]
bfcache.patch

Review of attachment 8673654 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +4726,5 @@
>    bool playingThroughTheAudioChannel =
>       (!mPaused &&
>        !Muted() &&
> +      // We should consider any bfcached page as non-playing.
> +      !mEventDeliveryPaused &&

Shouldn't we check mPausedForInactiveDocumentOrChannel instead of this? That gets set to true for an inactive document (or channel), and bfcached documents are inactive.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: bfcache.patch

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97e9ac5785f5

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffaa49646d0e

Comment 9

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=15752456&repo=mozilla-inbound

Comment 10

[Andrea Marchesini [:baku]]

Attachment: bfcache.patch

Ehsan, can you double check the changes in the test?

Comment 11

[(no longer active)]

What kind of notification are you getting here, and is it "active" or "inactive"?  I'd like to understand why this is changing here...

Note that test_swapped_browser() is called two times, once when the audio element is playing and once when it's not, so I suspect you should be seeing different values in the two cases.

Comment 12

[Andrea Marchesini [:baku]]

With this patch we receive a notification when we the document is marked as inactive. So we should receive inactive when the document is not visible (replaced by something else and in bfcache), and active when it's shown again.

Comment 13

[(no longer active)]

Comment on attachment 8674884 [details] [diff] [review]
bfcache.patch

Review of attachment 8674884 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below fixed.

::: browser/base/content/test/general/browser_audioTabIcon.js
@@ +192,5 @@
>             event.detail.changed.indexOf("muted") >= 0;
>    });
>    let AudioPlaybackPromise = new Promise(resolve => {
>      let observer = (subject, topic, data) => {
> +      ok(true, "Should see an audio-playback notification");

OK, so instead of doing this, please compare |data| against "active" or "inactive" based on the isPlaying argument.  Also there is a comment a few lines below when yielding on AudioPlaybackPromise that you need to update as well.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: bfcache.patch

Comment 15

[Andrea Marchesini [:baku]]

Attachment: bfcache.patch

I cannot do what Ehsan is suggesting because the notification is received by the window who contains the 2 tabs. So, swapping tab I receive 2 notifications and that doesn't allow me to compare the state with isPlaying.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/780061f96448

Comment 17

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d1d71aba0fbc - one of this change, this one or the other caused a perma failure like https://treeherder.mozilla.org/logviewer.html#?job_id=15945937&repo=mozilla-inbound

since i was unable to determine which of this 2 changes caused this i had to backout both to get the treen green again.

Comment 18

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8ab3ab2de5

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This also broke gij(10) and gij(34): https://treeherder.mozilla.org/logviewer.html#?job_id=15959893&repo=mozilla-inbound

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81b802b69a74

Comment 21

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8c423cd24085 for Linux (https://treeherder.mozilla.org/logviewer.html#?job_id=16610043&repo=mozilla-inbound) and Mulet (https://treeherder.mozilla.org/logviewer.html#?job_id=16611694&repo=mozilla-inbound) webgl reftest failures, and Mulet Gij failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16611680&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=16611661&repo=mozilla-inbound.

Comment 22

[Phil Ringnalda (:philor)]

And one more: the push right before you enabled a bunch of tests on e10s runs, including browser_audioTabIcon.js, and you apparently made it racy enough that on slow runs it failed like https://treeherder.mozilla.org/logviewer.html#?job_id=16621574&repo=mozilla-inbound around 10% of the time on Linux32 debug and 25% of the time on ASan.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: bfcache.patch

mtseng, sorry for changing bug, but this is exactly the same issue we have with reftests in the other bug. As you can see, I changed the code, but enabling/disabling the pref in reftests.ini I break all the tests.

Do you have a time to give me help with it?
Thanks!

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I think the problem is not the same. The problem here is actually default-preferences in reftest.list can be setting only once. If you write several lines of default-preferences, previous default-preferences will be ignored. I change the patch and see if it works. Here is try result.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7c444fa360

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/786bc35f327a

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this and bug 1207546 out in https://hg.mozilla.org/integration/mozilla-inbound/rev/16002f5829fb for gij bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=16930542&repo=mozilla-inbound

Comment 27

[eyal gruss]

wfm on ff42

Comment 28

[Andrea Marchesini [:baku]]

It's still broken, but I have to fix a gaia marionette test and it's hard to set up the correct environment to run it locally.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: interdiff

Finally I found the time to set up a local environment for mulet + marionette and I wrote this interdiff.

roc, we cannot use mPausedForInactiveDocumentOrChannel in IsPlayingThroughTheAudioChannel because that boolean is set to true when we mute the element by the audiochannel. That means that from that moment it will be muted forever. The real check we want to do is just about bfcache. I used IsActive() for that.

I also added a couple of calls to UpdateAudioChannelPlayingState() when we touch a flag that can change the status of IsPlayingThroughTheAudioChannel.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a96f1eaabfce

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a96f1eaabfce

Comment 32

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac 0SX 10.9.5 using Firefox 45 Beta 2 (buildID: 20160201143558): tab-sound-icon correctly displayed after back/forward navigation.