Closed Bug 1575051 Opened 5 months ago Closed 3 months ago

Fix usage of nsIDocShellTreeItem in nsDocShell::FindItemWithName

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla72
Fission Milestone M4
Tracking Status
firefox72 --- fixed

People

(Reporter: djvj, Assigned: farre)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [rm-docshell-tree-item:hard])

Attachments

(4 files)

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.

Component: DOM: Core & HTML → Document Navigation
Fission Milestone: --- → M5
Priority: -- → P2
Depends on: 1561715
Blocks: 1561715
No longer depends on: 1561715

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.

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

Depends on: 1553804
Attachment #9093576 - Attachment description: Bug 1575051 - Remove nsIDocShellTreeItem.findWithName → Bug 1575051 - Remove nsIDocShellTreeItem.findWithName.
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ebd1612a4ae
Remove nsIDocShellTreeItem.findWithName. r=kmag
Backout by cbrindusan@mozilla.com:
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
Attachment #9093576 - Attachment description: Bug 1575051 - Remove nsIDocShellTreeItem.findWithName. → Bug 1575051 - Remove nsIDocShellTreeItem.findItemWithName.
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c385531b4ee3
Remove nsIDocShellTreeItem.findItemWithName. r=kmag

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

Trying a reland.

Flags: needinfo?(afarre)
Fission Milestone: M5 → M4

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

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).

Attachment #9093576 - Attachment description: Bug 1575051 - Remove nsIDocShellTreeItem.findItemWithName. → Bug 1575051 - Part 1: Remove nsIDocShellTreeItem.findItemWithName.
Whiteboard: [rm-docshell-tree-item:hard]
Pushed by afarre@mozilla.com:
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

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?

(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.

(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.

Flags: needinfo?(afarre)
Blocks: 1590782
Attachment #9093576 - Attachment description: Bug 1575051 - Part 1: Remove nsIDocShellTreeItem.findItemWithName. → Bug 1575051 - Part 1: Remove nsIDocShellTreeItem.findItemWithName. r=kmag
Attachment #9100475 - Attachment description: Bug 1575051 - Part 2: Look in chrome browsing context group when docshell is missing. → Bug 1575051 - Part 2: Look in chrome browsing context group when docshell is missing. r=kmag
Attachment #9100476 - Attachment description: Bug 1575051 - Part 3: Test that nsWindowWatcher can find windows. → Bug 1575051 - Part 3: Test that nsWindowWatcher can find windows. r=kmag
Attachment #9101265 - Attachment description: Bug 1575051 - Part 4: Expose JS stack access check control on FindWithName. → Bug 1575051 - Part 4: Expose JS stack access check control on FindWithName. r=peterv
Pushed by afarre@mozilla.com:
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
Blocks: 1592290
You need to log in before you can comment on or make changes to this bug.