Closed Bug 1646540 Opened 11 months ago Closed 7 days ago

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

Categories

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

task

Tracking

()

RESOLVED WORKSFORME
Fission Milestone M7a

People

(Reporter: kmag, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

(Keywords: meta)

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

Summary: [meta] Audit uses of GetPrivate(|Scriptable)(Parent|Root) → [meta] Audit uses of GetPrivate(Parent|Root)

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

Fission Milestone: --- → ?

Fission M7a

Fission Milestone: ? → M7a

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

Flags: needinfo?(afarre)

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

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(afarre)

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.

Flags: needinfo?(bugs)

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.

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.

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

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

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

Flags: needinfo?(bugs)

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

Status: ASSIGNED → RESOLVED
Closed: 7 days ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.