Closed Bug 1573630 Opened 1 year ago Closed 5 months ago

Remove usage of nsIDocShellTreeItem in nsCoreUtils::IsTabDocument

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla79
Fission Milestone Future
Tracking Status
firefox79 --- fixed

People

(Reporter: djvj, Assigned: Jamie)

References

(Blocks 2 open bugs)

Details

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

Attachments

(1 file)

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.

Component: DOM: Core & HTML → Disability Access APIs
Fission Milestone: --- → M5
Priority: -- → P2
Whiteboard: [rm-docshell-tree-item:simple]
Fission Milestone: M5 → Future

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

Fission Milestone: Future → M6

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

Flags: needinfo?(jteh)
Summary: Fix usage of nsIDocShellTreeItem in nsCoreUtils::IsTabDocument → Audit usage of nsIDocShellTreeItem in nsCoreUtils::IsTabDocument

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.
Flags: needinfo?(jteh)
See Also: → 1634591

(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.

Type: enhancement → task
Fission Milestone: M6 → Future
Priority: P2 → P3
Summary: Audit usage of nsIDocShellTreeItem in nsCoreUtils::IsTabDocument → Remove usage of nsIDocShellTreeItem in nsCoreUtils::IsTabDocument
Depends on: 1634591

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.

Assignee: nobody → jteh
Status: NEW → ASSIGNED
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1a7bfda1824
Remove usage of nsIDocShellTreeItem in nsCoreUtils::IsTopLevelContentDocInProcess. r=MarcoZ
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
You need to log in before you can comment on or make changes to this bug.