Audit callers of GetDisplayRootFrame() for Fission
Categories
(Core :: Layout, task)
Tracking
()
People
(Reporter: botond, Assigned: dholbert, NeedInfo)
References
Details
(Whiteboard: fission-soft-blocker)
While working through bug 1698693, it occured to me that GetDisplayRootFrame() is another function that could use Fission-related auditing.
This function returns the rootmost frame in the current process. It's possible that some callers assume that in a content process, it returns the root frame of the cross-process RCD, which was the case before Fission but is no longer necessarily the case with Fission.
|
||
Comment 1•3 years ago
|
||
Tracking for Fission M8. We should audit the callers soon so we still have time to fix any that are not Fission-compatible.
|
Assignee | |
Comment 2•3 years ago
|
||
I'm taking a look at this today (and Botond is going to help with some of the APZ callsites).
|
|
||
Comment 3•3 years ago
|
||
This bug is a soft blocker for Fission M8. We'd like to fix it before our M8 Release experiment, but we won't delay the experiment waiting for it.
|
|
Assignee | |
Comment 4•3 years ago
•
|
||
Here's the link to the API here
https://searchfox.org/mozilla-central/search?q=GetDisplayRootFrame&path=
More-specific link (searching searchfox "uses", from the function's definition):
https://searchfox.org/mozilla-central/search?q=symbol:_ZN13nsLayoutUtils19GetDisplayRootFrameEP8nsIFrame&redirect=false
The calls from /dom seem fine, generally.
from bool Document::ShouldThrottleFrameRequests
Seems fine; just testing if a node got painted recently (which we should know on a per-process basis)
from mozilla::dom::Element::GetTransformToViewport
Seems fine, though this one I'm hoping to study a bit further; it's part of the implementation of our privileged JS "getTransformToViewport" JS API, which I could imagine would be something that we'd need to make work across process boundaries. But it seems to work properly now. From a quick investigation, it looks like we use this API for Video PiP and for devtools overlays, and both of those seem to work properly with fission enabled -- I tested using the embedded youtube video at https://developers.google.com/youtube/youtube_player_demo .
from nsContentUtils::WidgetForContent
Seems fine; just getting the widget for a particular nsIContent* pointer. Any such widget that we'd find would necessarily share the same process as the associated nsIContent object, so there should be no reason this would need to cross process boundaries.
from nsDOMWindowUtils::SetDisplayPortForElement
Seems fine; this is something to do with our retained layers structures, and managing/releasing them on the display root. This all gets managed on a per-process basis, so there's no reason this would want to cross process boundaries.
from mozilla::dom::BrowserChild::RecvRenderLayers
Seems fine; this is in a spot where we want to invalidate our whole layer tree (and we use this API to get the display-root to do that). Each process manages its own layer tree, so it's fine/good that we're not crossing process boundaries.
[more to come]
|
|
Assignee | |
Comment 5•3 years ago
•
|
||
The calls from layout/base/nsLayoutUtils.cpp seem fine, modulo some followup for one spot (bug 1724808)
from nsLayoutUtils::DisplayRootHasRetainedDisplayListBuilder:
Seems fine; display lists are built/maintained on a per-process basis, no reason this would need to extend beyond the display root of the given process.
from nsLayoutUtils::GetTransformToAncestorScaleExcludingAnimated
This one might need some followup. This API has one caller which expects to "Compute the scale to the screen" for an image. In a transformed cross-origin iframe, we won't actually get the full scale-to-the-screen; I'm not sure if this is a problem or not (though worst-case I think it just means we spin our wheels a bit). I filed bug 1724808 to follow up on this.
from nsLayoutUtils::GetTransformToAncestorScaleExcludingAnimated
Seems fine. This one's about the upkeep of the layer tree, which is a per-process thing, so it's good for it to not cross process boundaries.
from nsLayoutUtils::PaintFrame
Seems fine; something to do with toggling a flag while painting the layer tree, which is a per-process thing.
from ComputeMaxSizeForPartialPrerender
Seems fine; it looks like this function has to do with figuring out how large our dirty area should be when painting with a partial-prerender. In a cross-origin iframe scenario, the child process handles its own painting and shouldn't need to tunnel out in order to perform this dirty-rect computation.
|
|
Assignee | |
Comment 6•3 years ago
|
||
The calls from layout/generic/nsIFrame.cpp seem fine
They're all to do with paint scheduling and layer invalidation, which happen on a per-process basis.
The call from layout/generic/nsSubDocumentFrame.cpp seems fine
It's in ClearDisplayItems() and has to do with display list cleanup; and display lists / painting is a per-process thing.
The calls from layout/painting/nsDisplayList.cpp seem fine
They're to do with display list management and painting decisions, none of which seem like they need to reason about content outside of the same-process display-root.
So I think we're good here, though I'll add a needinfo to remind myself to follow up on GetTransformToViewport per comment 4 to confirm-why/better-understand-how things are working at that callsite.
Description
•