Closed Bug 1709125 Opened 3 years ago Closed 3 years ago

Unable to restore focus with fission + xorigin enabled

Categories

(Testing :: Mochitest, defect, P2)

Product:
Testing
Component:
Mochitest
Version:
Default
Type:
defect

Tracking

(Fission Milestone:M7a, firefox-esr78 disabled, firefox88 disabled, firefox89 disabled, firefox90 disabled, firefox91 fixed)

Status:
RESOLVED FIXED
Milestone:
91 Branch
Project Flags:
Fission Milestone M7a
Tracking Flags:
Tracking Status
firefox-esr78 --- disabled
firefox88 --- disabled
firefox89 --- disabled
firefox90 --- disabled
firefox91 --- fixed

People

(Reporter: ahal, Assigned: hsivonen)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1709125 - Make Document::HasFocus() use the BrowsingContext hierarchy.
48 bytes, text/x-phabricator-request
Details | Review

While triaging failures for bug 1700781, I noticed (somewhat belatedly) that the harness isn't restoring focus between tests when fission and xorigin are enabled. This causes a little timeout after each test as we wait for focus and will often cause chunks to reach the maximum taskcluster timeout (and can cause other test failures as well).

I'm unsure how to fix this myself, but the code that attempts to restore focus lives here:
https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/testing/mochitest/tests/SimpleTest/TestRunner.js#394

Notably that _makeIframe function has a bunch of code specific to xorigin mode, so maybe the code to re-focus the window also needs some modifications in this case.

Chris, unfortunately this will need to be fixed before we can land bug 1700781. Hopefully a fission engineer will be able to identify the issue relatively quickly (see link from description).

Flags: needinfo?(cpeterson)

Another possibility here is that the if statement that guards the re-focusing code needs an additional clause to account for xorigin.

kmag will investigate. If this is a bug in the mochitest harness, he will fix it. If this is a bug in Gecko's focus code, then he will ask hsivonen to investigate.

Assignee: nobody → kmaglione+bmo
Severity: -- → S3
Fission Milestone: --- → M7a
status-firefox88: --- → disabled
status-firefox89: --- → disabled
status-firefox90: --- → affected
status-firefox-esr78: --- → disabled
Flags: needinfo?(cpeterson) → needinfo?(kmaglione+bmo)
Priority: -- → P2

See the tasks in this push for some examples:
https://treeherder.mozilla.org/jobs?repo=try&revision=81300d0a57271ea89fdcd832d3b46b874adb6749

Note both the failing and passing tasks are hitting this issue, it's just that most chunks happen to stay under the timeout.

Unfortunately, I haven't been able to reproduce this locally, but it does seem to be an issue in the focus code rather than the Mochitest framework.

Essentially, the harness tries to focus its own window, which is the top-level window for the tab, and the test iframe (which is in this case cross-origin and cross-process), calling window.focus() and iframe.focus(), and also asking SpecialPowers to call focus() on the <browser> element for the tab just in case. It tries that 3 times until document.hasFocus() is true and document.activeElement is the test iframe. I'm not sure which of those fails, but the logic does seem more or less sane, so I have to conclude that the issue must be in the platform.

So forwarding to Henri, as discussed.

Flags: needinfo?(kmaglione+bmo) → needinfo?(hsivonen)

Actually, it turns out I can reproduce this locally with the revision of the try push, just not the one I had checked out already. document.hasFocus() is returning false, but the activeElement check passes. I bisected, and it's regression from https://hg.mozilla.org/mozilla-central/rev/5c4c4c9aa022

Assignee: kmaglione+bmo → hsivonen
Regressed by: 1696323

