Open Bug 1597425 Opened 2 years ago Updated 1 year ago

Remove nsIDocShellTreeItem usage in nsFrameLoader::SwapWithOtherLoader in dom/base/nsFrameLoader.cpp

Categories

(Core :: DOM: Navigation, defect, P3)

defect

Tracking

()

Fission Milestone Future

People

(Reporter: djvj, Unassigned)

References

(Blocks 1 open bug)

Details

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

In file dom/base/nsFrameLoader.cpp

Still relevant method, if both frame-loaders are in process.

Internally uses nsIDocShellTreeItem obtain root tree item for this and other frameloader.

Also uses docshell tree to get parent.

Checks ItemType for all of the above.

All this still needs to be changed to use BrowsingContext

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

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

Tetsuharu, can you (or someone else on your team) 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 is broken with Fission, fixing it blocks enabling Fission is Nightly.

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

Flags: needinfo?(tetsuharu.ohzeki)
Summary: Fix uses of nsFrameLoader::SwapWithOtherLoader in dom/base/nsFrameLoader.cpp → Audit nsIDocShellTreeItem usage in nsFrameLoader::SwapWithOtherLoader in dom/base/nsFrameLoader.cpp

In nsFrameLoader::SwapWithOtherLoader(), rest of usages of nsIDocShellTreeItem is after the guard of calling IsRemoteFrame(). In Fission, AFAIK, all code path for content would enter to nsFrameLoader::SwapWithOtherRemoteLoader() and return it.

So I feel this code might not be broken in Fission.


Additionally, I'm a solo contributor doing in my hobby time :)

Flags: needinfo?(tetsuharu.ohzeki)

(In reply to Tetsuharu OHZEKI [:tetsuharu] (UTC+9) from comment #3)

In nsFrameLoader::SwapWithOtherLoader(), rest of usages of nsIDocShellTreeItem is after the guard of calling IsRemoteFrame(). In Fission, AFAIK, all code path for content would enter to nsFrameLoader::SwapWithOtherRemoteLoader() and return it.

So I feel this code might not be broken in Fission.

That's good to hear. Thanks for checking. In that case, I will postpone this cleanup bug until after we ship Fission.

Additionally, I'm a solo contributor doing in my hobby time :)

Your help is definitely appreciated! :)

Fission Milestone: M6 → Future
Summary: Audit nsIDocShellTreeItem usage in nsFrameLoader::SwapWithOtherLoader in dom/base/nsFrameLoader.cpp → Remove nsIDocShellTreeItem usage in nsFrameLoader::SwapWithOtherLoader in dom/base/nsFrameLoader.cpp
You need to log in before you can comment on or make changes to this bug.