1573630 - Remove usage of nsIDocShellTreeItem in nsCoreUtils::IsTabDocument

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task, P3)

Description

[Kannan Vijayan [:djvj]]

https://searchfox.org/mozilla-central/source/accessible/base/nsCoreUtils.cpp#357

This function determines whether a document is a tab document or not. For content documents, it checks for a null parent. For chrome documents, it checks whether the parent of the document is equal to the root of the document.

Only the first case should need fixing. Explained below.

In the former case (content-process), the logic should do the following.

1. Check if parent is null, if non-null, then this is not a tab document.
2. If the parent is null, then check for an out-of-process parent, and return false if it exists, otherwise true.

In the latter case (chrome process), the logic should work as is, since chrome process contains everything in-process, and should not have any notion of "out of process" parents for the current doc. If this assumption is false, then an appropriate fix is needed for the chrome-process case as well.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

Jamie, can someone on your team please audit this use of the nsIDocShellTreeItem interface in nsCoreUtils::IsTabDocument?

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 and your team should prioritize (or delegate) the fix accordingly.

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

[James Teh [:Jamie]]

Nothing is broken here, but the current naming is confusing. Low priority renaming job. We should:

1. Rename IsTabDocument to IsTopLevelContentDocInProcess.
2. Rename eTabDocument to eTopLevelContentDocInProcess.
3. In the Windows code, add assertions that all uses of both of these are in the parent process.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to James Teh [:Jamie] from comment #3)

Nothing is broken here

Awesome. In that case, this bug doesn't need to block Fission MVP.

but the current naming is confusing. Low priority renaming job. We should:

1. Rename IsTabDocument to IsTopLevelContentDocInProcess.
2. Rename eTabDocument to eTopLevelContentDocInProcess.
3. In the Windows code, add assertions that all uses of both of these are in the parent process.

I filed bug 1634591 for the renamings.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1573630: Remove usage of nsIDocShellTreeItem in nsCoreUtils::IsTopLevelContentDocInProcess.

This usage of nsIDocShellTreeItem is safe, but nsIDocShellTreeItem is going away and it's trivial to convert this to use BrowsingContext.
As a bonus, the code is also shorter and more readable.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1a7bfda1824
Remove usage of nsIDocShellTreeItem in nsCoreUtils::IsTopLevelContentDocInProcess. r=MarcoZ

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d1a7bfda1824