Document::GetSubDocumentFor(nsIContent* aContent) is mainly used in nsFocusManager but also a few other places. It should be looked at to decide if anything needs to change for Fission.

Its use in inLayoutUtils::GetSubDocumentFor was already looked at in Bug 1591156.

I took a look over the remaining callers:

Working

• /dom/base/nsContentUtils.cpp#6352

  • Should be fine, since subdocuments in another process will return before this is hit.
    • https://searchfox.org/mozilla-central/rev/9f76a47f4aa935b49754c5608a1c8e72ee358c46/dom/base/nsContentUtils.cpp#6352

• /dom/base/nsObjectLoadingContent.cpp#2270

  • This should be fine, intended to return null for cross-origin.

• /layout/base/nsDocumentViewer.cpp#651

  • Should be fine, since GetInProcessParent is used above, and GetSubDocumentFor(...) is called from that document.

Broken / Unsure

• /dom/base/nsFocusManager.cpp#887

  • Specifically called out as not working with Fission in the comment above it. Links to a bug that is closed though... At minimum the comment needs to be changed, but maybe more code changes is required?
  • https://bugzilla.mozilla.org/show_bug.cgi?id=1613054

• /dom/base/nsFocusManager.cpp#272

  • Will not function properly with Fission. There are only 3 callers but my experience with Focus is too minimal to tell if they need fixing.

• /dom/base/nsFocusManager.cpp#4399

  • Won't work with an OOP subdocument. Callers might be fine though, someone else will need to verify.

• /dom/base/nsFocusManager.cpp#4633

  • Unsure

• /accessible/generic/OuterDocAccessible.cpp#55

  • Appears to be broken, but maybe something is handling creation in the other process. Don't know enough about Accessible to tell.

