ScrollFrameHelper::RestrictToRootDisplayPort uses nsPresContext::GetToplevelContentDocumentPresContext which is not fission-friendly. This will probably impact cases where an OOP iframe is very large (much larger than the window size rendering the top-level document) as we won't be clamping the OOPIF's displayport to the root displayport. This in turn can result in ~unbounded memory usage.

nsLayoutUtils::CalculateRootCompositionSize suffers from the same problem.

As explained in bug 1525561 comment 18, these two functions basically serve the same purpose (limit the display port base size of a subframe with a larger viewport than the root viewport), and could share more code (and the fission-friendliness fix can be made to the shared code, so we only have to fix it once).

This bug is a Fission Nightly blocker (milestone M6c).

Botond, who can work on this for Fission M6c?

I brought this bug up at a recent APZ planning meeting. Based on the Fission schedule, which shows M6c targeting 84 (though at the time it may have showed a month like November), we said we'd prioritize this after desktop zooming ships (currently targeting 82).

I'll bring it up again at a subsequent APZ planning meeting and make sure it gets an assignee and gets fixed in time.

A few observations based on code reading:

• RestrictToRootDisplayPort and CalculateRootCompositionSize serve similar purposes: to limit the size of subframe displayports in cases where the subframe's viewport is very large (specifically, larger than the root displayport). However, they come into play in slightly different ways.
• RestrictToRootDisplayPort is used to limit the size of the displayport base rect to which the main thread applies the displayport margins to compute the displayport rect. If we fail to limit this, the base rect could be huge, and therefore the displayport rect could be huge.
• CalculateRootCompositionSize is used to limit what APZ thinks is the base rect when computing the displayport margins. If we fail to limit this, the displayport margins could be huge (since APZ sizes them by applying a multiplier to the base rect size), and therefore the displayport rect again could be huge.
• As suggested by the name, RestrictToRootDisplayPort uses the root displayport as the limiting size. This is relatively new, as of bug 1617427; before that, it used the root viewport (also called the root composition size).
• As suggested by the name, CalculateRootCompositionSize has not been updated to use the root displayport size (but it should be). Once updated thusly, we can probably refactor the two functions to share code.
• Unlike the root composition size, the root displayport size is not something that's likely to be currently propagated to OOP content processes, so we'll have to add that in this bug.

I'll start by writing a test that creates an OOP iframe with a large viewport, and uses the APZ test data mechanism to query the displayport we compute for it, which with the current code should be too large.

Though I am not sure about what the limited size is suitable for OOP iframe-d documents, BrowserChild::GetVisibleRect() might be a candidate limitation for the OOP iframe case as an interim solution, and if we see issues something like graphical glitches in future we can propagate the root displayport size to OOP documents later in a followup bug.

Botond, asking for your progress on this for Fission M6c (Feb 8).

I'm hoping to have a test for this by the end of this week and a fix next week.

The attached test is failing in the expected way (the displayport is too large). I will work on the fix next.

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

The attached test is failing in the expected way (the displayport is too large). I will work on the fix next.

Botond, do you have an ETA for landing this patch with the test fix?

This week, as mentioned in comment 8.

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

I'm hoping to have a test for this by the end of this week and a fix next week.

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

This week, as mentioned in comment 8.

Sounds good! Sorry I missed your earlier comment.

So, while investigating a fix, I realized that there isn't actually a problem with RestrictToRootDisplayPort().

While the restricting done inside that function does use the in-process RCD's displayport, the incoming rect is already restricted to the cross-process RCD's displayport. (This happens because it's restricted to the visible rect, which in turn is restricted to BrowserChild::GetVisibleRect(). This was added in the M4 bug 1558482.)

However, CalculateRootCompositionSize() does need fixing (and is what's causing the test to fail).

Depends on D103538

Backed out for causing iframe displayport failures

backout: https://hg.mozilla.org/integration/autoland/rev/b3fe2cfc821b57b2e74f577dd8a630f97f400a36

push: https://treeherder.mozilla.org/jobs?repo=autoland&test_paths=gfx%2Flayers%2Fapz%2Ftest%2Fmochitest%2Fbrowser.ini&revision=1955e50f311e1493c23eb735a01886bc2752b6c2&selectedTaskRun=PbK-DW25TYOt56Ysgczv2A.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=328873326&repo=autoland&lineNumber=7187

[task 2021-02-04T19:00:44.402Z] 19:00:44 INFO - TEST-PASS | gfx/layers/apz/test/mochitest/browser_test_group_fission.js | http://mochi.test:8888/browser/gfx/layers/apz/test/mochitest/helper_fission_large_subframe.html | expected at least one nonempty paint -
[task 2021-02-04T19:00:44.402Z] 19:00:44 INFO - Buffered messages finished
[task 2021-02-04T19:00:44.404Z] 19:00:44 INFO - TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/browser_test_group_fission.js | http://mochi.test:8888/browser/gfx/layers/apz/test/mochitest/helper_fission_large_subframe.html | iframe displayport should be no more than twice as tall as page displayport -
[task 2021-02-04T19:00:44.404Z] 19:00:44 INFO - Stack trace:
[task 2021-02-04T19:00:44.404Z] 19:00:44 INFO - chrome://mochikit/content/browser-test.js:test_ok:1323
[task 2021-02-04T19:00:44.404Z] 19:00:44 INFO - chrome://mochitests/content/browser/gfx/layers/apz/test/mochitest/FissionTestHelperParent.jsm:receiveMessage:45
[task 2021-02-04T19:00:44.406Z] 19:00:44 INFO - GECKO(2335) | [Child 3184: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0x7fc6ff5db000 == 1 [pid = 3184] [id = 1] [url = about:blank]

Interesting. I did do a Try push before landing and the test was passing there.

I can reproduce the failure locally. It looks like the comparison is failing because the page is getting empty displayport margins (while the iframe gets regular ones, which makes the iframe's displayport more than twice as tall as the page's).

I think the behaviour might have changed as a result of something that landed on m-c recently, possibly one of {bug 1687926, bug 1687927, bug 1687886}.

What I'm seeing is:

• The page gets initial, empty displayport margins in InitializeRootDisplayport.
• Subsequently, MaybeCreateDisplayPort identifies the page's RSF as eligible for getting non-empty margins due to being the first encountered async-scrollable frame, but it doesn't set them because it already has margins (it doesn't check for emptiness).

This is a bit suspicious and worth further investigation (as we do want the first-encountered async-scrollable frame to get non-empty displayport margins on page load), but it's fairly orthogonal to this bug, and easy to work around in the test (by doing something that triggers the page getting non-empty margins more explicitly), so I'll work around it and reland.

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

• Subsequently, MaybeCreateDisplayPort identifies the page's RSF as eligible for getting non-empty margins due to being the first encountered async-scrollable frame, but it doesn't set them because it already has margins (it doesn't check for emptiness).

Good to know! This would be a point in favour of using minimal display ports every where we set zero margin display ports as that would fix this issue. We should follow up on this.

(In reply to Timothy Nikkel (:tnikkel) from comment #21)

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

• Subsequently, MaybeCreateDisplayPort identifies the page's RSF as eligible for getting non-empty margins due to being the first encountered async-scrollable frame, but it doesn't set them because it already has margins (it doesn't check for emptiness).

Good to know! This would be a point in favour of using minimal display ports every where we set zero margin display ports as that would fix this issue. We should follow up on this.

Filed bug 1691160 for further investigation of this.

https://hg.mozilla.org/mozilla-central/rev/603d8cc037d3
https://hg.mozilla.org/mozilla-central/rev/71ab3cea137a