1646540 - [meta] Audit uses of GetPrivate(Parent|Root)

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, task, P3)

Description

[Kris Maglione [:kmag]]

+++ This bug was initially created as a clone of Bug #1642433 +++

Similar to uses of nsIDocShellTreeItem, many/most of the callers of these methods will probably do the wrong thing under Fission with remote frames.

Comment 1

[Neha Kochar [:neha]]

Don't know how this missed our triage till now :(

Comment 2

[Chris Peterson [:cpeterson]]

Fission M7a

Comment 3

[Neha Kochar [:neha]]

Andreas, can you please pick this up after session storage work?

Comment 4

[Neha Kochar [:neha]]

Henri said he can help out with this one so assigning to him.

Comment 5

[Henri Sivonen (:hsivonen)]

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsWindowRoot.cpp#276
https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsWindowRoot.cpp#345
Copypaste dispatch works on the high level, which suggest these only uses of GetPrivateParent are OK.

Comment 6

[Henri Sivonen (:hsivonen)]

GetPrivateRoot:

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsGlobalWindowOuter.cpp#2111
This attaches scrolling and copypaste key listeners. These functions seems to work, so assuming this is OK.

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsGlobalWindowOuter.cpp#4044
This is a layout flush for bug 25117 purposes. This being process-scoped seems reasonable enough.

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsGlobalWindowOuter.cpp#5662
https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/layout/base/PresShell.cpp#6558
These open up a new thing to audit: GetTopWindowRoot and nsPresShell::GetRootWindow.

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsGlobalWindowOuter.cpp#6536
https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/events/KeyEventHandler.cpp#297
Copypaste generally works, which suggests these are OK.

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsGlobalWindowOuter.cpp#6763
https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsGlobalWindowOuter.cpp#3004
Focus ring painting generally works, which suggests these are OK.

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/docshell/base/nsDocShellTreeOwner.cpp#76
This seems central enough that if this was broken, thing should be very visibly bad, but needinfoing smaug to confirm that this is OK.

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/dom/base/nsFocusManager.cpp#444
This seems to raise the puppet widget as appropriate, and the result is generally OK to the extent the concept of raising the puppet widget makes sense.

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/layout/printing/nsPrintJob.cpp#2416
If printing works, this has to be OK.

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/layout/xul/nsXULPopupManager.cpp#448
I believe this is now parent-only code and doesn't involve Fission issues.

Comment 7

[Henri Sivonen (:hsivonen)]

GetTopWindowRoot:

https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/docshell/base/nsDocShell.cpp#12550
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/base/nsGlobalWindowOuter.cpp#6527
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/commandhandler/nsCommandManager.cpp#234
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/events/KeyEventHandler.cpp#270
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/editor/libeditor/EditorEventListener.cpp#68
Copypaste generally works, which suggests these are OK.

https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/base/Document.cpp#13461
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/layout/base/nsDocumentViewer.cpp#2698
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/layout/xul/nsMenuPopupFrame.cpp#992
XUL pop-ups only, which are now parent-only.

https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/base/nsContentUtils.cpp#8191
Usage looks either parent-only or Fission-specific (so presumably Fission-compatible).

https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/base/nsGlobalWindowInner.cpp#3806
Used for deciding to show focus ring, which seems to work.

https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/base/nsGlobalWindowOuter.cpp#7370
Dead code?

https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/ipc/BrowserParent.cpp#562
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/ipc/BrowserParent.cpp#577
Fission-specific and, therefore, presumably Fission-compatible.

https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/webauthn/WebAuthnManagerBase.cpp#88
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/webauthn/WebAuthnManagerBase.cpp#112
WebAuthN seems to work with Fission, so probably OK, but I haven't really examined if WebAuthN works from OOP iframes.

Comment 8

[Henri Sivonen (:hsivonen)]

PresShell::GetRootWindow:

This seems to be used for event targeting, so I think the task of auditing this one is a duplicate of bug 1541589, which is FIXED.

Comment 9

[Henri Sivonen (:hsivonen)]

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

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/layout/printing/nsPrintJob.cpp#2416
If printing works, this has to be OK.

Possibly related bug already on file: bug 1700379.

Comment 10

[Henri Sivonen (:hsivonen)]

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

https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/webauthn/WebAuthnManagerBase.cpp#88
https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/dom/webauthn/WebAuthnManagerBase.cpp#112
WebAuthN seems to work with Fission, so probably OK, but I haven't really examined if WebAuthN works from OOP iframes.

Bug 1402114 indicates that WebAuthN isn't supposed to work in iframe without opt-in. The opt-in is bug 1460986, which isn't fixed for non-Fission, either.

Comment 11

[Henri Sivonen (:hsivonen)]

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

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/docshell/base/nsDocShellTreeOwner.cpp#76
This seems central enough that if this was broken, thing should be very visibly bad, but needinfoing smaug to confirm that this is OK.

This is for handling drag&drop and for handling tooltips. I'll check those.

Comment 12

[Henri Sivonen (:hsivonen)]

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

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

https://searchfox.org/mozilla-central/rev/083983b7f09b00cdfe4f70396e39ea4f8a1735e1/docshell/base/nsDocShellTreeOwner.cpp#76
This seems central enough that if this was broken, thing should be very visibly bad, but needinfoing smaug to confirm that this is OK.

This is for handling drag&drop and for handling tooltips. I'll check those.

Tooltips works. Dragging works. Dropping works.

Apart possibly from the more precise bugs already on file, this stuff looks OK, so marking WFM.