1587403 - Fix nsIDocShellTreeItem usage in nsContentUtils::FindPresShellForDocument

Status: RESOLVED WONTFIX

(Core :: Layout, defect, P2)

Description

[Kannan Vijayan [:djvj]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

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

Comment 3

[Chris Peterson [:cpeterson]]

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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

Comment 6

[Chris Peterson [:cpeterson]]

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

Comment 7

[Nika Layzell [:nika] (ni? for response)]

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?

Comment 8

[Emilio Cobos Álvarez (: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.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

moving to m6c to match bug 1639328.

Comment 10

[Neha Kochar [:neha]]

Emilio, is this already fixed by bug 1639328?

Comment 11

[Emilio Cobos Álvarez (: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.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Resolving WONTFIX based on comment 11