1575051 - Fix usage of nsIDocShellTreeItem in nsDocShell::FindItemWithName

Status: RESOLVED FIXED

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

Description

[Kannan Vijayan [:djvj]]

https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2952

This bug relates to fixing both FindItemWithName as well as related methods that are used to find the given tree item, including DoFindItemWithName and FindChildWithName.

There are many issues in the implementation of this code. Firstly, the logic around the lookup for names _parent and _top is incorrect. It uses the in-process methods to retrieve the parent and root docshells, respectively, which will return wrong results in post-fission semantics (the parent may be null if out-of-process, and the root may be the wrong root if ancestors are out-of-process).

Additionally, the typical case for the query descends into the children of the docshell, which will only iterate across children that are in the same process.

All of the logic in this method, along with related sub-methods, needs to be changed to use BrowsingContext.

Comment 1

[Andreas Farre [:farre]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fc6d6bc84d32543183a07a057d7bfd36f9f9bdd

Comment 2

[Andreas Farre [:farre]]

This has been re-implemented in BrowsingContext::FindWithName, and it's just a matter of switching over to use it, and stop using nsDocShell::FindItemWithName.

Comment 3

[Andreas Farre [:farre]]

Attachment: Bug 1575051 - Part 1: Remove nsIDocShellTreeItem.findItemWithName. r=kmag

This also allows us to remove TabGroup::FindItemWithName, which is a
big step towards removing TabGroup entirely.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ebd1612a4ae
Remove nsIDocShellTreeItem.findWithName. r=kmag

Comment 5

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 0ebd1612a4ae (bug 1575051) for gv-junit crashes and bc failures on browser_browsingContext-02.js.

Backout:

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=0ebd1612a4aec7bde54aea243ae5b526a7d9a094&selectedJob=268140623

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

Failure log gv-junit:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=268144965&repo=autoland&lineNumber=1454

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d380479464e7
Backed out changeset 0ebd1612a4ae for gv-junit crashes and bc failures on browser_browsingContext-02.js. CLOSED TREE

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c385531b4ee3
Remove nsIDocShellTreeItem.findItemWithName. r=kmag

Comment 8

[Andreas Farre [:farre]]

First failure (browser_browsingContext-02) is due to this patch. Silly mistake, now corrected.
Second (gv-junit) is unrelated.

Trying a reland.

Comment 9

[Cristian Brindusan [:cbrindusan]]

Backed out 2 changesets (Bug 1582716, Bug 1575051) for gv-junit failures, new exception.

Backout: https://hg.mozilla.org/integration/autoland/rev/da9b99b612612970527222d0285ed4f729b5e0e4

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=c385531b4ee3f790face699dcfb5471253c8a2a6&selectedJob=268168178

Range of retriggers:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Copt%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-junit-e10s%2C%28gv-junit%29&fromchange=eb8d46c7826b5209d9b3a7c83f453d033c643742&tochange=4ae54d71bad082fe2ebba99a407f027f68628f73

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

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

Another failure which seems to have started from these changes:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=268211478&repo=autoland&lineNumber=1322

[task 2019-09-24T18:13:22.106Z] 18:13:22 INFO - TEST-START | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js
[task 2019-09-24T18:13:23.066Z] 18:13:23 INFO - TEST-INFO | started process screenshot
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - TEST-INFO | screenshot: exit 0
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - Buffered messages logged at 18:13:22
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - Entering test bound
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - Buffered messages finished
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | 65536 == 0 - got 65536, expected 0 (operator ==)
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - Stack trace:
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - testStates@chrome://mochitests/content/browser/accessible/tests/browser/states/head.js:57:14
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - execute@resource://specialpowers/SpecialPowersSandbox.jsm:87:12
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - _spawnTask@resource://specialpowers/SpecialPowersChild.jsm:1691:15
[task 2019-09-24T18:13:23.167Z] 18:13:23 INFO - receiveMessage@resource://specialpowers/SpecialPowersChild.jsm:285:21
[task 2019-09-24T18:13:23.168Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | true == true -
[task 2019-09-24T18:13:23.168Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | 65536 == true -
[task 2019-09-24T18:13:23.168Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | true == true -
[task 2019-09-24T18:13:23.168Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | 0 == 0 -
[task 2019-09-24T18:13:23.168Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | true == true -
[task 2019-09-24T18:13:23.227Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | 0 == 0 -
[task 2019-09-24T18:13:23.227Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | true == true -
[task 2019-09-24T18:13:23.227Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | 65536 == true -
[task 2019-09-24T18:13:23.227Z] 18:13:23 INFO - TEST-PASS | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | true == true -
[task 2019-09-24T18:13:23.289Z] 18:13:23 INFO - Leaving test bound
[task 2019-09-24T18:13:23.290Z] 18:13:23 INFO - GECKO(768) | [Parent 8376, Gecko_IOThread] WARNING: pipe error: 109: file z:/task_1569327007/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 341
[task 2019-09-24T18:13:23.290Z] 18:13:23 INFO - GECKO(768) | [Parent 8376, Gecko_IOThread] WARNING: pipe error: 109: file z:/task_1569327007/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 341
[task 2019-09-24T18:13:23.290Z] 18:13:23 INFO - GECKO(768) | [Child 6832, Chrome_ChildThread] WARNING: pipe error: 109: file z:/task_1569327007/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 341
[task 2019-09-24T18:13:23.290Z] 18:13:23 INFO - GECKO(768) | [Child 6832, Chrome_ChildThread] WARNING: pipe error: 109: file z:/task_1569327007/build/src/ipc/c[Child 9980, Chrome_ChildThread] WARNING: pipe error: 109: file z:/task_1569327007/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 341
[task 2019-09-24T18:13:23.290Z] 18:13:23 INFO - GECKO(768) | [Child 9980, Chrome_ChildThread] WARNING: pipe error: 109: file z:/task_1569327007/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 341
[task 2019-09-24T18:13:23.348Z] 18:13:23 INFO - GECKO(768) | MEMORY STAT | vsize 2104143MB | vsizeMaxContiguous 66089885MB | residentFast 284MB | heapAllocated 127MB
[task 2019-09-24T18:13:23.348Z] 18:13:23 INFO - TEST-OK | accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js | took 1242ms

Comment 11

[Andreas Farre [:farre]]

Attachment: Bug 1575051 - Part 2: Look in chrome browsing context group when docshell is missing. r=kmag

Also some minor cleanup in nsWindowWatcher, as well as a small fix,
where GetWindowByName forgot to addref its return value (as changed in
Part 1).

Comment 12

[Andreas Farre [:farre]]

Attachment: Bug 1575051 - Part 3: Test that nsWindowWatcher can find windows. r=kmag

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c8f70f9328b
Part 1: Remove nsIDocShellTreeItem.findItemWithName. r=kmag
https://hg.mozilla.org/integration/autoland/rev/e01256038537
Part 2: Look in chrome browsing context group when docshell is missing. r=kmag
https://hg.mozilla.org/integration/autoland/rev/67aaf4a157af
Part 3: Test that nsWindowWatcher can find windows. r=kmag

Comment 14

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on browser_browsingContext-getWindowByName.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/155063a2df5a954bebbc434dd429c1f3a04df6d1
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=271155607&repo=autoland&lineNumber=18746

Comment 15

[Boris Zbarsky [:bzbarsky]]

So I don't understand the GetEntryGlobal() bit in the patch. It used to be used only in some situations but not others, but now is getting used across the board? Why does that not change observable behavior?

Comment 16

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)

So I don't understand the GetEntryGlobal() bit in the patch. It used to be used only in some situations but not others, but now is getting used across the board? Why does that not change observable behavior?

The only place where we didn't do this before was from PerformRetargeting, and the only time it should make a difference in that case is when there's a JS caller on the stack during a link click or form submission that has a different global from the document the link/form is in.

We discussed all of these scenarios pretty thoroughly during our Berlin work week after we decided to remove TabGroups, but I was pretty jet lagged at the time, so I don't remember all of the details, and I may have missed something here. I'm still pretty sure this is correct for ordinary link clicks, since those are always dispatched async, and enter the realm of the global that owns the link element before loading. For forms, I'm less certain after re-checking, and it may be possible to trigger slightly different behavior by calling form.submit() on an element in a different frame, if it names a target that the caller frame has access to but the frame containing the form does not. It would have to be a pretty odd corner case, though.

Either way, it might be better to just add a flag ignore the current entry global when called from PerformRetargeting, but still keep the lookup logic in FindWithName so that it doesn't need to be duplicated in so many places.

Comment 17

[Andreas Farre [:farre]]

Attachment: Bug 1575051 - Part 4: Expose JS stack access check control on FindWithName. r=peterv

Comment 18

[Andreas Farre [:farre]]

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

Either way, it might be better to just add a flag ignore the current entry global when called from PerformRetargeting, but still keep the lookup logic in FindWithName so that it doesn't need to be duplicated in so many places.

So I did this, see Part 4.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93dabea6010a
Part 1: Remove nsIDocShellTreeItem.findItemWithName. r=kmag
https://hg.mozilla.org/integration/autoland/rev/e98fdadada24
Part 2: Look in chrome browsing context group when docshell is missing. r=kmag
https://hg.mozilla.org/integration/autoland/rev/1fa048a91fb9
Part 3: Test that nsWindowWatcher can find windows. r=kmag
https://hg.mozilla.org/integration/autoland/rev/6b3edf5e5ad0
Part 4: Expose JS stack access check control on FindWithName. r=peterv

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/93dabea6010a
https://hg.mozilla.org/mozilla-central/rev/e98fdadada24
https://hg.mozilla.org/mozilla-central/rev/1fa048a91fb9
https://hg.mozilla.org/mozilla-central/rev/6b3edf5e5ad0