1613054 - Figure out what to do if SetFocus ends up being called for an iframe element hosting an out-of-process iframe

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, task)

Description

[Henri Sivonen (:hsivonen)]

The patch for bug 1556627 doesn't deal with this case. Figure out if more work is needed here.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 2

[Henri Sivonen (:hsivonen)]

As seen in bug 1580618, we need a call to .focus() on an OOP iframe synchronously make the iframe element the focused element of the DOM inner window that the element is in. Then, we can have the process hosting the iframe content do its focus setting asynchronously.

This start going wrong when elementToFocus is set to nullptr in SetFocusInner, because the focus-taking element inside the iframe is not in the current process.

Comment 3

[Henri Sivonen (:hsivonen)]

My attempt to eagerly set the iframe as the focused element fails, because it's composed doc doesn't match the expected doc at
https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/dom/base/nsGlobalWindowInner.cpp#4079
and
https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/dom/base/nsFocusManager.cpp#2333

Neil, what's the significance of these checks? How should I adjust these checks to allow an iframe aElement in these cases without breaking something else?

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1613054 part 1 - Synchronously request frame focus when .contentWindow.focus() called on OOP iframe.

Comment 5

[Neil Deakin]

(In reply to Henri Sivonen (:hsivonen) from comment #3)

My attempt to eagerly set the iframe as the focused element fails, because it's composed doc doesn't match the expected doc at
https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/dom/base/nsGlobalWindowInner.cpp#4079

You can't set the focused element to an element from a different document. If the documents don't match, then something is calling SetFocusedElement on the wrong window.

https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/dom/base/nsFocusManager.cpp#2333

This check can fail some if the element was moved or the focus was adjusted during the focus event that was fired earlier within Focus(). If so, we don't want to continue focusing the element. It isn't perfect but we try to handle some situations where script might try to adjust the focus during a focus/blur event listener.

Comment 6

[Chris Peterson [:cpeterson]]

This bug is a Fission Nightly blocker (milestone M6c).

Comment 7

[Henri Sivonen (:hsivonen)]

Bug 1649099 shows that things are OK after the iframe has loaded. The problem with the test here is that the iframe still reports having an inner window at the time of the focus() call. We should probably make the inner window of an iframe that's about to navigate to a different side go away sooner.

Before doing that I should test Chrome some more.

Comment 8

[Henri Sivonen (:hsivonen)]

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

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1613054 part 2 - On iframe.focus() create the initial about:blank for in-process case and treat the iframe as focusable for OOP case.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efecc359f305
part 1 - Synchronously request frame focus when .contentWindow.focus() called on OOP iframe. r=nika

Comment 11

[Noemi Erli[:noemi_erli]]

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

Comment 12

[Henri Sivonen (:hsivonen)]

Diagnostic try run to figure out why we can create an initial-looking about:blank when it clearly isn't initial:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54cdcd82323ab99fd5618926321348662290d039

Comment 13

[Henri Sivonen (:hsivonen)]

Try run with release asserting that we don't run the about:blank creation path that fires events:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26a4bca3366ad159c4c8a4fa695b3e7ec2d3b650

Comment 14

[Henri Sivonen (:hsivonen)]

Try run with script blocker:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5470a183653f32c10d21a453b85e8865aff474f2

Comment 15

[Henri Sivonen (:hsivonen)]

Filed bug 1663853 as a follow-up.

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #14)

Try run with script blocker:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5470a183653f32c10d21a453b85e8865aff474f2

Sigh. This doesn't work:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315178499&repo=try&lineNumber=112788

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73f6874cb3dd
part 2 - On iframe.focus() create the initial about:blank for in-process case and treat the iframe as focusable for OOP case. r=nika

Comment 18

[Alexandru Michis [:malexandru]]

Backed out changeset 73f6874cb3dd (Bug 1613054) for causing bc failures in browser_abort_visibility.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/f22c20641704d783097b1b128d826c8460de7354

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cusercancel%2Crunning%2Cpending%2Crunnable&searchStr=bc&tochange=f22c20641704d783097b1b128d826c8460de7354&fromchange=a99710068e299a3264f5cae69dc89d959370baef

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315208206&repo=autoland&lineNumber=3658

Comment 19

[Henri Sivonen (:hsivonen)]

Landing requeued with edit per discussion with Nika.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5194d31bb88d
part 2 - On iframe.focus() create the initial about:blank for in-process case and treat the iframe as focusable for OOP case. r=nika

Comment 21

[Razvan Maries]

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=c9Gzf7qJTZWyobu80ynf9g.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=1f97271accd57286c88d27ac770e8bf73462bb71&searchStr=bc&failure_classification_id=2

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315227233&repo=autoland&lineNumber=17482

Backout: https://hg.mozilla.org/integration/autoland/rev/2e060dffb1c85dcad897939be1cd53c4e076f858

Comment 22

[Henri Sivonen (:hsivonen)]

(In reply to Razvan Maries from comment #21)

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315227233&repo=autoland&lineNumber=17482

Sigh. This test was a pre-existing intermittent, which I why I didn't look into in more in the context of a try push.

Comment 23

[Henri Sivonen (:hsivonen)]

Try run with XUL excluded from the new behavior:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f83f96ec3ff794b0fde62d2093ada96e1055229

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b80dd69ca213
part 2 - On iframe.focus() create the initial about:blank for in-process case and treat the iframe as focusable for OOP case. r=nika

Comment 25

[Narcis Beleuzu [:NarcisB]]

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