Closed Bug 1394455 Opened 7 years ago Closed 7 years ago

[e10s] Media is unblocked while hovering an unfocused tab

Categories

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

57 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla57
Iteration:
57.3 - Sep 19
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + verified

People

(Reporter: emilghitta, Assigned: mconley)

References

Details

(Keywords: regression, Whiteboard: [photon-performance] [testcoverage])

Attachments

(2 files)

Attached image 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.
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!
Flags: needinfo?(mconley)
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.
Summary: Media is unblocked while hovering an unfocused tab → [e10s] Media is unblocked while hovering an unfocused tab
Priority: -- → P1
Whiteboard: [testcoverage]
moving tracking from duplicate bug 1394118
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)
Blocks: 1385453
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.
Flags: needinfo?(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.
Flags: needinfo?(alwu)
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.
(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.
(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.
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).
Assignee: nobody → 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.
Flags: needinfo?(mconley)
See Also: → 1395735
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.
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify+
Whiteboard: [testcoverage] → [photon-performance] [testcoverage]
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 on attachment 8905223 [details]
Bug 1394455 - Disable tab warming until bug 1397426 is fixed. :(

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

Sad r+.
Attachment #8905223 - Flags: review?(florian) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/515aab24cac4
Disable tab warming until bug 1397426 is fixed. :( r=florian
https://hg.mozilla.org/mozilla-central/rev/515aab24cac4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.