1597463 - Audit usage of nsIDocShellTreeItem in PresShell::GetParentPresShellForEventHandling

Status: RESOLVED FIXED

(Core :: DOM: Events, task, P2)

Description

[Kannan Vijayan [:djvj]]

In file layout/base/PresShell.cpp

https://searchfox.org/mozilla-central/rev/2a10f4812f3f7c7645d253a4fe52f26bf58e20e8/layout/base/PresShell.cpp#6229

Users are PresShell::GetRootWindow, and PresShell::EventHandler::HandleEvent

Used mainly to for event retargeting to parent PresShell.

Change to use BrowsingContext.

If parent is out-of-process DocShell, then forward event handling via IPC.

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

[Chris Peterson [:cpeterson]]

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

Comment 5

[Neha Kochar [:neha]]

Emilio, is this something you will be able to look into?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

This looks like relatively self-contained code a DOM::Events person should probably look at.

My understanding of this is that this is mostly trying to handle a rare error condition, and if there's no parent document we just don't fire the event (which seems fine if my understanding of how we dispatch events across processes works).

Masayuki, is that right? If so we could close this, or we could try to remove the error-handling case so that fission becomes more similar to non-fission... But not 100% sure how can we hit that other than maybe clicking on an iframe that removes itself or something...

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It was introduced by bug 573689. It was important pre-e10s world for making sure that any key events can be handled by UI in chrome. It seems that in e10s world, it's not required because if not dispatching keyboard events won't block the event propagation in the main process. I guess that keyboard input in OOP iframe shouldn't be propagated in parent of the iframe because of privacy reason. So, I guess that this handling is not required in release build (but perhaps, need to keep it for debugging with --disable-e10s).

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

It was introduced by bug 573689. It was important pre-e10s world for making sure that any key events can be handled by UI in chrome. It seems that in e10s world, it's not required because if not dispatching keyboard events won't block the event propagation in the main process. I guess that keyboard input in OOP iframe shouldn't be propagated in parent of the iframe because of privacy reason.

Confirmed with Edgar that it's discussed and reviewed when he fixed mouse enter/leave events dispatching in OOP iframe. The conclusion and current implementation is that key event in OOP iframes won't be propagated to parent.

So, I guess that this handling is not required in release build (but perhaps, need to keep it for debugging with --disable-e10s).

With the confirmation above, sounds we can close this bug?

Comment 9

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)

With the confirmation above, sounds we can close this bug?

If my understanding is correct, we should keep this behavior but replace the usage of nsIDocShellTreeItem with BrowsingContext. Take this bug.

Comment 10

[Edgar Chen [:edgar]]

Attachment: Bug 1597463 - Audit usage of nsIDocShellTreeItem in PresShell::GetParentPresShellForEventHandling;

Comment 11

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73f864b116f8eda5c6b975c8001df0839ffa4162&group_state=expanded

Comment 12

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da09c1a966d3faa5bb3220db6dcbd7f30e68e952

Comment 13

[Edgar Chen [:edgar]]

(In reply to Chris Peterson [:cpeterson] from comment #2)

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.

Given that this code works as-is with Fission, maybe we could move Fission Milestone to future?

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Edgar Chen [:edgar] from comment #13)

Given that this code works as-is with Fission, maybe we could move Fission Milestone to future?

Sure. In that case, this bug doesn't need to block shipping the Fission MVP. I'll move this bug to Fission Future as you suggest.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba8f984262e3
Audit usage of nsIDocShellTreeItem in PresShell::GetParentPresShellForEventHandling; r=masayuki

Comment 16

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ba8f984262e3