1696908 - Enforce focused BrowsingContext ordering by action id

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

We know that out-of-order attempts to set the active BrowsingContext must be rejected by action id or things go wrong.

While we don't have a concrete demonstration of things going wrong in an analogous way with the focused BrowsingContext it seems prudent to implement the same mechanism for focused BrowsingContext in the hope of solving mystery problems in this general area.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1696908 - Ensure parent-managed order of setting the focused browsing context.

Comment 2

[Henri Sivonen (:hsivonen)]

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

Comment 3

[Henri Sivonen (:hsivonen)]

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

Comment 4

[Henri Sivonen (:hsivonen)]

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

Comment 5

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

I found bug 1695135 is fixed by this, Henri, do you want to add the test in bug 1695135 here? Or I could post a patch for the test in bug 1695135 and land it there.

Comment 6

[Henri Sivonen (:hsivonen)]

Let's land the test as part of that other bug.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e072b56f8518
Ensure parent-managed order of setting the focused browsing context. r=nika

Comment 8

[Alexandru Michis [:malexandru]]

Backed out changeset e072b56f8518 (bug 1696908) for causing bc failures in browser_bug1303838.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/0fbb2d7f53d500f374dc43465fbb8308827f99e3
Push with failures, failure log.

(Update): There were also bc failures caused by these changes.
(Update 2): Also looks to have caused mochitest plain failures.

Comment 9

[Henri Sivonen (:hsivonen)]

Hmm. Looks like all the failures were previously-known intermittents.

Comment 10

[Henri Sivonen (:hsivonen)]

It looks like we reject nsFocusManager::SetFocusedWindowInternal due to stale action id:
https://pernos.co/debug/bdobZfU_AhZJAcJOM8bfNA/index.html#f{m[AfLx,CFKC_,t[Zw,CKg_,f{e[AfLx,CFHM_,s{af4284PAA,bAXo,uAWxF1A,oAW5ILg___

For some reason, that Pernosco trace is broken in the sense that local variable cannot be inspected, so it's hard to make progress on getting closer to the root cause before that's resolved.

Comment 11

[Henri Sivonen (:hsivonen)]

Found this comment:

    // Sync the window for a newly-created OOP iframe
    // The actionID will be unused, so pass 0 to trigger assertions
    // in case code is later changed to use it accidentally.

And this patch changed the code that way.

Comment 12

[Henri Sivonen (:hsivonen)]

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

Comment 13

[Henri Sivonen (:hsivonen)]

Findings so far:

The assumption that for a given action id there is at most one focused BrowsingContext may be violated when we focus a remote iframe: In that case, we may set a focused BrowsingContext in the process hosting the iframe element and a second BrowsingContext in the process hosting the iframe contents.

Comment 14

[Henri Sivonen (:hsivonen)]

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

Comment 15

[Henri Sivonen (:hsivonen)]

It seems that the problem here is that a content process starts focusing stuff due to a load finishing before a parent-initiated focusing process happens, but the IPC message that makes the content process-initiated action take its place in the queue happens after the parent-initiated action has taken its place in the queue. This leads to the content process-initiated focusing messing up the parent-initiated focusing action (which takes many IPC round trips to complete.)

One option to explore would be making parent-initiated actions always complete.

Comment 16

[Henri Sivonen (:hsivonen)]

I think I'm going to still pursue examining the test itself. Adding more logging makes the failure happen less often...

Comment 17

[Henri Sivonen (:hsivonen)]

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

When opening links that contain iframes in a background tab, the focusing of the iframe races with chrome-initiated focusing and I failed to come up with a nice way to make the test wait for the iframe getting focused first.

Comment 18

[Henri Sivonen (:hsivonen)]

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

Comment 19

[Henri Sivonen (:hsivonen)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27d3d97f875f
Ensure parent-managed order of setting the focused browsing context. r=nika

Comment 21

[Andreea Pavel [:apavel]]

Backed out for build bustage at nsFocusManager.cpp

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=cDHG3rTYQuWXbQmC9mec7A.1&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=8119f4fe4e541911842ecf511ebd03932a88c177

Failure log: https://treeherder.mozilla.org/logviewer?job_id=335050304&repo=autoland&lineNumber=33535

Backout: https://hg.mozilla.org/integration/autoland/rev/02618acdb118bf277fcc3302980e2b52c3513734

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/562969cdeb6d
Ensure parent-managed order of setting the focused browsing context. r=nika

Comment 23

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/562969cdeb6d