(In reply to Kris Maglione [:kmag] from comment #6)

Actually, it turns out I can reproduce this locally with the revision of the try push, just not the one I had checked out already. document.hasFocus() is returning false, but the activeElement check passes. I bisected, and it's regression from https://hg.mozilla.org/mozilla-central/rev/5c4c4c9aa022

:-(

The regressing change seemed pretty clearly a step in the right direction. Perhaps it was incomplete?

I'll take a look.

Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)

https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/dom/base/Document.cpp#4324 walks the window hierarchy instead of the BrowsingContext hierarchy.

(Code-wise, this doesn't look like a regression, but the bug whose fix "regressed" this one was somehow masking the bug here.)

I have trouble coming up with a test case that would fail without the patch, which is probably related to why this wasn't caught earlier.

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86a2163e7a47
Make Document::HasFocus() use the BrowsingContext hierarchy. r=edgar
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28904 for changes under testing/web-platform/tests

https://hg.mozilla.org/mozilla-central/rev/86a2163e7a47

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox90: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Upstream PR merged by moz-wptsync-bot

(In reply to Andrew Halberstadt [:ahal] from comment #0)

While triaging failures for bug 1700781, I noticed (somewhat belatedly) that the harness isn't restoring focus between tests when fission and xorigin are enabled. This causes a little timeout after each test as we wait for focus and will often cause chunks to reach the maximum taskcluster timeout (and can cause other test failures as well).

Andrew, this harness focus bug you found with the fission-xorigin tests should be fixed now.

Flags: needinfo?(ahal)

Backed out for causing permafailures on test_hover_mouseleave.html

backout: https://hg.mozilla.org/integration/autoland/rev/0c8028d891714aecff3bf9adc7b1870995c7f861

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cmochitest%2Cfission&revision=86a2163e7a475f2b9310a278365499778e8ca6e0&selectedTaskRun=LKHRgP-4TvWtuhrkogCP4Q.0

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

[task 2021-05-08T14:16:56.103Z] 14:16:56 INFO - Buffered messages logged at 14:11:28
[task 2021-05-08T14:16:56.103Z] 14:16:56 INFO - must wait for load
[task 2021-05-08T14:16:56.104Z] 14:16:56 INFO - must wait for focus
[task 2021-05-08T14:16:56.104Z] 14:16:56 INFO - Buffered messages finished
[task 2021-05-08T14:16:56.105Z] 14:16:56 INFO - TEST-UNEXPECTED-FAIL | dom/events/test/test_hover_mouseleave.html | Test timed out. -
[task 2021-05-08T14:16:56.950Z] 14:16:56 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-05-08T14:16:56.953Z] 14:16:56 INFO - TEST-UNEXPECTED-FAIL | dom/events/test/test_hover_mouseleave.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run. Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected +0
[task 2021-05-08T14:16:56.953Z] 14:16:56 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-05-08T14:16:56.954Z] 14:16:56 INFO - afterCleanup@SimpleTest/SimpleTest.js:1560:18
[task 2021-05-08T14:16:56.954Z] 14:16:56 INFO - executeCleanupFunction@SimpleTest/SimpleTest.js:1636:7
[task 2021-05-08T14:16:56.954Z] 14:16:56 INFO - SimpleTest.finish@SimpleTest/SimpleTest.js:1656:3
[task 2021-05-08T14:16:56.955Z] 14:16:56 INFO - killTest@SimpleTest/TestRunner.js:194:22
[task 2021-05-08T14:16:56.964Z] 14:16:56 INFO - GECKO(1547) | MEMORY STAT | vsize 2621MB | residentFast 181MB | heapAllocated 12MB
[task 2021-05-08T14:16:56.979Z] 14:16:56 INFO - TEST-OK | dom/events/test/test_hover_mouseleave.html | took 328738ms

This was a tier 2 permafailure but switched to tier 1 once it reached mozilla-central - please see this comment here for a more detailed explanation

Flags: needinfo?(hsivonen)
Status: RESOLVED → REOPENED
status-firefox90: fixed → affected
Resolution: FIXED → ---
Target Milestone: 90 Branch → ---
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28914 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot

Looks like it got backed out. But I'll pull this stack into my own queue and I can my stack ready to go as soon as this lands.

Flags: needinfo?(ahal)

(In reply to Natalia Csoregi [:nataliaCs] from comment #18)

Backed out for causing permafailures on test_hover_mouseleave.html

This test passes for me locally when run individually.

During a successful run, waitForFocus doesn't end up invoking hasFocus at all.

There are four invocations of hasFocus between the test start and the call for waitForFocus (and none after).

Two of those come from C++ Document::SetScriptGlobalObject (for focus timestamp).

Two of them come from TestRunner._makeIframe the first is from XHR completion and the other is a retry. On both occasions hasFocus returns true. So even with the patch during a successful run, we retry TestRunner._makeIframe but not due to hasFocus itself.

Flags: needinfo?(hsivonen)

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

Two of them come from TestRunner._makeIframe the first is from XHR completion and the other is a retry. On both occasions hasFocus returns true. So even with the patch during a successful run, we retry TestRunner._makeIframe but not due to hasFocus itself.

This is due to activeElement returning body on the first attempt, because the is no real focused element.

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

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

Two of them come from TestRunner._makeIframe the first is from XHR completion and the other is a retry. On both occasions hasFocus returns true. So even with the patch during a successful run, we retry TestRunner._makeIframe but not due to hasFocus itself.

This is due to activeElement returning body on the first attempt, because the is no real focused element.

The iframe becomes focused only when we focus it here:
https://searchfox.org/mozilla-central/rev/cca1566127a2fcc013e9c09f9d90ed70df2250a4/testing/mochitest/tests/SimpleTest/TestRunner.js#404

Seems inefficient to wait for a full second for the focusing to take effect.

Let's see if the failure just moves to the next test if I disable test_hover_mouseleave.html
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42acb1169aaa89ff2948a978262dfc8a7b42dacd

It looks like even with this patch applied I'm still seeing "Unable to restore focus, expect failures and timeouts" in the log:
https://treeherder.mozilla.org/logviewer?job_id=339309084&repo=try&lineNumber=9553

From this push:
https://treeherder.mozilla.org/jobs?repo=try&revision=363b3e38c14aa51bd0507bb48d8acf41ef36a0a4

The perma-fails in that push are from hitting the max taskcluster timeout due to the 3 second delay that happens after each test trying to restore focus.

Flags: needinfo?(hsivonen)

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

Seems inefficient to wait for a full second for the focusing to take effect.

No arguments here, I'm happy to bump that down to 100ms or something. That would likely solve the timeouts that are blocking me at least. Though I think we would still want to fix this first? I'm not sure.

Now test_legacy_non-primary_click.html fails the same way even though it's earlier in the manifest.

Apart from waitForFocus, the other commonality is synthesizeMouseAtCenter, but there are plenty of tests that use those in the directory.

(In reply to Andrew Halberstadt [:ahal] from comment #27)

It looks like even with this patch applied I'm still seeing "Unable to restore focus, expect failures and timeouts" in the log:
https://treeherder.mozilla.org/logviewer?job_id=339309084&repo=try&lineNumber=9553

From this push:
https://treeherder.mozilla.org/jobs?repo=try&revision=363b3e38c14aa51bd0507bb48d8acf41ef36a0a4

The perma-fails in that push are from hitting the max taskcluster timeout due to the 3 second delay that happens after each test trying to restore focus.

Perhaps there is a race condition between the use of hasFocus() and the active browsing context and the focused browsing context getting updated across processes, and thing fair on try but not locally. It would be great to catch the failure in Pernosco.

Flags: needinfo?(hsivonen)

It looks like whenever this fails, there's concurrent logging along these lines:

[task 2021-05-12T08:27:39.339Z] 08:27:39 INFO - GECKO(2636) | [Child 3134, Main Thread] WARNING: could not set real-time limit in CubebUtils::InitLibrary: file /builds/worker/checkouts/gecko/dom/media/CubebUtils.cpp:619
[task 2021-05-12T08:27:39.359Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.419Z] 08:27:39 INFO - GECKO(2636) | ###!!! [Parent][RunMessage] Error: Channel closing: too late to send/recv, messages will be lost
[task 2021-05-12T08:27:39.422Z] 08:27:39 INFO - GECKO(2636) | ###!!! [Parent][RunMessage] Error: Channel closing: too late to send/recv, messages will be lost
[task 2021-05-12T08:27:39.426Z] 08:27:39 INFO - GECKO(2636) | ###!!! [Parent][RunMessage] Error: Channel closing: too late to send/recv, messages will be lost
[task 2021-05-12T08:27:39.426Z] 08:27:39 INFO - GECKO(2636) | ###!!! [Parent][RunMessage] Error: Channel closing: too late to send/recv, messages will be lost
[task 2021-05-12T08:27:39.437Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.486Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.508Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.521Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.542Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.604Z] 08:27:39 INFO - GECKO(2636) | [Child 3131, Main Thread] WARNING: could not set real-time limit in CubebUtils::InitLibrary: file /builds/worker/checkouts/gecko/dom/media/CubebUtils.cpp:619
[task 2021-05-12T08:27:39.641Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.668Z] 08:27:39 INFO - GECKO(2636) | [Child 3047, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3357
[task 2021-05-12T08:27:39.681Z] 08:27:39 INFO - GECKO(2636) | [Child 3047, Main Thread] WARNING: '!obs', file /builds/worker/checkouts/gecko/toolkit/components/sessionstore/RestoreTabContentObserver.cpp:58
[task 2021-05-12T08:27:39.696Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.712Z] 08:27:39 INFO - GECKO(2636) | [Child 3047, Main Thread] WARNING: NS_ENSURE_TRUE(Preferences::InitStaticMembers()) failed: file /builds/worker/checkouts/gecko/modules/libpref/Preferences.cpp:4442
[task 2021-05-12T08:27:39.749Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.770Z] 08:27:39 INFO - GECKO(2636) | [Child 3075, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3357
[task 2021-05-12T08:27:39.779Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.787Z] 08:27:39 INFO - GECKO(2636) | [Child 3075, Main Thread] WARNING: '!obs', file /builds/worker/checkouts/gecko/toolkit/components/sessionstore/RestoreTabContentObserver.cpp:58
[task 2021-05-12T08:27:39.810Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.836Z] 08:27:39 INFO - GECKO(2636) | [Child 3075, Main Thread] WARNING: NS_ENSURE_TRUE(Preferences::InitStaticMembers()) failed: file /builds/worker/checkouts/gecko/modules/libpref/Preferences.cpp:4442
[task 2021-05-12T08:27:39.843Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:39.860Z] 08:27:39 INFO - GECKO(2636) | [Child 2798, Main Thread] WARNING: early callback, or time went backwards: '!aAllowIdleDispatch', file /builds/worker/checkouts/gecko/xpcom/threads/IdleTaskRunner.cpp:179
[task 2021-05-12T08:27:40.201Z] 08:27:40 INFO - GECKO(2636) | MEMORY STAT | vsize 2619MB | residentFast 185MB | heapAllocated 16MB
[task 2021-05-12T08:27:40.415Z] 08:27:40 INFO - TEST-OK | dom/events/test/test_legacy_event.html | took 1126ms
[task 2021-05-12T08:27:40.542Z] 08:27:40 INFO - TEST-START | dom/events/test/test_legacy_non-primary_click.html
[task 2021-05-12T08:32:56.176Z] 08:32:56 INFO - GECKO(2636) | 1620808376175 addons.xpi ERROR System addon update list error Error: got node name: html, expected: updates

This looks a lot like whatever is logging that is the disturbance and whatever test happens to run concurrently with that event fails.

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

with updateSystemAddons returning immediately

That's not it. test_hover_mouseleave.html failing again.

It turns out that the test harness accesses nsIFocusManager.focusedWindow, which returns null when the failure happens and non-null when the failure doesn't happen.

So far, it looks like the test harness wants nsIFocusManager.focusedWindow to be non-null if document.hasFocus() returns true, and the patch here apparently introduced a situation where that doesn't hold.

I don't quite understand why the code calls childDesiredWindow.focus(). Shouldn't desiredWindow.focus() end up also restoring the focus the same way?
https://searchfox.org/mozilla-central/rev/5359952d8b0be3e706e8c943c2bef2674723b8a9/testing/mochitest/tests/SimpleTest/SimpleTest.js#1096

Trying with that assumption:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a2adbd00b1ad4ae4438059fabe298c19af22863

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

I don't quite understand why the code calls childDesiredWindow.focus(). Shouldn't desiredWindow.focus() end up also restoring the focus the same way?
https://searchfox.org/mozilla-central/rev/5359952d8b0be3e706e8c943c2bef2674723b8a9/testing/mochitest/tests/SimpleTest/SimpleTest.js#1096

Trying with that assumption:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a2adbd00b1ad4ae4438059fabe298c19af22863

And the result is very orange. :-(

Neil, do you remember why the code looks up desiredChildWindow? AFAICT, it is the child window of desiredWindow that focus would be restored to if focus were to be restored to desiredWindow. Why does the code need to call .focus() on that window instead of calling .focus() on desiredWindow?

Do you have suggestions on how to make this work with Fission, when the BrowsingContext corresponding to desiredChildWindow may not be discoverable synchronously if it doesn't already have focus?

Flags: needinfo?(enndeakin)

- childDesiredWindow.addEventListener("focus", focusedOrLoaded, true);
- if (isChildProcess) {
- childDesiredWindow.focus();
- } else {
- SpecialPowers.focus(childDesiredWindow);
- }
+ desiredWindow.focus();

You would still need to listen for the focus event on the expected child iframe. I tried re-adding the event listener and ran the first failed test above ( docshell/test/navigation/test_bug430624.html) and the test passed again.

That said, I would expect that the original code could just call desiredWindow.focus() instead of childDesiredWindow.focus().

Flags: needinfo?(enndeakin)

Thanks.

You would still need to listen for the focus event on the expected child iframe.

There needs to be a more Fission-friendly way to do that than attaching the listener to the XPConnected DOM window when there's a BrowsingContext without a DOM window.

That said, I would expect that the original code could just call desiredWindow.focus() instead of childDesiredWindow.focus().

This works:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f9cd091fcf89ce5e999b62b6f9165d09a8647e7

Going to try without the SpecialPowers part next:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d310acd30522f52d6fe825ce580f4c8a52648ee

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

Going to try without the SpecialPowers part next:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d310acd30522f52d6fe825ce580f4c8a52648ee

Does very orange.

Trying to use WebIDL APIs to get the child window to attache the listener to:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1abca7b9308fd77210e30c86caaa6a965df90b2

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

Oops. Now with what I intended:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17855521221b38484b36d2b7d63fd575f776297d

Using Web APIs was very orange. Let's try adding some BrowsingContext-using XPIDL API surface:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63035f842e8cb3e2d4ffac13d68e9487a87acd9c

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

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

Oops. Now with what I intended:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17855521221b38484b36d2b7d63fd575f776297d

Using Web APIs was very orange. Let's try adding some BrowsingContext-using XPIDL API surface:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63035f842e8cb3e2d4ffac13d68e9487a87acd9c

That's very orange, too. :-(

Tried another way, also very orange:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=13a9f4461cf76c27e87aee497af6d16af1bb736e

I have a hard time coming up with changes that could improve things. Part of my problem is that I don't have full clarity of what the key characteristics of the way getFocusedElementForWindow is obtained and used in waitForLoadAndFocusOnWindow are.

Neil, do you have ideas?

To recap, Document::HasFocus() is obviously wrong for Fission. Patching it in the manner of the attached patch makes test_hover_mouseleave.html fail. Disabling test_hover_mouseleave.html makes an earlier test fail in the similar way!

The key problem introduced by the attached patch making Document::HasFocus work on BrowsingContexts seems to be that it's then potentially out of sync with the DOM window-based checks. In particular, when test_hover_mouseleave.html fails, nsIFocusManager.focusedWindow returns null even though hasFocus() returns true.

Flags: needinfo?(enndeakin)

waitForLoadAndFocusOnWindow is used to focus the desiredWindow that is passed to it. But the actual focus might be in a subframe (perhaps nested several levels deep)

Let's say some one focused an element in a nested subframe within a window, and assume for now that the window and all subframes are all in the same process. Then, one switches to a different window. When one switches back to that window again using waitForFocus, waitForLoadAndFocusOnWindow will be called with the top-level window as the desiredWindow. However, the focused window would be set to the nested subframe, and that is where the window/document focus events would be fired. So getFocusedElementForWindow is used to get this nested subframe and listen for events on it.

The focus manager's focusedWindow is always the deeply nested iframe that has the focus.

When e10s came along, each process had its own focus manager and separate focusedWindow. When you focus with a web page, the content process set the focusedWindow to the child iframe as before, BUT the parent process also has a focusedWindow set to the window containing the child <browser>. That's different than without e10s where focus was only in one iframe.

You can see this effect if you focus something in the sidebar, then lower and raise the window. The focus/blur events only fire on the sidebar frame. But if you focus something in a webpage, then lower and raise the window, the focus/blur events fire on the webpage as well as the top window browser.xhtml.

I assume that fission complicates this further and that the events fire on every process in the tree from wherever the focus is.

The non-e10s and non-fission cases would expect focusedWindow to always be the window containing focusedElement.

From brief testing I just did, it looks like the events only fire on the lowest nested process and frame that is focused. It seems from the description above (I haven't tested) that focusedWindow is null in a document containing a iframe when that iframe is focused and out-of-process? However testing in Chrome shows that document.hasFocus() returns true if any descendant iframe has focus. (so the HasFocus changes in this patch look correct from a brief glance).

Do we want to simply deprecate using focusedWindow and instead rely on using the focused browsing context instead? Where is focusedWindow being accessed where the test failure happens?

Note that waitForFocus doesn't support focusing out-of-process iframes. I have been working a version that does, but it is for browser-chrome tests only, so it wouldn't help here.

The code in TestRunner._makeIframe appears to be trying to focus some window (I assume this is in the top-level content window in the tab the tests are running in?) I'm not clear why it isn't just waiting for a focus event on 'iframe' and instead uses a timer. The SpecialPowers.focus() probably isn't needed, unless window.focus() runs into priviledge problems maybe.

Flags: needinfo?(enndeakin)

(In reply to Neil Deakin from comment #47)

waitForLoadAndFocusOnWindow is used to focus the desiredWindow that is passed to it. But the actual focus might be in a subframe (perhaps nested several levels deep)

Let's say some one focused an element in a nested subframe within a window, and assume for now that the window and all subframes are all in the same process. Then, one switches to a different window. When one switches back to that window again using waitForFocus, waitForLoadAndFocusOnWindow will be called with the top-level window as the desiredWindow. However, the focused window would be set to the nested subframe, and that is where the window/document focus events would be fired. So getFocusedElementForWindow is used to get this nested subframe and listen for events on it.

When the top BrowsingContext of a given hierarchy isn't the active BrowsingContext, we don't have the information about what the focused BrowsingContext of that hierarchy would be in processes other than the one hosting the corresponding DOM window.

That is, we only know about the actually-focused BrowsingContext in a cross-process way.

Even if we did know what the would-be-focused OOP BrowsingContext is for a given currently-inactive hierarchy, it's not clear to me if we have a mechanism that would allow for a JS-based onfocus handler on it.

From brief testing I just did, it looks like the events only fire on the lowest nested process and frame that is focused. It seems from the description above (I haven't tested) that focusedWindow is null in a document containing a iframe when that iframe is focused and out-of-process?

I believe so.

However testing in Chrome shows that document.hasFocus() returns true if any descendant iframe has focus. (so the HasFocus changes in this patch look correct from a brief glance).

Do we want to simply deprecate using focusedWindow and instead rely on using the focused browsing context instead?

The problem is: What would we compare it with? The problem isn't so much switching focusedWindow to a Fission-aware API but the window out param of getFocusedElementForWindow.

Do you have ideas of where to go from here?

Where is focusedWindow being accessed where the test failure happens?

From https://searchfox.org/mozilla-central/rev/5359952d8b0be3e706e8c943c2bef2674723b8a9/testing/mochitest/tests/SimpleTest/SimpleTest.js#994 called from https://searchfox.org/mozilla-central/rev/5359952d8b0be3e706e8c943c2bef2674723b8a9/testing/mochitest/tests/SimpleTest/SimpleTest.js#1087-1091 AFAICT.

The SpecialPowers.focus() probably isn't needed, unless window.focus() runs into priviledge problems maybe.

https://searchfox.org/mozilla-central/rev/5359952d8b0be3e706e8c943c2bef2674723b8a9/testing/mochitest/tests/SimpleTest/SimpleTest.js#1098 is needed per comment 41. However, changing desiredChildWindow to desiredWindow seems to work.

AFAICT, desiredChildWindow is needed for the addEventListener line:
https://searchfox.org/mozilla-central/rev/5359952d8b0be3e706e8c943c2bef2674723b8a9/testing/mochitest/tests/SimpleTest/SimpleTest.js#1094
...and I don't know if we have any Fission-compatible way to accomplish the same thing.

Flags: needinfo?(enndeakin)

How bad an idea would it be to introduce a new vendor-specific event enabled only when tests are running, let's say MozBrowsingContextFocus that would first fire right after focus on a DOM window that a focus event fires on and thereafter would asynchronously fire on each DOM window up the ancestor chain?

This way, the harness could listen for this event on desiredWindow without having to figure out which descendant the focus will be restored to.

In addition to Neil already being needinfoed, needinfoing Nika for additional Fission perspective and annevk in case I've missed some existing Web Platform capability.

Flags: needinfo?(nika)
Flags: needinfo?(annevk)

Given that we're running in a context with SpecialPowers already available, it seems like it might be nicer to bounce into the parent process with SpecialPowers.spawnChrome and wait for the focus to visibly change from there instead, so we don't need to add custom code for bubbling a MozBrowsingContextFocus event through the BC tree, and can just add a listener in one place. The parent process should also know more context about focus so can double-check state there as well.

Would something like that work here? I could totally be misunderstanding the problem in some way :-)

Flags: needinfo?(nika)

Thanks. I'll try that.

Also, one option might be polling document.hasFocus() from busy-repeating setTimeouts.

Let's see if polling hasFocus works. (I had a bit of trouble figuring out what exactly to do with spawnChrome):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e18b4e7463af2b0ddbfd3e7102def14e9ef0a179

I'm going to just rework waitForFocus/promiseFocus to work better with fission. I filed bug 1712900 for this.

Flags: needinfo?(enndeakin)
Depends on: 1712900

Not aware of any APIs beyond https://html.spec.whatwg.org/multipage/interaction.html#focus-management-apis (except for a couple of events that are largely duplicative of focus and blur).

Flags: needinfo?(annevk)

(In reply to Neil Deakin from comment #55)

I'm going to just rework waitForFocus/promiseFocus to work better with fission. I filed bug 1712900 for this.

Thanks.

I still tried hacking around this, but it's still not right:
https://treeherder.mozilla.org/jobs?repo=try&revision=ddbbe9abb8bb5d697465e6999335b3b71065f067&selectedTaskRun=eBzsXJbKRCyzzZVuIu3Eig.0

I'm suspending this bug while waiting for Neil's results from bug 1712900.

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d83f19a93b3
Make Document::HasFocus() use the BrowsingContext hierarchy. r=edgar

https://hg.mozilla.org/mozilla-central/rev/1d83f19a93b3

Status: REOPENED → RESOLVED
Closed: 3 years ago → 3 years ago
status-firefox91: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29251 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot

The patch landed in nightly and beta is affected.
:hsivonen, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(hsivonen)

Marking disabled on beta, since Fission is still disabled on beta. Also, this isn't really upliftable without bug 1712900.

status-firefox90: affected → disabled
Flags: needinfo?(hsivonen)

Thanks all, I can confirm the focus errors are gone and bug 1700781 is unblocked.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: