1646561 - Fix GetInProcessParentDocshell usage in DocShell::GetVisibility

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Kris Maglione [:kmag]]

It doesn't handle OOP ancestors, which it probably should. Though it's possible that we propagate visibility information to OOP docshells in some other way.

Comment 1

[Chris Peterson [:cpeterson]]

Emilio, what happens if this code is wrong for Fission OOP iframes? Do we need to fix this before we enable Fission?

See also IsVisibleConsideringAncestors bug 1646486.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I'm not particularly familiar with this API... It seems there are a bunch of callers, some from devtools, other from ESM, etc... Most of them look not too terrible, but I haven't checked in detail so beware.

The function of course is completely broken in both fission and e10s... Maybe we should either assert we only call it from the parent process, or try to remove it somehow, it doesn't seem like there are too many callers.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

We should also check nsIFrame::IsVisibleConsideringAncestors and make sure that's OK, as it's also broken with fission due to using GetCrossDocParentFrame: https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/layout/generic/nsIFrame.cpp#409

Comment 4

[Neha Kochar [:neha]]

Moving to M7 based on comment 2.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

I think this might be working somewhat now thanks to the other changes you've been doing for visibility etc. with Fission :emilio. Is there more work we need to do here, or can we resolve this?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

GetVisibility right now seems completely independent from IsActive right now, but probably shouldn't. I think effectively most of the callers could be changed to check BrowsingContext::IsActive without changing behavior, but there are some which do seem to care about activeness vs. actually-in-viewport.

Comment 7

[Steven MacLeod [:smacleod]]

Attachment: Bug 1646561 - Stop using DocShell::GetVisibility in PresShell. r=emilio

Comment 8

[Steven MacLeod [:smacleod]]

Attachment: Bug 1646561 - Stop using DocShell::GetVisibility in nsRefreshDriver. r=emilio

Depends on D108238

Comment 9

[Steven MacLeod [:smacleod]]

Attachment: Bug 1646561 - Stop using DocShell::GetVisibility in nsXULPopupManager. r=emilio

Depends on D108239

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ab33e9d46a4
Stop using DocShell::GetVisibility in PresShell. r=emilio
https://hg.mozilla.org/integration/autoland/rev/39dc65fb7714
Stop using DocShell::GetVisibility in nsRefreshDriver. r=emilio

Comment 11

[Steven MacLeod [:smacleod]]

Attachment: Bug 1646561 - Stop using DocShell::GetVisibility in nsFocusManager. r=emilio!,mconley!

We stop checking for the tab state to be STATE_LOADED since it
messes up initial focus. Instead, we directly check if the tab
is warming, since that was the intention of this check, AFAICT.
See Bug 1397426 for where this was introduced.

Depends on D108240

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/708a20e04616
Stop using DocShell::GetVisibility in nsXULPopupManager. r=emilio

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7ab33e9d46a4
https://hg.mozilla.org/mozilla-central/rev/39dc65fb7714
https://hg.mozilla.org/mozilla-central/rev/708a20e04616

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab94b17b8da3
Stop using DocShell::GetVisibility in nsFocusManager. r=emilio,mconley

Comment 15

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_abort_visibility.js

backout: https://hg.mozilla.org/integration/autoland/rev/7f3f32624f0707d56f7bdb4606205227aafcbe30

push: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=cdOtdibvTxK4N7Fd7QKUSg.0&revision=ab94b17b8da39b7edae0d1c83d3b779c90b56a97&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer?job_id=333415847&repo=autoland&lineNumber=5948

