nsPresContext::IsRootContentDocument returns true for OOP iframes
Categories
(Core :: Layout, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox69 | --- | fixed |
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
Most of the call sites of this probably expect it to return true if we're the root content document in the current process, but have an outer content document in a different process.
We probably need two variants of this though, and need to audit all the callers to see which one they want.
|
||
Comment 1•5 years ago
|
||
I wrote a patch that switched IsRootContentDocument to check mDocument->IsRootContentDocument(), as that property has been made to work with fission. The try run shows some crashes [1] that need to be investigated. They may be cause by different expectations of this property as Matt suggested.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3325fc8875fabf732aec12a5dfd24d200fb00d4c
|
Assignee | |
Comment 2•5 years ago
|
||
It looks like PresShell::AssumeAllFramesVisible has infinite recursion if PresShell::IsRootContentDocument returns false, but PresShell::GetToplevelContentDocumentPresContext returns itself.
Apart from just preventing that, we may need to look into what the expected behaviour of the visibility code is under fission.
tnikkel, do you have ideas on how you'd expect this to work with fission?
|
||
Comment 3•5 years ago
|
||
So currently we have chrome documents assume all their images are visible. We do a pass over the frame tree starting at every root content document, assuming the root content document itself is fully visible. Background tabs are handled via PresShell::SetIsActive; so that we still keep the list of visible images in background tabs, but we unlock them so their decoded data can be discarded.
If we use the definition of "root content document" as "document that is content that either has no parent or a non-content parent in the same process" then most of that transfers over to the fission world. The only part of that that would need to change (modulo bugs) for fission would be that we would no longer want to consider every root content documents as fully visible. We would probably want to consider iframes that are scrolled (far) out of view as not visible. We'd just need a mechanism for OOP iframes to know if they are visible or not. I believe I've seen a bug for providing this in fission. Once we have that we could just run the regular image visibility frame walk or mark all images not visible (depending on our visibility status). I don't think it will be worth it to know a rect/region that gives which part of the iframe is visible, a purely visible/not bool would be enough.
|
|
||
Comment 4•5 years ago
|
||
The bug to provide visibility information has landed, so we will call PresShell::SetIsActive on OOP-iframes when they are scrolled into/out of view or tab away from the page [1]. We also intend to communicate some clipping information so that an OOP-iframe that is sized to 10000 pixels tall doesn't paint all of itself.
As for 'root content document', it seems like there are two notions here.
- 'In-process root content document' - A content document who's parent (via DocShell) is null/chrome
- 'Global root content document' - A content document who's cross process parent (via BrowsingContext) is null/chrome
It seems like there are uses for both of these definitions. There is currently a Document::IsTopLevelContentDocument [2] getter that corresponds with (2). We also want to use (2) for determining whether a content document should have a transparent background. As for images, it sounds like we need to use (1).
[1] https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/dom/ipc/BrowserChild.cpp#2768
[2] https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/dom/base/Document.h#2292
|
|
Assignee | |
Comment 5•5 years ago
|
||
Shall we add two new nsLayoutUtils functions (with clear naming as to which behaviour they provide), document the old one as deprecated and then we can incrementally move callers over to the right one?
|
|
||
Comment 6•5 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
Shall we add two new nsLayoutUtils functions (with clear naming as to which
behaviour they provide), document the old one as deprecated and then we can
incrementally move callers over to the right one?
I think that sounds reasonable to me. That'll be easier than trying to audit every use case.
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 7•5 years ago
|
||
|
|
Assignee | |
Comment 8•5 years ago
|
||
Depends on D34098
|
|
Assignee | |
Comment 9•5 years ago
|
||
Depends on D34099
|
||
Comment 10•5 years ago
|
||
|
||
Comment 11•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/78130ff16153
https://hg.mozilla.org/mozilla-central/rev/3562bfab16fc
https://hg.mozilla.org/mozilla-central/rev/af857902f4ef
|
||
Comment 12•5 years ago
|
||
Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.
This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:
0ee3c76a-bc79-4eb2-8d12-05dc0b68e732
|
||
Comment 13•4 years ago
|
||
This doesn't change behavior, and is consistent with the changes made in
bug 1554832 to this code.
|
||
Comment 14•4 years ago
|
||
Comment on attachment 9190824 [details]
Bug 1554832 - Explicitly use InProcess version of IsRootContentDocument for frame visibility. r=tnikkel,hiro
Revision D98473 was moved to bug 1649367. Setting attachment 9190824 [details] to obsolete.
Description
•