Closed Bug 1727437 Opened 3 years ago Closed 3 years ago

Audit callers of nsPresContext::GetRootPresContext

Categories

(Core :: Layout, task, P2)

Product:
Core
Component:
Layout
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Project Flags:
Fission Milestone MVP

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: fission-soft-blocker)

(Similar to bug 1699846)

nsPresContext::GetRootPresContext() works fine in-process, but doesn't traverse beyond oop-iframe process boundaries. We need to be sure its callers are OK with that.

I'm cloning the state of bug 1699846 to file this bug, since I'm basically spinning this off from that bug (bug 1699846 comment 3).

Assignee: nobody → dholbert

searchfox link:
https://searchfox.org/mozilla-central/search?q=symbol:_ZNK13nsPresContext18GetRootPresContextEv&redirect=false

Quick first-glance observation: the calls from layout/xul (in nsMenuPopupFrame.cpp and nsXULPopupManager.cpp) are likely-fine, since child processes (and OOP iframes) shouldn't be able to render XUL at this point.

Fission Milestone: --- → MVP

Working through the callsites here:

  • accessible/generic/LocalAccessible.cpp
    ** LocalAccessible::LocalChildAtPoint: Seems fine; it's an explicitly same-process API, so its internals shouldn't need to cross process-boundaries. (There's a separate cross-process API to perform the same task, called ChildAtPoint. Searchfox link showing the two APIs: https://searchfox.org/mozilla-central/rev/be9c0ff754a04b32226b68aa014a8137ac29bd53/accessible/generic/LocalAccessible.h#281-287 ) ==> no action required

  • dom/base/Selection.cpp
    ** AutoScroller::DoAutoScroll: This code uses GetRootPresContext just to get the root frame, so we can get the scroll "destination-point" with respect to that root frame's screen position; and we compare that to the screen rect that presShell->GetViewManager()->GetDeviceContext() gives us, to help determine how far to scroll the view. This all seems to be working consistently with same-process objects when it's comparing things, so it looks like it's fine. ==> no action required

  • dom/base/nsDOMWindowUtils.cpp
    ** nsDOMWindowUtils::SendQueryContentEvent: This is an implementation of a privileged JS API that's only called from test script (not from our actual frontend code at all), so I don't think we need to worry about it impacting users. So I don't think we need to dig further on this one. ==> probably no action required, though it would be great if @masayuki could take a look and confirm, though.
    ** nsDOMWindowUtils::GetLastTransactionId: This is explicitly getting the transaction ID from the refresh driver, which is a per-process thing, so this is fine. ==> no action required

  • dom/events/ContentEventHandler.cpp
    ** ContentEventHandler::ConvertToRootRelativeOffset: This is some IME-related stuff that I don't fully understand. @masayuki, perhaps you can confirm whether the GetRootPresContext() call there looks fine from a fission perspective? (i.e. whether it's fine that it returns the root pres context of the iframe rather than the whole nested document, if it's called for an out-of-process iframe)

[more to come]

Flags: needinfo?(masayuki)

nsDOMWindowUtils::SendQueryContentEvent

Yeah, it's test only API, and it should not cross process boundary. It just requires a widget which can work with widget in the main process or a puppet widget in content processes.

ContentEventHandler::ConvertToRootRelativeOffset

It's no problem. The root caller (widget) requires offset relative to the widget. It's adjusted by the main process:
https://searchfox.org/mozilla-central/rev/be9c0ff754a04b32226b68aa014a8137ac29bd53/dom/ipc/BrowserParent.cpp#3054-3058
So, ContentEventHandler in content processes should return offset from its (in-process) root widget.

(Are there any test site of OOP iframe? Henri's web site is down...)

Flags: needinfo?(masayuki)

Thanks, Masayuki!

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

(Are there any test site of OOP iframe? Henri's web site is down...)

hiro's note in this phabricator review has one option for an alternative: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select

But also, https://hsivonen.fi/fission-host.html seems to be up right now. Presumably it was just down for some short-term issue back when comment 3 was posted.

(picking up from comment 2...)

  • dom/events/EventDispatcher.cpp
    ** EventDispatcher::Dispatch: This function's 4 calls are all just trying to communicate with the refresh driver which lives on the outermost document. In an OOP-iframe scenario, we'll correctly communicate with the outermost same-process refresh driver. ==> no action required

  • dom/html/HTMLCanvasElement.cpp
    ** HTMLCanvasElement::RegisterFrameCaptureListener: Similar to above, we're just communicating with the refresh driver. ==> no action required

  • layout/base/PresShell.cpp
    ** PresShell::RenderDocument: Fine, just flushing will-paint observers which must necessarily be same-process. ==> no action required
    ** PresShell::EventHandler::ComputeRootFrameToHandleEventWithPopup (2 usages): Masayuki, could I also punt these two calls to you? It looks like you've touched some code in this area, and I'm not familiar enough with this code to make a quick auditing judgement.
    ** PresShell::EventHandler::AdjustContextMenuKeyEvent: Masayuki, same here, could you take a look at this one?
    ** PresShell::WillPaint: Fine, just flushing will-paint observers which must necessarily be same-process. ==> no action required
    ** PresShell::DidPaintWindow: Probably-fine; this call is just part of excluding popups from the internal "widget-first-paint" notification. It's conceivable that with fission, we could now inadvertently send this notification in a scenario where we're in an OOP iframe inside of a popup. It looks like we only listen for this notification from tests, though, based on this search which just turns up 3 locations: this sending location, a usage in browser/components/tests/ and in testing/talos. ==> no action required
    ** PresShell::GetRootPresShell: I was worried this might be another rabbithole, but fortunately it's only got a 5 local callers ("Uses" in the search results here). The first two are about synthetic mouse events for testing, so are clearly not user-facing. The third is in RecordMouseLocation to e.g. handle mouse events; this seems fine as being handled by the same-process root presshell (doesn't need to cross process boundaries). The fourth is in MaybeFlushThrottledStyles which is used to be sure we've got up-to-date style info when handling an input event; and that's reasonable to do on a same-process basis. And the fifth is in DetermineFontSizeInflationState which is disabled by default and basically mobile-specific so I'm not worrying about it here (since we're not including mobile in the initial fission rollout). ==> no action required

ni=masayuki for help with ComputeRootFrameToHandleEventWithPopup and AdjustContextMenuKeyEvent

[more to come]

Flags: needinfo?(masayuki)

(In reply to Daniel Holbert [:dholbert] from comment #4)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

(Are there any test site of OOP iframe? Henri's web site is down...)

hiro's note in this phabricator review has one option for an alternative: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select

But also, https://hsivonen.fi/fission-host.html seems to be up right now. Presumably it was just down for some short-term issue back when comment 3 was posted.

Thanks! I verified that IME window is positioned at correct position even in OOP <iframe>. So, ContentEventHandler::ConvertToRootRelativeOffset works fine.

** PresShell::EventHandler::ComputeRootFrameToHandleEventWithPopup (2 usages): Masayuki, could I also punt these two calls to you? It looks like you've touched some code in this area, and I'm not familiar enough with this code to make a quick auditing judgement.

I also just touched at factoring out it (the original method was one of the longest method in our code). So, I'm still not sure, but it seems that nsLayoutUtils::GetPopupFrameForEventCoordinates always returns nullptr in remote process because in my understanding, all popup windows (frames) are created only in the main process. When it returns non-nullptr, the popup manager needs to have popups. It's set only here:
https://searchfox.org/mozilla-central/rev/ad2ffab089e4e0c0fe99a1a046ab2b1c45546bdb/layout/xul/nsXULPopupManager.cpp#1065
I see process type check around there is only:
https://searchfox.org/mozilla-central/rev/ad2ffab089e4e0c0fe99a1a046ab2b1c45546bdb/layout/xul/nsXULPopupManager.cpp#1859

Enn: nsXULPopupManager can have popup frames in non-main process?

Flags: needinfo?(enndeakin)

** PresShell::EventHandler::AdjustContextMenuKeyEvent: Masayuki, same here, could you take a look at this one?

I'm not so familiar with this too.

According to the code and behavior, context menu should be opened where a position at caret or focused content or root document's top-left.
For the all cases, I see the expected result. If nothing has focus, the context menu event is delivered to the root document, and otherwise, delivered to focused document.
https://searchfox.org/mozilla-central/rev/ad2ffab089e4e0c0fe99a1a046ab2b1c45546bdb/widget/WidgetEventImpl.cpp#444,447,452,455

So, when a focused element is in an OOP <iframe>, the mRefPoint is always overwritten:
https://searchfox.org/mozilla-central/rev/ad2ffab089e4e0c0fe99a1a046ab2b1c45546bdb/layout/base/PresShell.cpp#8876,8894

So, no problem even if it couldn't get the root document's coordinates from an isolated content process.

Flags: needinfo?(masayuki)

I see process type check around there is only:
https://searchfox.org/mozilla-central/rev/ad2ffab089e4e0c0fe99a1a046ab2b1c45546bdb/layout/xul/nsXULPopupManager.cpp#1859

Enn: nsXULPopupManager can have popup frames in non-main process?

Henri had said there was some case where a popup appeared in a child process, but the assert he added linked to above suggest that he removed that case, so yes popups should only appear in the parent process.

Flags: needinfo?(enndeakin)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

** PresShell::EventHandler::AdjustContextMenuKeyEvent: Masayuki, same here, could you take a look at this one?

I'm not so familiar with this too.

According to the code and behavior, context menu should be opened where a position at caret or focused content or root document's top-left.
For the all cases, I see the expected result.

While an oop frame might not have different behaviour, this looks broken in general though. When text is selected, the context menu should appear near the selected text but my testing shows that it appears in the topleft corner. Probably been broken since e10s.

Thanks! Then, the method always returns given root frame in content process from here:
https://searchfox.org/mozilla-central/rev/5a362eb7d054740dc9d7c82c79a2efbc5f3e4776/layout/base/PresShell.cpp#7738-7739
So, it does not have any problem.

Thanks, Masayuki and Neil! So I think we're at ==> no action needed for all callsites so far.

Picking up from comment 5:

  • layout/base/nsLayoutUtils.cpp
    ** nsLayoutUtils::GetPopupFrameForEventCoordinates: masayuki/Neil vetted this in comment 6 and comment 8, as part of vetting ComputeRootFrameToHandleEventWithPopup, so this one is ==> no action needed
    ** nsLayoutUtils::CalculateBoundingCompositionSize: This chunk of code should be fine; botond updated this function in bug 1650183 and bug 1691572 to be fission-friendly, and it's documented as being specific to a particular process. ==> no action needed

  • layout/base/nsPresContext.cpp
    ** nsPresContext::IsDOMPaintEventPending: This chunk has to do with MozAfterPaint events and communicating with the refresh driver, and those are managed per-process. ==> no action needed
    ** nsPresContext::NotifyContentfulPaint: This chunk is about the "first-contentful-paint" metric. We may have a behavior-change here, where we'll start excluding paints from OOP iframes; and that's probably-good from a security perspective. I filed bug 1728463 just to note the behavior-change. Beyond that, there's ==> no action needed

  • layout/base/nsRefreshDriver.cpp
    ** All of the calls here are just trying to access the refresh driver that's associated with the root nsPresContext. I don't think we want refreshdriver's mechanics to cross process boundaries, so this all seems fine. ==> no action needed

[more to come...]

Picking up from comment 11:

  • layout/forms/nsComboboxControlFrame.cpp
    ** nsComboboxControlFrame::GetCSSTransformTranslation: This is about figuring out placement of the dropdown menu. It's a helper for nsComboboxControlFrame::GetAvailableDropdownSpace() and has no other callers besides that. GetAvailableDropdownSpace() has two callers, both of which are skipped in content processes, via XRE_IsContentProcess() checks: here and here. So fission doesn't make a difference here. ==> no action needed

  • layout/generic/nsGfxScrollFrame.cpp:
    ** GetPaintedLayerScaleForFrame: This function has two callers, and it's used to get the potentially-transformed-up size of the scrolled-frame, so that we can scroll properly (in this content process). If this is called inside a fission OOP iframe, there shouldn't be any reason for this operation to consider scales from outer cross-process documents, since those scales would effectively be compositing effects that happen outside of our process, I think. ==> no action needed
    ** ScrollFrameHelper::RestrictToRootDisplayPort: This function is seems to just be an optimization for stability, as described in its initial comment -- it limits the size of the display port that we set up for a scrollable element, I think. In a scenario with an OOP iframe that happens to be huge, with even-huger scrollable elements inside of it, it looks like maybe we'll now only limit those scrollable elements to the size of the iframe, rather than the size of the ultimate browser viewport (which would be managed by a separate content process). This change could conceivably result in increased memory usage for this (somewhat-pathological) scenario, but I think it's kinda necessary, and it wouldn't have any other user-detectable effects. ==> no action needed
    ** ScrollFrameHelper::PostOverflowEvent: This is about a paint-time update (determining whether this particular scrollframe just started overflowing and requesting a will-paint callback to account for that, using the root pres context's painting cycle). This is fine; we should use our own content-process's painting cycle for this. ==> no action needed

[more to come...]

  • layout/generic/nsIFrame.cpp
    ** nsIFrame::GetOffsetToCrossDoc (4 callsites): This is about comparing the positions of two nsIFrame* for which we have pointers, so they must be same-process. Hiro also skimmed the callers of this function previously, as noted in bug 1727431 comment 1, and didn't find anything that looked problematic from a fission perspective. ==> no action needed
    ** nsIFrame::GetTransformMatrix: The callsite here is inside a nsLayoutUtils::IsPopup(this) check, and per comment 6 and comment 8, popup frames are only created in the main process, so we should be OK to consider this callsite as only occurring in the main process. ==> no action needed
    ** SchedulePaintInternal: This has to do with paint request/timing; each child process necessarily has to handle its own painting, so it's fine & good for this call not to cross process boundaries. ==> no action needed

  • layout/painting/nsDisplayList.cpp
    ** nsDisplayListBuilder::LeavePresShell: This one has to do with first-contentful-paint, and my notes above and in bug 1728463 about NotifyContentfulPaint & fission are similarly applicable here. (Not-crossing-process-boundaries is probably good for first-contentful-paint notification.) ==> no action needed
    ** nsDisplayListBuilder::AddToAGRBudget: This has to do with a painting optimization (for AGR = animated geometry roots), and doesn't seem like it needs any cross-process considerations. ==> no action needed

  • layout/xul/nsMenuPopupFrame.cpp and layout/xul/nsXULPopupManager.cpp:
    ** All calls here should be fine, per comment 1, comment 6, and comment 8.

  • view/nsView.cpp
    ** nsView::DidCompositeWindow: This has to do with paint timing/scheduling; we just send NotifyDidPaintForSubtree to the rootPresContext. I don't think there's any cross-child-process considerations to consider there. ==> no action needed

  • widget/nsNativeBasicTheme.cpp
    ** nsNativeBasicTheme::GetDPIRatioForScrollbarPart: Here, we're ultimately just trying to call GetRootWidget so that we can ask it for its default scale. This seems like it should work just fine with our in-process root pres context; it doesn't depend on e.g. scale factors from the outer document or anything like that. ==> no action needed

And that's it! Closing this audit-bug as FIXED.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

(In reply to Daniel Holbert [:dholbert] from comment #12)

** ScrollFrameHelper::RestrictToRootDisplayPort: This function is seems to just be an optimization for stability, as described in its initial comment -- it limits the size of the display port that we set up for a scrollable element, I think. In a scenario with an OOP iframe that happens to be huge, with even-huger scrollable elements inside of it, it looks like maybe we'll now only limit those scrollable elements to the size of the iframe, rather than the size of the ultimate browser viewport (which would be managed by a separate content process). This change could conceivably result in increased memory usage for this (somewhat-pathological) scenario, but I think it's kinda necessary, and it wouldn't have any other user-detectable effects. ==> no action needed

Just wanted to chime in here and say that the mentioned pathological scenario (OOP iframe with a huge viewport) has been discussed. You are correct that RestrictToRootDisplayPort will do less restricting with Fission, but it turns out its input (the rect it restricts) is already restricted to the browser viewport size in nested content processes, so there is so memory usage issue (see bug 1650183 comment 14 for details).

Thanks, Botond! That's great to hear.

You need to log in before you can comment on or make changes to this bug.