Closed Bug 1525561 Opened 6 years ago Closed 4 years ago

Fission: Audit / fix callers of GetToplevelContentDocumentPresContext

Categories

(Core :: Layout, task, P2)

Product:
Core
Component:
Layout
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla80
Project Flags:
Fission Milestone M6a
Tracking Flags:
Tracking Status
firefox80 --- fixed

People

(Reporter: emilio, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1525561 - Rename nsPresContext::GetToplevelContentDocumentPresContext to GetInProcessRootContentDocumentPresContext.
47 bytes, text/x-phabricator-request
Details | Review

Most of them seem to want the dimensions of the content area in some way.

Summary: Fission: Audit / fix callers of GetRootLevelContentDocumentPresContext → Fission: Audit / fix callers of GetToplevelContentDocumentPresContext
Priority: -- → P3
Priority: P3 → P2

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage 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

Fission Milestone: --- → ?

Tracking for Fission Nightly (M6)

@ Emilio, what needs to be done for this audit? Who should do it?

Type: enhancement → task
Fission Milestone: ? → M6
Flags: needinfo?(emilio)

Someone familiar with the related parts of the code, and with fission.

For example for the use I wrote: https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/dom/events/EventStateManager.cpp#3831

Really wants event coordinates from the top-level content document. But the current behavior in a fission iframe is that the coordinates are relative to the <iframe> viewport, I think.

In that particular case, it's probably ok, because it's a custom-cursor restriction for large cursors, and we could probably live with it if <iframe>s couldn't overflow large cursors outside of their own viewport. But we should make the explicit call.

Other places probably have correctness issues / behavior changes with fission as well. We should audit them.

Flags: needinfo?(emilio)
Severity: normal → N/A
Fission Milestone: M6 → M6a

I checked all call sites of the GetToplevelContentDocumentPresContext (actually I did write a few of them).

These are fine in fission, or I've already filed bugs;

For rest of the call sites, I am not sure whether it's ok or would be an issue in fission but can defer it later or blah blah blah, so I am going to ask persons respectively. Guys, can you please respond here or file bugs? Thank you!

Flags: needinfo?(kats)
Flags: needinfo?(jnicol)
Flags: needinfo?(hkirschner)
Flags: needinfo?(botond)
Flags: needinfo?(aethanyc)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

I think this function is only used if we are using in-process combobox drop downs, which I don't think we do anymore, perhaps in disable-e10s scenarios.

Can somebody confirm what the current behaviour of GetToplevelContentDocumentPresContext will be if fission is enabled? i.e. if the function is called in a content process that is different from the one that contains the top-level content document, will this function return null? Is that also the case if there's an A -> B -> A kind of iframe nesting, where the "A" documents are in the same process and the "B" document is in a different process?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

I filed bug 1649440 for the ZoomToFocusedInput callsite. The ChromeProcessController shouldn't be affected by fission since that is only ever run in the UI process. (And anyway, that code was for Fennec and is currently dead, but it may come back to life with desktop zooming). I filed bug 1649447 for the ViewportUtils use.

The last one is in nsLayoutUtils::ShouldDisableApzForElement and that one requires some answers from devtools/accessibility folks. The relevant use case for ShouldDisableApzForElement is that there is some content inside a subframe (such as a devtools overlay or the AccessibleCaret) that needs to move in sync with scrolling. So we disable APZ in those subframes. However in a fission world, I don't know how devtools overlays or AccessibleCaret work - they're not going to be getting regular scroll events either so the work needed here from the APZ side will depend on the architecture of those pieces for fission-friendliness.

Needinfo to :gl and :TYLin for this. :gl, can you explain (or redirect to somebody who can) how the problem described in this bug might be affected by fission? I don't recall the details of which process the highlighters are drawn from and what mechanism is used to notify them of updates in scroll position of subfrrames. :TYLin, same question for you for the AccessibleCaret stuff - for fission, which process is going to hold the AccessibleCaret and how will it be notified of scroll position updates?

Flags: needinfo?(kats) → needinfo?(gl)

Forwarding the needinfo request on highlighter and fission over to Razvan.

Flags: needinfo?(gl) → needinfo?(rcaliman)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)

Can somebody confirm what the current behaviour of GetToplevelContentDocumentPresContext will be if fission is enabled? i.e. if the function is called in a content process that is different from the one that contains the top-level content document, will this function return null?

No, it just returns the root pres context in the same process by walking up the ancestor of the pres context.

Is that also the case if there's an A -> B -> A kind of iframe nesting, where the "A" documents are in the same process and the "B" document is in a different process?

