1647156 - Missing content in nested scroll frame (page info popup in Firefox profiler)

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Markus Stange [:mstange]]

Attachment: screen recording

STR:

1. With WebRender enabled, go to https://share.firefox.dev/3fJYDAH .
2. Open the page info popup by clicking the "Firefox Nightly (79.0) Android 5.1" button at the top.
3. Scroll down in the popup.

Expected results:
All text should appear normally.

Actual results:
During scrolling, part of the text pops in too late or disappears to early.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This is svg filtered content?

Comment 2

[Markus Stange [:mstange]]

Yes

Comment 3

[Darkspirit]

Gnome Xwayland, Debian Testing, Macbook Pro
mozregression --good 2020-05-20 --bad 2020-06-20 --pref gfx.webrender.all:true -a https://share.firefox.dev/3fJYDAH

9:07.19 INFO: Last good revision: f1a9f9c468088583e895a9200066187968e4c5a9
9:07.19 INFO: First bad revision: 7d080876c7e29a86d025b3b5c73108f27dc4f6d6
9:07.19 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f1a9f9c468088583e895a9200066187968e4c5a9&tochange=7d080876c7e29a86d025b3b5c73108f27dc4f6d6

7d080876c7e29a86d025b3b5c73108f27dc4f6d6 Nicolas Silva — Bug 1635472 - Move the displayport in larger increments with WebRender. r=kats

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Wasn't expecting that to show up as the regressing bug.

Comment 5

[Darkspirit]

The regression range contains one other commit (bug 1580766 comment 21), but I didn't quote it because its backout commit (bug 1580766 comment 22) didn't fix the bug.

Comment 6

[Markus Stange [:mstange]]

This happens even without any filters. (I unset both filter declarations using the devtools, so that there was no shadow, and the issue still appeared.)

Comment 7

[Nicolas Silva [:nical]]

Attachment: Bug 1647156 - Improve the displayport snapping logic. r=kats

aligning the displayport position in large increments can cause issues when the displayport is small enough to be smaller than the alignment itself. The risk is higher for small frames with no displayport margin as the alignment might have previously be rounded up to 512. Luckily not having margins is an indication that we aren't trying hard to benefit from APZ and a large alignment isn't benefiting us. This patch takes that into consideration and defaults to small alignements when there is no margins. The alignment is also rounded to a multiple of 256 instead of 512 to further reduce the likelihood of running into issues with small displayports.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I looked at how to go about testing this. There is an existing mochitest at gfx/layers/apz/test/mochitest/helper_hittest_hoisted_scrollinfo.html which was added to test a different bug encountered on that same profiler dropdown panel, so we can use that as a starting point. I verified that by making a couple of small changes to the sizes there I can replicate the checkerboarding. And I also verified that the visually-observed checkerboard gets captured in about:checkerboard as checkerboard events, so it should be detectable programatically via APZ's internal checkerboard detector, which means we should be able to catch it in a test.

1. Copy gfx/layers/apz/test/mochitest/helper_hittest_hoisted_scrollinfo.html to gfx/layers/apz/test/mochitest/helper_checkerboard_filteredpanel.html or something.
2. Add an entry to the subtests for test_group_checkerboarding.html for it.
3. Modify the new copy with the following diff. Verify that if you open this file in a WR-enabled nightly browser and scroll the scroller you see it visually checkerboard. In my case if I set the scroller's scrollTop to 175 there's a good amount of checkerboard there.

@@ -13,12 +13,12 @@

     #scroller {
         width: 300px;
-        height: 500px;
+        height: 518px;
         overflow: scroll;
     }

     .spacer {
-        height: 1000px;
+        height: 939px;
         background-image: linear-gradient(red, blue);
     }
 </style>

4. Modify the <script> inside the subtest - I think the test(testDriver) function should just need to document.querySelector('#scroller').scrollTop = 175; to trigger the checkerboard, and then copy the stuff here to make the test verify there's no checkerboarding.
5. Run the test without the patch to ensure it fails. Hopefully it does but I didn't try this myself, and maybe the checkerboard detection code needs to beefed up. If you run into problems let me know, I can help debug.

Comment 11

[Darkspirit]

This is a severe regression that must be fixed with 79.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Yeah, I agree. I hit this semi often and it looks really bad. It can happen even in the URL bar (just happened to me with a large data: uri).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/820d52d0d3bc
Improve the displayport snapping logic. r=kats

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/820d52d0d3bc

Comment 15

[Nicolas Silva [:nical]]

Comment on attachment 9158451 [details]
Bug 1647156 - Improve the displayport snapping logic. r=kats

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect rendering
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple change.
• String changes made/needed: None.

Comment 16

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 9158451 [details]
Bug 1647156 - Improve the displayport snapping logic. r=kats

approved for 79.0b4

Comment 17

[Julien Cristau [:jcristau] (back August 18)]

https://hg.mozilla.org/releases/mozilla-beta/rev/367d47729181