1597479 - Audit nsIDocShellTreeItem usage in nsLayoutUtils::GetDeviceContextForScreenInfo in layout/base/nsLayoutUtils.cpp

Status: NEW

(Core :: Layout, defect, P2)

Description

[Kannan Vijayan [:djvj]]

In file layout/base/nsLayoutUtils.cpp

Called from nsScreen exclusively

• nsScreen seems to be the “screen” script object.
• GetPixelDepth, GetRect, GetAvailRect

These are all rare APIs that should not need to be fast.

The implementation walks up the docshell tree, stopping if a window-less docshell is found.

For windowed docshells, it calls EnsureSizeAndPositionUpToDate, and if a PresContext->DeviceContext is found, returns it (breaking the walk up the tree).

Change to use BrowsingContext.

At users (GetPixelDepth, GetRect, GetAvailRect on nsScreen), detect walk out of process and issue IPC to chrome to retrieve the value.

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

[Jonathan Watt [:jwatt]]

Emily, would you mind taking a look at this? It would be good for you to have some knowledge of place BrowsingContext plays in our code. :)

Comment 5

[Chris Peterson [:cpeterson]]

Tentatively assigning this bug to Emily so I know which Fission bugs are being investigated.

Comment 6

[Chris Peterson [:cpeterson]]

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

Comment 7

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1597479 - Replace usage of nsIDocShellTreeItem in nsLayoutUtils::GetDeviceContextForScreenInfo with BrowsingContext

Comment 8

[Emily McDonough [:alaskanemily]]

So it appears this usage should be OK. There should be a PuppetWidget at each process boundary. This function is used through nsScreen, and the PuppetWidget is querying the screen manager to find the appropriate screen for getting screen information.

It might be worthwhile to move this function fully into nsScreen, since that only uses it for getting screen information from the device context (where the PuppetWidget's device context might not be suitable for general use of this function), or possibly just querying the screen manager right from nsScreen which might reduce the complexity of the entire operation.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Moving to future milestone based on comment 8.