1585769 - Attempting to open some videos in the Picture-in-Picture player results in an all-white player window and SecurityError: The operation is insecure error

Status: RESOLVED FIXED

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

Description

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

STR:

1. Visit https://www.formulize.org/videos/
2. Click "Play" on the slideshow player
3. Click the > arrow at the bottom of the player until it gets overtaken by a YouTube video
4. Hover the video so that the Picture-in-Picture toggle displays
5. Click on the toggle

ER:

The video should open in the Picture-in-Picture player window.

AR:

The player window opens, but only shows white. The following error is displayed in the Browser Console

15:21:07.492 SecurityError: The operation is insecure. PictureInPictureChild.jsm:942

Comment 1

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

This seems to be due to how the triggering principal on the <browser> is based on the top-level frame of the originating video.

I think it's a mistake to declare that the triggering principal is that top-level frame principal. I think we can get away with having it be the null principal (principal of least ability).

Comment 2

[:Gijs (he/him)]

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

This seems to be due to how the triggering principal on the <browser> is based on the top-level frame of the originating video.

I think it's a mistake to declare that the triggering principal is that top-level frame principal. I think we can get away with having it be the null principal (principal of least ability).

I'm not super familiar with the architecture here, but that would potentially change what referrer is sent for the video, or whether same-site cookies get used, or what container it gets opened in, or ... So I'd proceed with caution...

Comment 3

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

Attachment: Bug 1585769 - Create a fresh <video> element for the Picture-in-Picture player window rather than cloning the original element. r?JSON_Voorhees

Cloning the original node was needed when we needed the MediaInfo copied over, but
HTMLVideoElement::CloneElementVisually copies it over for us, so we can create a
fresh <video> element. This should also be a much healthier thing to do
security-wise, since we're not cloning strange nodes from the web.

Comment 4

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

I'm upgrading the priority of this, because we now appear to be broken for YouTube embeds on (at least) Reddit.

STR:

1. Visit https://old.reddit.com/r/puptheband/comments/dcunuj/pup_sibling_rivalry_live_at_the_verge/ (though I imagine any embedded video on Reddit will work)
2. Play the video
3. While the video is playing, attempt to open it in Picture-in-Picture via the toggle

ER:

The video should pop out into a PiP player window.

AR:

A blank PiP player window is opened, but the original video continues to play in its starting location.

Comment 5

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

Going to write a regression test for this too.

Comment 6

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

Attachment: Bug 1585769 - Regression test. r?JSON_Voorhees

Depends on D48123

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e82ef20a4b11
Create a fresh <video> element for the Picture-in-Picture player window rather than cloning the original element. r=JSON_voorhees

Comment 8

[Razvan Maries]

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

Comment 9

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

Note that the fix for this has landed, and I'm just waiting on the review for the test patch, then I can close this out.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00b56e289d2e
Regression test. r=JSON_voorhees

Comment 11

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 00b56e289d2e (Bug 1585769) for causing perma bc failures in toolkit/components/pictureinpicture/tests/browser_thirdPartyIframe.js CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=1501701f252e26578df9bfa37ffb22d7a5f5e825&selectedJob=271534451

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=271534451&repo=autoland&lineNumber=20281

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a31cb7d5f3ba
Backed out changeset 00b56e289d2e for causing perma bc failures in toolkit/components/pictureinpicture/tests/browser_thirdPartyIframe.js CLOSED TREE

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56c7f7a3915c
Regression test. r=JSON_voorhees

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/56c7f7a3915c