[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - TEST-PASS | dom/webauthn/tests/browser/browser_abort_visibility.js | webauthn request pending -
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - must wait for focus
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - Buffered messages logged at 18:01:47
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - visbility state: visible
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - active: true
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - Buffered messages finished
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - TEST-UNEXPECTED-FAIL | dom/webauthn/tests/browser/browser_abort_visibility.js | webauthn request pending - Got "aborted", expected "pending"
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - Stack trace:
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - chrome://mochikit/content/browser-test.js:test_is:1359
[task 2021-03-16T18:01:47.838Z] 18:01:47 INFO - chrome://mochitests/content/browser/dom/webauthn/tests/browser/browser_abort_visibility.js:assertStatus:20
[task 2021-03-16T18:01:47.933Z] 18:01:47 INFO - Leaving test bound test_minimize_make
[task 2021-03-16T18:01:47.933Z] 18:01:47 INFO - Entering test bound test_minimize_get
[task 2021-03-16T18:01:48.567Z] 18:01:48 INFO - Attempting to get credential for origin: https://example.com
[task 2021-03-16T18:01:48.567Z] 18:01:48 INFO - visbility state: visible
[task 2021-03-16T18:01:48.567Z] 18:01:48 INFO - active: true
[task 2021-03-16T18:01:48.567Z] 18:01:48 INFO - TEST-PASS | dom/webauthn/tests/browser/browser_abort_visibility.js | webauthn request pending -

Comment 16

[Natalia Csoregi [:nataliaCs]]

Also failing:

• TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_popup_preventdefault_chrome.xhtml | focused window after restore - got null, expected [object Window]
• TEST-UNEXPECTED-FAIL | dom/credentialmanagement/tests/browser/browser_active_document.js | Test timed out -

Comment 17

[Steven MacLeod [:smacleod]]

Attachment: Bug 1646561 - Stop using DocShell::GetVisibility in EventStateManager. r=emilio!

Comment 18

[Neha Kochar [:neha]]

Edgar, can you please take over this one? The remaining work is:
Patch D108452 - Fix the Mac test failure to fix the backout.
Patch D108781 - Discuss with smaug as he needed something additionally fixed in the patch.

Comment 19

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2e8a33b57ac4a29137e9232bfc94c7d599f9159

Comment 20

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #19)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2e8a33b57ac4a29137e9232bfc94c7d599f9159

Okay, so the test failure on Mac are also caused by occlusion state updating.

Comment 21

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55633c4f1e23029c744cb86c614f5b72c3cac8b4

Comment 22

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50f673e9fe012285a596070f1efd444c29106d53

Comment 23

[Edgar Chen [:edgar]]

I tested the behavior of trying to focus element in a hidden cross-origin iframe on fission, non-fission and other browsers,

--	display: none	visibility: hidden
Gecko (Fission)	YES	YES NO
Gecko (non-Fission)	NO	NO
Blink	NO	YES
Webkit	YES*	YES*

YES: focus could move into hidden iframe.
NO: focus could not move into hidden iframe.
*: Webkit requires user to interact with iframe first, e.g. click it, to allow iframe the get the focus.

--
Update:
Correct the result of Fission for visibility: hidden.

Comment 24

[Edgar Chen [:edgar]]

Attachment: display_none.html

Comment 25

[Edgar Chen [:edgar]]

Attachment: visibility_hidden.html

Comment 26

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] (away ~ 7/4) from comment #23)

I tested the behavior of trying to focus element in a hidden cross-origin iframe on fission, non-fission and other browsers,

--	display: none	visibility: hidden
Gecko (Fission)	YES	NO
Gecko (non-Fission)	NO	NO
Blink	NO	YES
Webkit	YES*	YES*

YES: focus could move into hidden iframe.
NO: focus could not move into hidden iframe.
*: Webkit requires user to interact with iframe first, e.g. click it, to allow iframe the get the focus.

So each browser has different behavior and as we don't allow to focus element in in-process iframe that is not visible in the viewport, so I would like just make oop iframe behave the same as in-process iframe. We have fixed the visibility: hidden case for remote iframe in bug 1541253, so I would also make the display: none case for oop iframe behave the same as in-process iframe.

Comment 27

[Edgar Chen [:edgar]]

For the scrolling behavior, I testes various cases with in-process iframe, but I could not find a case that we try to scroll the view to the hidden iframe. So yes, we have kinda different focus behaviour on hidden/background tab and hidden iframe. But since this is what we have now, I am not sure how risky to change it. I would like to keep the behaviour, i.e. not allow focus the element in hidden iframe, please let me know if you have any concern, emilio.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

With "hidden", I meant "scrolled out of view". Obviously frames inside visibility: hidden or display: none shouldn't be focusable. So I was misunderstanding what you said with IsVisible. Anyhow, I think the issue is that we don't propagate the "under hidden embedder element" bit correctly to OOP iframes, which is what makes the visibility: hidden case behave inconsistently on fission.

Comment 29

[Edgar Chen [:edgar]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #28)

With "hidden", I meant "scrolled out of view". Obviously frames inside visibility: hidden or display: none shouldn't be focusable.

Thanks, I filed bug 1716762 for this.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

I don't quite understand the switch from visibility check to active check. Those used to be quite different things, especially in the parent process.

Comment 31

[Edgar Chen [:edgar]]

With bug 1716762, the display: none information will be propagated to OOP iframe through PresShell along with the information of visibility: hidden. The DocShell::GetVisibility will check nsIFrame::IsVisibleConsideringAncestors which checks PresShell::IsUnderHiddenEmbedderElement. I also write some test and didn't find issue. So I think the remaining usage in nsFocusManager and EventStateManager should be fine.

Comment 32

[Edgar Chen [:edgar]]

Close per comment #31.