(Similar to bug 1599913)

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

We should complete this audit in M7a. Daniel, please assign someone to this task for M7a (Fx90).

There are 15 callsites ("Uses" at https://searchfox.org/mozilla-central/search?q=GetParentPresContext&path= ).

Starting to go through them... The first two look un-concerning.

https://searchfox.org/mozilla-central/rev/2f109387cc6886859d3f985ed9aca352fff653b8/gfx/layers/apz/util/APZCCallbackHelper.cpp#428

• This one is basically-innocuous; it's an assertion that's becoming less strict. I filed bug 1709460 on that.

https://searchfox.org/mozilla-central/rev/2f109387cc6886859d3f985ed9aca352fff653b8/layout/base/GeometryUtils.cpp#236

• This one is innocuous / correct. It's checking whether we can compare frames from two different documents, and it's correct & intentional that it fails to work for cross-origin content.

[I'll try to get through the rest before too long; hopping into a meeting now.]

I'll jump ahead to nsPresContext.cpp:

• Its first call is in nsPresContext::GetInProcessRootContentDocumentPresContext(). That one's definitely fine as-is since it's in an explicitly "in-process" function.
• Its second call is in nsPresContext::GetRootPresContext(), which seems to be a rabbit-hole in and of itself, with lots of callers that need to be triaged
• Its third call is in nsPresContext::NotifyInvalidation, which seems to be about handling paints and invalidation. This one looks fine as-is; I don't imagine paint-invalidations would need or want to cross process boundaries.

Besides those and comment 2, the other GetParentPresContext calls are in these files:
PresShell.cpp
ViewportUtils.cpp
nsLayoutUtils.cpp
nsGfxScrollFrame.cpp

Most of these calls look to be APZ-related, and I'm not familiar enough with our APZ code/invariants to know what to make of those. (And I'm not sure which [if any] of those might be GeckoView-specific, which could put them out of scope for the first major fission release.)

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

Besides those and comment 2, the other GetParentPresContext calls are in these files:
PresShell.cpp
ViewportUtils.cpp
nsLayoutUtils.cpp
nsGfxScrollFrame.cpp

Most of these calls look to be APZ-related, and I'm not familiar enough with our APZ code/invariants to know what to make of those.

botond, perhaps you could triage/audit the GetParentPresContext calls in these four files? (from the "Uses (nsPresContext::GetParentPresContext)" section of https://searchfox.org/mozilla-central/search?q=GetParentPresContext&path= )

I've gone through them, here is my analysis:

• PresShell.cpp
  • CreateRangePaintInfo: This code is related to drag previews. I'm not super familiar with it, but yes, this one may need to be updated for Fission. ==> TODO: Let's spin this out into a seprate bug, along the lines of "Ensure drag previews are correctly positioned/sized when dragging content inside an OOP iframe with the parent document pinch-zoomed in"
  • GetCumulativeResolution: Based on the direction taken in bug 1518908 comment 5, OOP iframes do not need to know about the resolution of the enclosing parent document (this is handled by the "parent to child conversion matrix" instead) ==> no action required
  • MarkFramesInSubtreeApproximatelyVisible: Here it looks like the condition (pc->IsRootContentDocument() || !pc->GetParentPresContext())) should be updated to something like "cross-process root content or root chrome, but not nested root content" ==> TODO: update required
• ViewportUtils.cpp
  • ConvertToScreenRelativeVisual: This is also related to drag previews ==> TODO: Handle as part of the same bug as CreateRangePaintInfo
• nsLayoutUtils.cpp
  • CalculateBasicFrameMetrics: Here, the check is inside a block that already includes a presContext->IsRootContentDocument() check, and therefore is only true in non-e10s cases. Fission doesn't change that ==> no action required.
  • ComputeScrollMetadata: This is used to compute FrameMetrics::mIsLayersIdRoot. Nested content processes get their own LayersID, so this is fine ==> no action required
  • GetRootMetadata: This code is meant to ensure the root APZC in a given process has a displayport, so the existing check is appropriate ==> no action required
• nsGfxScrollFrame.cpp
  • DecideScrollableLayer
    • first call site: this assumes that chrome documents are not OOP ==> TODO: check on this -- can they be?
    • second call site: this one is a check guarding the call to RestrictToRootDisplayPort(), which specifically deals with the in-process root (and this is fine as discussed in bug 1650183 comment 14) ==> no action required

(In reply to Botond Ballo [:botond] from comment #5)

• MarkFramesInSubtreeApproximatelyVisible: Here it looks like the condition (pc->IsRootContentDocument() || !pc->GetParentPresContext())) should be updated to something like "cross-process root content or root chrome, but not nested root content" ==> TODO: update required

I believe bug 1649367 has an in progress patch for this.

• CalculateBasicFrameMetrics: Here, the check is inside a block that already includes a presContext->IsRootContentDocument() check, and therefore is only true in non-e10s cases. Fission doesn't change that ==> no action required.

We'll need to audit the IsRootContentDocument call though, but I guess we can do that in the IsRootContentDocument bug.

botond, could you file helper bugs as-needed for the TODO items that you mentioned in comment 5?

(In reply to Botond Ballo [:botond] from comment #5)

• CreateRangePaintInfo: This code is related to drag previews. I'm not super familiar with it, but yes, this one may need to be updated for Fission. ==> TODO: Let's spin this out into a seprate bug, along the lines of "Ensure drag previews are correctly positioned/sized when dragging content inside an OOP iframe with the parent document pinch-zoomed in"

Filed bug 1712400 for this.

• MarkFramesInSubtreeApproximatelyVisible: Here it looks like the condition (pc->IsRootContentDocument() || !pc->GetParentPresContext())) should be updated to something like "cross-process root content or root chrome, but not nested root content" ==> TODO: update required

Based on comment 6, this will be dealt with in bug 1649367, which I've re-nominated for Fission.

• ConvertToScreenRelativeVisual: This is also related to drag previews ==> TODO: Handle as part of the same bug as CreateRangePaintInfo

Will be handled in bug 1712400 as well.

• nsGfxScrollFrame.cpp
  • DecideScrollableLayer
    • first call site: this assumes that chrome documents are not OOP ==> TODO: check on this -- can they be?

Filed bug 1712401 for this.

Rolling over the remaining Fission M7a bugs to the Fission M8 milestone.

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.

Deferring this bug from Fission Milestone M8 to MVP. This bug doesn't need to block our Release channel experiment (M8) and we wouldn't uplift a fix to Beta 92 this late in the beta cycle.

Note: in the time that's passed since comment 2, we've added two new calls to this API:

• One call in DisplayPortUtils.cpp, in GetRootDisplayportBase:
  https://searchfox.org/mozilla-central/rev/9759c03110173c8cf62ba0de21aa9b06b850c794/layout/base/DisplayPortUtils.cpp#1201
• One call call in ViewportUtils.cpp, in TryInferEnclosingResolution:
  https://searchfox.org/mozilla-central/rev/9759c03110173c8cf62ba0de21aa9b06b850c794/layout/base/ViewportUtils.cpp#264

Fortunately I think both of these are fine. Both of these were added in recent commits authored/reviewed by folks working on fission audits (author:hiro, botond; reviewed by: tnikkel/hiro), in patches that were specifically about OOP child frames. So, I think it's safe to assume those callsites have already been vetted as being fine in a fission world. (Also, they're in assertions -- i.e. not in user-affecting logic).

Beyond that, the above comments have covered all of the existing callsites (comment 2, comment 3, comment 5, comment 6, comment 8). The only unresolved thread here is nsPresContext::GetRootPresContext() from comment 2, and I've spun off bug 1727437 for it.

With that, there's no more auditing to do here, so I think we're good to close this bug.