Closed Bug 1597463 Opened 1 year ago Closed 3 months ago

Audit usage of nsIDocShellTreeItem in PresShell::GetParentPresShellForEventHandling

Categories

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

task

Tracking

()

RESOLVED FIXED
81 Branch
Fission Milestone Future
Tracking Status
firefox81 --- fixed

People

(Reporter: djvj, Assigned: edgar)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

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.

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

Fission Milestone: --- → M6
Priority: -- → P3

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

Component: DOM: Navigation → Layout
Priority: P3 → --
Summary: Fix uses of mozilla::PresShell::GetParentPresShellForEventHandling in layout/base/PresShell.cpp → Audit uses of mozilla::PresShell::GetParentPresShellForEventHandling in layout/base/PresShell.cpp
Whiteboard: [rm-docshell-tree-item:hard] → [rm-docshell-tree-item:hard] [layout:triage-discuss]

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jwatt)

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

Fission Milestone: M6 → M6c

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

Fission Milestone: M6c → M6b
Flags: needinfo?(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...

Component: Layout → DOM: Events
Flags: needinfo?(masayuki)
Flags: needinfo?(jwatt)
Flags: needinfo?(emilio)

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

Flags: needinfo?(masayuki)
Whiteboard: [rm-docshell-tree-item:hard] [layout:triage-discuss] → [rm-docshell-tree-item:hard]

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

Flags: needinfo?(masayuki)
Flags: needinfo?(echen)

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

Assignee: nobody → echen
Status: NEW → RESOLVED
Closed: 3 months ago
Flags: needinfo?(masayuki)
Flags: needinfo?(echen)
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Severity: normal → S3
Type: defect → task
Priority: -- → P2
Summary: Audit uses of mozilla::PresShell::GetParentPresShellForEventHandling in layout/base/PresShell.cpp → Audit usage of nsIDocShellTreeItem in PresShell::GetParentPresShellForEventHandling

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

Flags: needinfo?(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.

Fission Milestone: M6b → M7
Flags: needinfo?(cpeterson)
Fission Milestone: M7 → Future
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba8f984262e3
Audit usage of nsIDocShellTreeItem in PresShell::GetParentPresShellForEventHandling; r=masayuki
Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
You need to log in before you can comment on or make changes to this bug.