Closed Bug 1587403 Opened 1 year ago Closed 3 months ago

Fix nsIDocShellTreeItem usage in nsContentUtils::FindPresShellForDocument

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED WONTFIX
Fission Milestone M6c

People

(Reporter: djvj, Unassigned)

References

(Blocks 1 open bug)

Details

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

https://searchfox.org/mozilla-central/rev/7cc0f0e89cb40e43bf5c96906f13d44705401042/dom/base/nsContentUtils.cpp#6123

The primary use is WidgetForDocument, which seems to be primarily used by layout code and rendering code.

This seems like a bit of a difficult fix. The logic walks up the docshell tree and returns the first PresShell that it finds, which may be out of process.

All layout code that uses this needs to be changed.

Whiteboard: [rm-docshell-tree-item:hard]
Fission Milestone: --- → M5
Priority: -- → P2
Fission Milestone: M5 → Future

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

Fission Milestone: Future → M6
Summary: Fix usage of nsIDocShellTreeItem in nsContentUtils::FindPresShellForDocument → Audit usage of nsIDocShellTreeItem in nsContentUtils::FindPresShellForDocument

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

Fission Milestone: M6 → M6b

@ Emilio: how should we handle display:none for cross-origin iframes with Fission?

Component: DOM: Navigation → Layout
Flags: needinfo?(emilio)
Summary: Audit usage of nsIDocShellTreeItem in nsContentUtils::FindPresShellForDocument → Fix nsIDocShellTreeItem usage in nsContentUtils::FindPresShellForDocument

Can you clarify? A display: none iframe doesn't create a load, and thus shouldn't create a new process, as far as I can tell.

Flags: needinfo?(emilio)
Flags: needinfo?(cpeterson)

But I think it can basically be left as is, with s/DocShellTreeItem/BrowsingContext, or even just GetInProcessParentDocument or such... I think it may need to cross the content / chrome boundary for non-e10s, but...

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

A display: none iframe doesn't create a load, and thus shouldn't create a new process, as far as I can tell.

But I think it can basically be left as is, with s/DocShellTreeItem/BrowsingContext, or even just GetInProcessParentDocument or such... I think it may need to cross the content / chrome boundary for non-e10s, but...

Nika, does this answer your question about display:none? Can we postpone these FindPresShellForDocument changes to Fission Future?

Flags: needinfo?(cpeterson) → needinfo?(nika)

I was trying to figure out what this code was doing, and it seemed, from some of the nearby comments, that this was intended to find the PresShell for a document in a display:none iframe element. I was also under the impression that a display:none iframe isn't loaded so we would never end up in this situation, but that made me uncertain as to what the code does.

If we never have an <iframe> which has no PresShell, could we perhaps get away with getting rid of this function entirely?

Flags: needinfo?(nika) → needinfo?(emilio)

err, I don't know what was I thinking, an iframe without a presshell can totally happen. However for cross-process iframes apparently it can't, but that seems more like a bug than anything else. Probably we should figure out what to do with this code in bug 1639328.

Flags: needinfo?(emilio)

moving to m6c to match bug 1639328.

Fission Milestone: M6b → M6c

Emilio, is this already fixed by bug 1639328?

Flags: needinfo?(emilio)

Not really, but it's not clear what the right fix for this would look like. In fission iframes we'd find the PuppetWidget right away so we shouldn't need to walk past process boundaries, so the code I think works as expected right now.

Flags: needinfo?(emilio)

Resolving WONTFIX based on comment 11

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.