(In reply to Steven MacLeod [:smacleod] from comment #1)

• /dom/base/nsFocusManager.cpp#887

  • Specifically called out as not working with Fission in the comment above it. Links to a bug that is closed though... At minimum the comment needs to be changed, but maybe more code changes is required?
  • https://bugzilla.mozilla.org/show_bug.cgi?id=1613054

• /dom/base/nsFocusManager.cpp#272

  • Will not function properly with Fission. There are only 3 callers but my experience with Focus is too minimal to tell if they need fixing.

• /dom/base/nsFocusManager.cpp#4399

  • Won't work with an OOP subdocument. Callers might be fine though, someone else will need to verify.

• /dom/base/nsFocusManager.cpp#4633

  • Unsure

:hsivonen, might you be able to weigh in on these focus related uses?

(In reply to Steven MacLeod [:smacleod] from comment #1)

• /accessible/generic/OuterDocAccessible.cpp#55
  • Appears to be broken, but maybe something is handling creation in the other process. Don't know enough about Accessible to tell.

Neha, do you know who might be able to weigh in on this usage?

Jamie, please can you please review the usage of Document::GetSubDocumentFor at /accessible/generic/OuterDocAccessible.cpp#55 ?

(In reply to Steven MacLeod [:smacleod] from comment #1)

• /accessible/generic/OuterDocAccessible.cpp#55
  • Appears to be broken, but maybe something is handling creation in the other process. Don't know enough about Accessible to tell.

This is already working as expected. In the OOP iframe document process, the document is seen as an in-process top level document and a RootAccessible is created accordingly (there are several code paths that might trigger this). The parent process is then notified. In the parent process, the document accessible is linked into the tree based on the embedder accessible sent via PBrowserBridge.

(In reply to Steven MacLeod [:smacleod] from comment #2)

(In reply to Steven MacLeod [:smacleod] from comment #1)

• /dom/base/nsFocusManager.cpp#887
  • Specifically called out as not working with Fission in the comment above it. Links to a bug that is closed though... At minimum the comment needs to be changed, but maybe more code changes is required?
  • https://bugzilla.mozilla.org/show_bug.cgi?id=1613054

Oops. Needs comment change.

• /dom/base/nsFocusManager.cpp#272
  • Will not function properly with Fission. There are only 3 callers but my experience with Focus is too minimal to tell if they need fixing.

Not sure about nsFocusManager::SetFocusedWindowWithCallerType usage, but the other two look familiar enough as instances where other code deals with Fission cases.

• /dom/base/nsFocusManager.cpp#4399
  • Won't work with an OOP subdocument. Callers might be fine though, someone else will need to verify.

Tabbing into and out of OOP iframes works.

• /dom/base/nsFocusManager.cpp#4633
  • Unsure

This also looks related to navigation by the tab key, and that works with OOP iframes.

With the possible exception of nsFocusManager::SetFocusedWindowWithCallerType, it looks like only a comment change is needed for these.

Assigning to Henri as the only remaining possible fixing is in focus code.

Neil, can you suggest a test case the exercises the need to call ClearFocus here?
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/base/nsFocusManager.cpp#433-441

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

Neil, can you suggest a test case the exercises the need to call ClearFocus here?
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/base/nsFocusManager.cpp#433-441

This codepath would occur if someone sets the focused window to a top-level window using this method, while a child frame is focused. In a chome context, for example, setting the focused window to the main browser window while something in the sidebar is focused.

There's probably already a test that calls this code in https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/tests/mochitest/chrome/window_focus.xhtml#1336

I don't know if there is a specific test that does this in a child process though.

This also looks related to navigation by the tab key, and that works with OOP iframes.

This might affect tabbing into documents that have design mode, content editable root elements, or have framesets though. The main focus test (window_focus.xhtml) was never updated for e10s let alone fission, so we should verify the behaviour here.

(In reply to Neil Deakin from comment #9)

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

Neil, can you suggest a test case the exercises the need to call ClearFocus here?
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/base/nsFocusManager.cpp#433-441

window_focus.html doesn't run that line at all, AFAICT. It comes to the content null check twice, and content null on both occasions. The first JS stack is:

0 synthesizeMouseAtPoint(left = "104", top = "140", aEvent = "[object Object]", aWindow = "[object Window]") ["chrome://mochikit/content/tests/SimpleTest/EventUtils.js":590:12]
this = [object Window]
1 synthesizeMouse(aTarget = "[object XULElement]", aOffsetX = "4", aOffsetY = "4", aEvent = "[object Object]", aWindow = "[object Window]") ["chrome://mochikit/content/tests/SimpleTest/EventUtils.js":508:9]
this = [object Window]
2 mouseOnElement/<() ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":318:42]
3 expectFocusShift(callback = [function], expectedWindow = "[object Window]", expectedElement = "null", focusChanged = "true", testid = ""mouse on unfocusable element n1"") ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":180:2]
this = [object Window]
4 mouseOnElement(element = "[object XULElement]", expectedElement = "null", focusChanged = "true", testid = ""mouse on unfocusable element n1"") ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":318:20]
this = [object Window]
5 startTest("[object Window]") ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":479:18]
this = [object Window]
6 SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<() ["chrome://mochikit/content/tests/SimpleTest/SimpleTest.js":1037:22]
this = [object Object]

The second JS stack is:

0 synthesizeMouseAtPoint(left = "114", top = "140", aEvent = "[object Object]", aWindow = "[object Window]") ["chrome://mochikit/content/tests/SimpleTest/EventUtils.js":590:12]
this = [object Window]
1 synthesizeMouse(aTarget = "[object XULElement]", aOffsetX = "4", aOffsetY = "4", aEvent = "[object Object]", aWindow = "[object Window]") ["chrome://mochikit/content/tests/SimpleTest/EventUtils.js":508:9]
this = [object Window]
2 mouseOnElement/<() ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":318:42]
3 expectFocusShift(callback = [function], expectedWindow = "[object Window]", expectedElement = "null", focusChanged = "true", testid = ""mouse on unfocusable element n3"") ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":180:2]
this = [object Window]
4 mouseOnElement(element = "[object XULElement]", expectedElement = "null", focusChanged = "true", testid = ""mouse on unfocusable element n3"") ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":318:20]
this = [object Window]
5 startTest("[object Window]") ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":479:18]
this = [object Window]
6 SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<() ["chrome://mochikit/content/tests/SimpleTest/SimpleTest.js":1037:22]
this = [object Object]

window_focus.xhtml calls ClearFocus once with a null window argument by invoking ClearFocus directly via XPConnect with JS stack:

0 startTest("[object Window]") ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":745:7]
this = [object Window]
1 SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<() ["chrome://mochikit/content/tests/SimpleTest/SimpleTest.js":1037:22]
this = [object Object]

window_focus.xhtml also calls SetFocusedWindow once with a null window argument by invoking the method directly via XPConnect with JS stack:

0 doFrameSwitchingTests("[object FocusEvent]") ["chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/window_focus.xhtml":1496:4]
this = [object Window]

Ah. theses are deliberate tests for null values.

I think window_focus.xhtml doesn't test the remaining question in this bug.

Let's try to see if we have a test that would want to call ClearFocus on a window but we only have a BrowsingContext available:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e74621a68c687980e93063f04983b743d8d9b

AFAICT, our test suite has no test that exercises the line:
https://searchfox.org/mozilla-central/rev/2f1a015b004b79f1145c81cdf86b15481a5630e2/dom/base/nsFocusManager.cpp#440

Also, I tried to focus and blur the sidebar in an rr recording, and Pernosco says that code didn't run.

Neil, do you have a more specific suggestion for exercising that line?

I don't know if there is any codepath that actually calls that at the moment. Most callers would focus a window with window.focus() rather than setting the focusedWindow directly.

However, I could get it to be called by first focusing the textbox in the bookmarks sidebar. Then either:

A. Using the browser console, enter Services.focus.focusedWindow = window

B. Using browser devtools, modify one of the browser toolbar buttons to have -moz-user-focus: none, then click on it.

(In reply to Neil Deakin from comment #18)

I don't know if there is any codepath that actually calls that at the moment. Most callers would focus a window with window.focus() rather than setting the focusedWindow directly.

However, I could get it to be called by first focusing the textbox in the bookmarks sidebar. Then either:

A. Using the browser console, enter Services.focus.focusedWindow = window

B. Using browser devtools, modify one of the browser toolbar buttons to have -moz-user-focus: none, then click on it.

Thanks. I have Pernosco traces pending for those scenarios.

Meanwhile, I loaded OOP-iframe-containing Web content in the sidebar using the Side View extensions, and clicking between form fields there and in the main content area doesn't end up executing nsFocusManager::SetFocusedWindowWithCallerType at all.

(In reply to Neil Deakin from comment #18)

A. Using the browser console, enter Services.focus.focusedWindow = window

This ends up running the else branch here:
https://searchfox.org/mozilla-central/rev/2f1a015b004b79f1145c81cdf86b15481a5630e2/dom/base/nsFocusManager.cpp#596

Is that OK? If it is, let's close this with a comment-only patch.

B. Using browser devtools, modify one of the browser toolbar buttons to have -moz-user-focus: none, then click on it.

I couldn't repro this way.

B. Using browser devtools, modify one of the browser toolbar buttons to have -moz-user-focus: none, then click on it.

I couldn't repro this way.

For me, it calls into ClearFocus and then goes into the main branch (not the else clause) and calls Blur and Focus.

That said, is the issue here just wanting to know if the GetContentWindow() call is safe to use with cross-origin frames?

If so, then the call at:

https://searchfox.org/mozilla-central/rev/1df3b4b4d27d413675fb66f375230cb25d636450/dom/base/nsFocusManager.cpp#439

only wants to know if the focused element is a frame or not. I'd suggest changing that caller to just QI to nsFrameLoaderOwner instead.

No test, because the expectation is that this code is not actually exercised anyway.

Try run to check that the change doesn't make the line that previously wasn't run by the test suite run now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cf11635e4bf57ebd9a6c58f6f384c57717fc3ec

Backed out for causing reftest failures

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

Push with failures

Failure log

The problem is in the parent process. I have trouble seeing any difference between the reftest images.

Neil, given that this patch causes a failure, should we just not change the code? Or should we special-case the parent like in so many other focus issues? How realistic is it for XPConnect JS to run this in the content processes?

Trying special-casing the parent:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c74a24e1bbf77fee46010170530b891d9fd51a2

I'd be ok if we just removed the ClearFocus line and surrounding code as well. Comment 17 seems to imply the code is never run. The documentation for focusedWindow doesn't say the focus should be cleared in the window, and since the code only seems to do this for top-level windows it would be more consistent to have it either done for all windows or none of them.

(In reply to Neil Deakin from comment #28)

I'd be ok if we just removed the ClearFocus line and surrounding code as well. Comment 17 seems to imply the code is never run. The documentation for focusedWindow doesn't say the focus should be cleared in the window, and since the code only seems to do this for top-level windows it would be more consistent to have it either done for all windows or none of them.

Thanks. Try run of that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8c7b7193c04a92cbcafc27483dc4be070d379df

Looks OK. I'll put it on autoland.

Backed out changeset 5d7dec65aced (Bug 1681983) for causing mochitest failures in test_focus.xhtml.
https://hg.mozilla.org/integration/autoland/rev/fc5287473e3e60e8561ee569c7615685542cbc74

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=5d7dec65acedaad1e4c4adff061536ca85403231&selectedTaskRun=HGJDi7vbTDmwyTnMrg_azg.0

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

Strange. How was the failure from comment 31 not caught in comment 17?

Hopefully a comprehensive enough try run for the parent special-casing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92dc95e668a4847b37b6374362adfe0805519a61

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

Hopefully a comprehensive enough try run for the parent special-casing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92dc95e668a4847b37b6374362adfe0805519a61

The try run looks like the patch didn't break stuff.

Neil, should I land the patch currently on Phab or land a patch that only has the comment change and doesn't change the ClearFocus code path at all?

Considering how many much time this has taken, and how many backouts has occurred, I would consider changing as little code as possible.

https://hg.mozilla.org/mozilla-central/rev/8e65f5fd433b