1606660 - Fission iframes don't handle allowfullscreen correctly.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

This check only runs in the parent process.

The infrastructure added in the blocking bugs will be useful to fix this.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 2

[Thomas Nguyen (:tnguyen)]

I think we also should take allow="fullscreen" into account here.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Chris Peterson [:cpeterson]]

This bug blocks Fission Nightly (milestone M6), which is targeting mid-2020. Therefore, this bug ought to be fixed some time in 2020 Q1 or Q2.

Comment 5

[Chris Peterson [:cpeterson]]

Emilio, did you have STR for this allowfullscreen bug?

kmag says this bug should also affect non-Fission e10s if the allowfullscreen check is in the parent process.

You added a comment (in bug 1606660) saying:

    // FIXME(emilio, bug 1606660): allowfullscreen should probably move to
    // OwnerShowInfo.

https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/dom/ipc/BrowserParent.cpp#458-459

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Well, that is true in fairness. That being said, this is the check that's supposed to prevent it for regular frames, and it is still not fission-safe...

Yet http://crisal.io/tmp/youtube-embed.html does work as expected... So something is off.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Here's a test-case: http://crisal.io/tmp/allowfullscreen-fission.html

It incorrectly shows fullscreenEnabled = true everywhere, while it should show false, then true.

The reason is that this check doesn't work in fission.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Yet the test-case in comment 6 ~works on Fission because of this feature policy check, which is not here.

So allowfullscreen doesn't work, but that feature-policy check should cause fullscreenEnabled to return false in the first place.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1606660 - Factor out AllowFullscreen checks for iframe. r=nika

This has the side effect of fixing support of mozallowfullscreen when
feature policy is enabled, because it checks only AllowFullscreen, which
doesn't check for the moz-prefixed attribute.

I left the swapframeloader bits alone because they're mozbrowser
specific and I have no idea what they're trying to do.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1606660 - Move allowfullscreen checks to the browsing context. r=nika

So that they work properly on fission iframes.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1606660 - Remove support for <browser disablefullscreen>. r=nika,smaug

I noticed that my previous patch wouldn't support it properly, and while
it could be feasible to fix it, it seems better to just remove the
feature. The current implementation doesn't work for remote browsers
already.

This was added in bug 1299795 but we no longer allow to load untrusted
content in the sidebar, afaict, and if we did we definitely don't allow
to load it in the parent process.

Depends on D78702

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/615339857e65
Factor out AllowFullscreen checks for iframe. r=nika
https://hg.mozilla.org/integration/autoland/rev/fef0196c86dd
Move allowfullscreen checks to the browsing context. r=nika

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/615339857e65
https://hg.mozilla.org/mozilla-central/rev/fef0196c86dd