1394455 - [e10s] Media is unblocked while hovering an unfocused tab

Status: VERIFIED FIXED

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

Description

[Emil Ghitta, QA [:emilghitta]]

Attachment: Tab hovering.gif

[Affected versions]:
Firefox 57.0a1 (Build Id:20170828100127)

[Affected platforms]:
Windows 10 64bit.
Mac 10.11.6.
Ubuntu 16.04 64bit.

[Steps to reproduce]:
1. Launch Firefox with a clean profile.
2. Access the https://www.youtube.com/ webpage.
3. Open 1 random video in a new tab without focusing it.
4. Hover over the tab.

[Expected result]:
The media is still blocked for that tab since it was not visited nor the Play Tab button was pressed.

[Actual result]:
The media gets unblocked and the Play Tab button dissapears.

[Regression range]:
This is a recent regression:
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2cbd34e59b97a522498c5b5dbd22677935a025e5&tochange=3bbd8e25df3d92464b97bcb42413febd9041af6f

[Additional Information]
Please observe the attached video for further information regarding this issue.

Comment 1

[Emil Ghitta, QA [:emilghitta]]

Hi Mike!

I see that you pushed some patches regarding tabs. I don't know if they are responsible for this behavior.

Can you please have a look into this ?

Thanks a lot!

Comment 2

[Alastor Wu [:alwu]]

I CAN reproduce this issue with enable e10s, but CAN NOT reproduce it once disable e10s.

From the log, it seems the document would be incorrectly changed to visible when the cursor hovering over, so triggering the resume mechanism.

Comment 5

[Julien Cristau [:jcristau]]

moving tracking from duplicate bug 1394118

Comment 6

[Stuart Philp :sphilp]

I'm seeing this not just on hover, but it seems to sometimes trigger playing background tabs when you are navigating tabs (ctrl-tab, ctrl-shift-tab, as well as opening and closing new tabs)

Comment 10

[Bobby Chien]

Alastor, as Julien mentioned this bug is a regression which could impact the release of block-autoply. could you help to see anything we could make the bug moving forward? Thanks.

Comment 11

[Alastor Wu [:alwu]]

As I mentioned in comment2, this issue is caused by incorrect the document's visibility, it should be fixed by front end's solution.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Hello everyone.

I landed the patch that opened this bug. I've been on PTO and today is a holiday. Tomorrow I will be back at work and begin working on this.

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Bobby Chien [:bchien] from comment #10)
> Alastor, as Julien mentioned this bug is a regression which could impact the
> release of block-autoply. could you help to see anything we could make the
> bug moving forward? Thanks.

I wouldn't hold back block-autoplay for this. It needs to be fixed it nonetheless.

Bug 1385453 also likely duplicates the mouse-over restart functionality of background decoder shutdown.

Comment 15

[Tzuhao Kuo [:kaku]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #13)
> (In reply to Bobby Chien [:bchien] from comment #10)
> > Alastor, as Julien mentioned this bug is a regression which could impact the
> > release of block-autoply. could you help to see anything we could make the
> > bug moving forward? Thanks.
> 
> I wouldn't hold back block-autoplay for this. It needs to be fixed it
> nonetheless.
> 
> Bug 1385453 also likely duplicates the mouse-over restart functionality of
> background decoder shutdown.
Which is bug 1274919.

Comment 16

[Caspy7]

Bug 1385453, comment 0 discusses using more than just tab hover for warming up tabs. So it would probably be good to rely on whatever mechanism their code is using (and will in the future).

Comment 17

[Mike Conley (:mconley) (:⚙️)]

I've started studying this. This problem is due to the fact that when warming up the tabs, we set the DocShell active state to true, which updates the visibility state, which is what the media suspending stuff is waiting for to unblock playback.

I suspect this is also causing bug 1395735, since the web-exposed Page Visibility API will also be affected (ie, the page will think it is visible when it is only warmed up).

I am confident that a solution to this will also be a solution to bug 1395735.

I might be able to hook up some infrastructure that will allow us to cause the layers to paint and upload from the content process without actually activating the DocShell, which should avoid these problems. I'm not sure this is possible, but I'm starting there for investigation.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

I'll note that if it's not possible to fix this bug before 57 needs to freeze / uplift, we can disable the warming API via this pref: http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/browser/app/profile/firefox.js#1569

So we have a killswitch - the worst case scenario here is that we put off the performance improvements from bug 1385453 a release until we can get these things fixed.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

I investigated this yesterday and today, and here are my findings.

The reason that the video is playing (and the Page Visibility API is telling the page that the tab is visible) is because we're setting the DocShell to be active. So we have to avoid doing that, and yet still need to get the layers for the page to the compositor.

I talked to billm, and as it turns out, this is actually possible. We need to first make the TabChild PuppetWidget visible, via TabChild::MakeVisible, then activate the PresShell, and then trigger a paint.

That appears to fix the problem, however, it puts us in a kind of confused state now - the way I've hacked up my PoC patch, the tab goes into STATE_LOADING without having set the docShell as active, and then when we select the warmed tab, we transition from STATE_LOADING to STATE_LOADED, and the docShell is never activated.

So we'll need to do some re-organizing here in the async tab switcher to account for this middle-ground warming state.

I really, really don't want to do that so close to uplift. The async tab switcher is pretty fragile code. I think the safest option, given the remaining time, is to disable tab warming for now, and then revisit this plan after 57 uplifts.

I'm pretty bummed out by that - the Telemetry showed that this gave a significant performance boost to tab switching. However, I think that the risks of attempting to jam this optimization in with slapped together regression fixes greatly outweigh the gains, considering what will be required.

So I think I'm going to post a patch disabling tab warming for now, and then file a new bug to change how the warming mechanism works.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1394455 - Disable tab warming until bug 1397426 is fixed. :(

Review commit: https://reviewboard.mozilla.org/r/177012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177012/

Comment 22

[Florian Quèze [:florian]]

Comment on attachment 8905223 [details]
Bug 1394455 - Disable tab warming until bug 1397426 is fixed. :(

https://reviewboard.mozilla.org/r/177012/#review182238

Sad r+.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/515aab24cac4
Disable tab warming until bug 1397426 is fixed. :( r=florian

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/515aab24cac4

Comment 26

[Adrian Florinescu [:aflorinescu]]

Comment 0 steps are not reproducible anymore, since comment 24 landed disabling tab warmup. 
Marking the bug as verified on: 57.0a1 20170910220126  - Windows 7x32, Mac OSX 10.12, Ubuntu 16.04