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

Status: NEW

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

Description

[Kannan Vijayan [:djvj]]

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

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

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

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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

Comment 4

[Chris Peterson [:cpeterson]]

(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! :)