1576605 - Video would not switch to picture in picture mode

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P1)

Description

[Alice0775 White]

Attachment: about:support

Reproducible: reproducible with new profile, often

Steps to reproduce:
1. Create new profile and start browser
2. Open any video (e.g, https://videos.cdn.mozilla.net/uploads/mozillaorg/Mozilla_2014_i_am.webm )
3. Start playback if necessary
4. Left click on blue toggle button
   --- if not reproduced, restart browser and repeat from step 2.
   Or
   Right click on the video and choose "Picture in Picture"
   --- if not reproduced, restart browser and repeat from step 2.

Actual Results:
Nothing happens

Expected Results:
Should switch to PiP mode

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f246623260ee3b80c2021c48f5d56f54073f8060&tochange=b134d9db39a9da2cdb1a4b7c19e6023063fde7f4

Regressed by: Bug 1568320

Comment 1

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

Hi Alice0775 White, thanks for testing Picture-in-Picture. Are there any errors in the Browser Console when you're able to reproduce this issue?

Comment 2

[Alice0775 White]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #1)

Hi Alice0775 White, thanks for testing Picture-in-Picture. Are there any errors in the Browser Console when you're able to reproduce this issue?

No error in browser console.

Only error on loaded when the video, but I think this is not related.
[Exception... "Favicon at "https://videos.cdn.mozilla.net/favicon.ico" failed to load: Forbidden." nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource:///modules/FaviconLoader.jsm :: onStopRequest :: line 236" data: no] FaviconLoader.jsm:236:22

Comment 3

[:Gijs (he/him)]

Mike is looking into this more, but some stuff I noticed...

I struggled to repro, but eventually managed to do so on Windows. It doesn't seem to repro on mac per talking to Mike, unsure why. Also unsure why it's not consistent...

In any case, when things break, I noticed that the child successfully calls togglePictureInPicture at https://searchfox.org/mozilla-central/rev/8ea946dcf51f0d6400362cc1d49c8d4808eeacf1/toolkit/actors/PictureInPictureChild.jsm#753, but at that point seems to believe we're already in picture in picture. That is, gWeakVideo points to the page's video. Unfortunately, I only got the debugger out once I could reproduce, so I'm not sure how we got into that state without an actual picture in picture window. In any case, at that point we never end up opening a picture in picture window because we just hang on to the weak video ref (or overwrite it with the same video) no matter how many times you click the button. I don't see any code that nulls out the weak video ref, except the page unload...

Comment 4

[Alice0775 White]

When you observe the screen carefully,
when the problem first occurs, it is observed that a small window opens for a moment and closes immediately. And after that, clicking the toggle button won't respond to anything.

Comment 5

[Itiel]

I can also see this often, on Windows 10.

Comment 6

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

I think I've figured this out. Bug 1568320 made it so that we're accidentally sending two MozTogglePictureInPicture events rather than 1 when we're dealing with a video that has the built-in controls visible, which is being interpreted as "open and then close the video in Picture-in-Picture". The events come so quickly though and go through messaging in such a way that it's non-deterministic.

The correct fix here is to fire only a single MozTogglePictureInPicture event

Comment 7

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

Attachment: Bug 1576605 - Don't fire extra MozTogglePictureInPicture when clicking toggle on videos with built-in controls. r?JSON_Voorhees

In bug 1568320, we made it so that the PictureInPictureToggleChild attaches handlers to
the toggle, even on videos that have controls by default, so we don't need the
videocontrols UAWidget to call the method for firing the event anymore.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63f962127985
Don't fire extra MozTogglePictureInPicture when clicking toggle on videos with built-in controls. r=JSON_voorhees

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/63f962127985

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift to beta? That is, if you think is would not be risky in some way. Thanks.

Comment 11

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

Comment on attachment 9089540 [details]
Bug 1576605 - Don't fire extra MozTogglePictureInPicture when clicking toggle on videos with built-in controls. r?JSON_Voorhees

Beta/Release Uplift Approval Request

• User impact if declined: Users testing Picture-in-Picture on Beta will find that videos that use the built-in browser controls may not enter Picture-in-Picture mode sometimes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a small, well-contained and well-understood patch.
• String changes made/needed: None.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9089540 [details]
Bug 1576605 - Don't fire extra MozTogglePictureInPicture when clicking toggle on videos with built-in controls. r?JSON_Voorhees

Improvement for an upcoming feature (though it won't ship in 70)
OK for uplift for beta 5.

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1ec25f256c8

Comment 14

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 71.0a1 (2019-09-09) and on Firefox 70.0b5 on Windows 10 x64, Windows 7 x32, Ubuntu 18.04 and Mac OS X 10.15.