1597462 - Audit nsIDocShellTreeItem usage in mozilla::AccessibleCaretEventHub::Init + mozilla::AccessibleCaretEventHub::Terminate in layout/base/AccessibleCaretEventHub.cpp

Status: RESOLVED FIXED

(Core :: Layout, task, P2)

Description

[Kannan Vijayan [:djvj]]

In file layout/base/AccessibleCaretEventHub.cpp

DocShell tree used in both methods to register and de-register weak Scroll and Reflow observers on docshell and all ancestors, using DocShell tree.

The internal uses of the Event-hub all relate to a single manager (AccessibleCaretEventManager, uniquely owned by Hub).

AccessibleCaretEventHub hangs off of PresShell

• Created unconditionally for every PresShell:
• https://searchfox.org/mozilla-central/rev/2a10f4812f3f7c7645d253a4fe52f26bf58e20e8/layout/base/PresShell.cpp#961

Only user of Init is in nsCanvasFrame::CreateAnonymousContent

• https://searchfox.org/mozilla-central/rev/2a10f4812f3f7c7645d253a4fe52f26bf58e20e8/layout/generic/nsCanvasFrame.cpp#90

There’s already a note in the single caller by emilio indicating this should maybe be offloaded to a ScriptRunner.

Only user of Terminate is in PresShell::Destroy

Weak observers on parent doc-shells should be forwarded to chrome process when process boundaries are crossed in the tree.

Remote observers should communicate back to original process of PresShell when invoked.

Comment 1

[Chris Peterson [:cpeterson]]

Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).

Comment 2

[Chris Peterson [:cpeterson]]

Moving to Layout.

Please audit this use of the nsIDocShellTreeItem interface. With Fission enabled, Documents and nsDocShells for related frames, such as subframes and parent documents, may not be available within the current process and the corresponding nsIDocShellTreeItem methods will return null

If this code works as-is with Fission, we don't need to remove this usage of nsIDocShellTreeItem until when we remove nsIDocShellTreeItem entirely (bug 1607591) after we ship Fission MVP.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jwatt, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I can probably take a look at this later.

Comment 5

[Chris Peterson [:cpeterson]]

Auditing whether this use of nsIDocShellTreeItem breaks when Fission is enabled blocks Fission Nightly.

Comment 6

[Sean Voisen (:svoisen)]

Bumping to P2 since we should get to this soon. TYLin, just a nudge on that when you have a moment :)

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1597462 - Register observers for AccessibelCaretEventhub only on current docshell.

This effectively reverts Bug 1441279 Part 6.
https://hg.mozilla.org/mozilla-central/rev/eab2985673a5

The original patch's extend commit message describes that if a
AccessibleCaretEventHub registers scroll and reflow observers only on the leaf
docshell, it fails to update caret position when an ancestor iframe is
scrolled.

I think the above statement is incorrect. When scrolling an ancestor
iframe, the visible caret in an inner iframe should scroll with other
elements by virtue of APZ. Also, AccessibleCaret::SetPosition() itself
detects whether its position is changed relative to the top level
absolute position container (the moz-custom-content-container under
CanvasFrame). When scrolling an ancestor iframe, AccessibleCaret's
position in the inner iframe should remain static.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e0ce9bdf1f3
Register observers for AccessibelCaretEventhub only on current docshell. r=hiro

Comment 9

[Dorel Luca [:dluca]]

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