Open Bug 1597479 Opened 1 year ago Updated 4 months ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

Fission Milestone Future

People

(Reporter: djvj, Assigned: alaskanemily)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 obsolete file)

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.

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 nsLayoutUtils::GetDeviceContextForScreenInfo in layout/base/nsLayoutUtils.cpp → Audit nsIDocShellTreeItem usage in nsLayoutUtils::GetDeviceContextForScreenInfo in layout/base/nsLayoutUtils.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)

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

Flags: needinfo?(jwatt) → needinfo?(emcdonough)
Priority: -- → P2

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

Assignee: nobody → emcdonough

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

Fission Milestone: M6 → M6c

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.

Flags: needinfo?(emcdonough)
Attachment #9149166 - Attachment is obsolete: true

Moving to future milestone based on comment 8.

Fission Milestone: M6c → Future
Whiteboard: [rm-docshell-tree-item:hard] [layout:triage-discuss] → [rm-docshell-tree-item:hard]
You need to log in before you can comment on or make changes to this bug.