In this case, B's top is the B's pres context, the grand child A's top is the grand child A's pres context, not same as the ancestor A's, IIUC.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

I'm afraid I really don't know anything about what effect fission has on this, nor what you mean by "it should be restricted in the OOP's top document". That patch was written because of the case where a nested scroll frame was much larger than its parent scroll port. (ie a table which scrolls horizontally, which is much taller than the height of the screen. Prior to that patch, we would calculate the nested scroll frame's display port using the full height of the table, which was too large and caused excessive memory usage. The patch made us clip it to the root composition bounds.

I don't think we want that behaviour to change because of fission, otherwise we risk using too much memory again.

Flags: needinfo?(jnicol)

(In reply to Jamie Nicol [:jnicol] from comment #10)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

I'm afraid I really don't know anything about what effect fission has on this, nor what you mean by "it should be restricted in the OOP's top document". That patch was written because of the case where a nested scroll frame was much larger than its parent scroll port. (ie a table which scrolls horizontally, which is much taller than the height of the screen. Prior to that patch, we would calculate the nested scroll frame's display port using the full height of the table, which was too large and caused excessive memory usage. The patch made us clip it to the root composition bounds.

So for example, if ScrollFrameHelper::RestrictToRootDisplayPort gets called in the OOP iframes, say A -> A -> B (A is the same domain, B is a cross origin), if the function gets called in the B, GetTopLevelContentDocumentPresContext returns B's pres context, so the root frame size is basically smaller than the top A's one, that's what I meant.

If B's root frame is smaller than the top A's then this is fine. But could there not be a case where B's root frame is larger than A's (ie a very tall iframe, taller than the screen). If there was then a large scroll frame within B we could run in to bug 1241917 again?

(In reply to Jamie Nicol [:jnicol] from comment #12)

If B's root frame is smaller than the top A's then this is fine. But could there not be a case where B's root frame is larger than A's (ie a very tall iframe, taller than the screen). If there was then a large scroll frame within B we could run in to bug 1241917 again?

If nsGkAtoms::DisplayPort is not set there (looks like it's set only for testing?), we end up calling nsLayoutUtils::CalculateCompositionSizeForFrame then, it calls GetScrollPortRect() or GetRect() depending on whether it's scrollable or not, Botond told me recently some XUL documents in browser chrome window doesn't have the root scroll frame, so it seems to me the size will not be larger than the iframe's size unless a OOP iframe (which were defined as XUL?) is inside a chrome window and it has no root scroll frame.

If you have any concerns, please file a bug for that, I am not 100% whether I am correct since I am not familiar with the code.

Re comment 4:

I am less sure about this, it looks like we will regress bug 807174 (that the bug introduced the code) in OOP iframes. TYLin, what do you think about? (Mats still is on PTO)

I didn't take a look at this one, but :tnikkel has an answer in comment 5. I hope that helps ;)

Re comment 7:

:TYLin, same question for you for the AccessibleCaret stuff - for fission, which process is going to hold the AccessibleCaret and how will it be notified of scroll position updates?

I tested AccessibleCaret with fission enabled on desktop. I didn't notice anything broken yet.

Every PresShell holds an AccessibleCaret, so AcessibleCaret should just work regardless of which process (or document) it is in as long as the scroll position update can be notified via nsIScrollObserver interface.

Regarding the usage of disabling APZ in nsLayoutUtils::ShouldDisableApzForElement. Suppose we have a document A containing an iframe B, and AccessibleCaret is visible in B. When scrolling A, I think it's OK that the AccessibleCaret in B not receiving any scroll event because it just scrolls with every content in B together, right? Also, AcessibleCaret should now be able to determine when to disable APZ if needed via AccessibleCaretEventHub::ShouldDisableApz() [1], which should cover the case where AccessibleCaret is in scrollable subframes that are not the root scrollframe.

[1] https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/layout/base/nsLayoutUtils.cpp#1039-1047

Flags: needinfo?(aethanyc)

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #14)

I tested AccessibleCaret with fission enabled on desktop. I didn't notice anything broken yet.

Every PresShell holds an AccessibleCaret, so AcessibleCaret should just work regardless of which process (or document) it is in as long as the scroll position update can be notified via nsIScrollObserver interface.

Thanks, that sounds like it should work ok then.

Regarding the usage of disabling APZ in nsLayoutUtils::ShouldDisableApzForElement. Suppose we have a document A containing an iframe B, and AccessibleCaret is visible in B. When scrolling A, I think it's OK that the AccessibleCaret in B not receiving any scroll event because it just scrolls with every content in B together, right?

I think it should, yeah.

Also, AcessibleCaret should now be able to determine when to disable APZ if needed via AccessibleCaretEventHub::ShouldDisableApz() [1], which should cover the case where AccessibleCaret is in scrollable subframes that are not the root scrollframe.

Ok, sounds good. If it's in a scrollable subframe of the same document it will still be in the same process so that case doesn't matter for fission purposes.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)

so it seems to me the size will not be larger than the iframe's size

But the iframe itself could be really big. I think the use site in RestrictToRootDisplayPort will need to be updated to handle fission, if my understanding of that code is correct. I'll file a bug for it.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)

so it seems to me the size will not be larger than the iframe's size

But the iframe itself could be really big. I think the use site in RestrictToRootDisplayPort will need to be updated to handle fission, if my understanding of that code is correct. I'll file a bug for it.

You are absolutely right! Yeah, an iframe size can be width:200vw, height:200vh or some. Thanks for the correction and filing the bug!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

This function serves a very similar purpose to RestrictToRootDisplayPort: it calculates a quantity (FrameMetrics::mRootCompositionSize) on the main thread that is shipped over to APZ, and used to restrict the "display port base" that APZ uses to compute the display port size. The restriction is important in exactly the same case as RestrictToRootDisplayPort, i.e. cases where a scrollable subframe's viewport is larger than root viewport.

We can probably unify CalculateRootCompositionSize and part of RestrictToRootDisplayPort to compute a single quantity related to the root viewport size that's used to restrict the display port base in both Layout and APZ. The work in bug 1650183 would then apply to the unified code and make both functions fission-friendly. I'll make a note about this in bug 1650183.

Flags: needinfo?(botond)

Assigning to Hiro to shepherd this audit since he got the ball rolling on this :) Thanks to all who've taken the time to help investigate.

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED
Depends on: 1650981

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

  • in nsPresContext::RecordInteractionTime
    • the code was introduced for "Time to First Input" in bug 1307675, and it seems that with enabling fission the telemetry data will be also sent from cross process iframes. Harald, is that what we want? (it doesn't look like to me). If it's not, can you please file a bug for this?

I did chat with Harald and bdekoz, then filed bug 1650981.

Flags: needinfo?(hkirschner)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)

The last one is in nsLayoutUtils::ShouldDisableApzForElement and that one requires some answers from devtools/accessibility folks. The relevant use case for ShouldDisableApzForElement is that there is some content inside a subframe (such as a devtools overlay or the AccessibleCaret) that needs to move in sync with scrolling. So we disable APZ in those subframes. However in a fission world, I don't know how devtools overlays or AccessibleCaret work - they're not going to be getting regular scroll events either so the work needed here from the APZ side will depend on the architecture of those pieces for fission-friendliness.

Needinfo to :gl and :TYLin for this. :gl, can you explain (or redirect to somebody who can) how the problem described in this bug might be affected by fission? I don't recall the details of which process the highlighters are drawn from and what mechanism is used to notify them of updates in scroll position of subfrrames. :TYLin, same question for you for the AccessibleCaret stuff - for fission, which process is going to hold the AccessibleCaret and how will it be notified of scroll position updates?

I did chat with rcaliman on Matrix, and investigate it a bit. A key factor is this doc variable. On non Fission world, it's the top content document, on Fission world, it's the root document in each process, so nsLayoutUtils::ShouldDisableApzForElement should work for highlighers in Fission as well.

I believe now all call sites of GetToplevelContentDocumentPresContext have been audited and each bug is blocking an appropriate meta bug such as apz-fission, etc.

I am going to reuse this bug to rename GetToplevelContentDocumentPresContext to GetRootContentDocumentPresContextInProcess and will close this once after it landed.

Flags: needinfo?(rcaliman)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)

I am going to reuse this bug to rename GetToplevelContentDocumentPresContext to GetRootContentDocumentPresContextInProcess and will close this once after it landed.

kmag suggested to me on Matrix, InProcessRoot, so I am going to name it GetInProcessRootContentDocumenPresContext.

Attachment #9162348 - Attachment description: Bug 1525561 - Rename nsPresContext::GetInProcessRootContentDocumentPresContext to GetRootContentDocumentPresContextInProcess. → Bug 1525561 - Rename nsPresContext::GetToplevelContentDocumentPresContext to GetInProcessRootContentDocumentPresContext.
Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0f2f773a9163 Rename nsPresContext::GetToplevelContentDocumentPresContext to GetInProcessRootContentDocumentPresContext. r=heycam

https://hg.mozilla.org/mozilla-central/rev/0f2f773a9163

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: