1548247 - document splitting: Sluggish autoscrolling

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Darkspirit]

Attachment: 2019-05-01 15-14-07.mp4

Win10, GTX1060

mozregression --launch 2019-05-01 --pref gfx.webrender.all:true gfx.webrender.split-render-roots:true -a about:support

Comment 1

[Alex Thayer [:alexical] (she/her)]

Looks like the problem is here. Or rather, I think it's in the implementation of gfxUtils::RecursivelyGetRenderRootForFrame. I think this is failing to traverse the boundary between the about:support document and the browser element. Matt, what would be the best way to recurse all the way up to the chrome DOM if the document we're in is loaded in the parent process?

Comment 2

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

https://searchfox.org/mozilla-central/rev/8308eb7ea14318f53b55f3289c2bb9b712265318/gfx/thebes/gfxUtils.cpp#1511 should be using nsLayoutUtils::GetCrossDocParentFrame, probably

Comment 3

[Matt Woodrow (:mattwoodrow)]

Yes, GetCrossDocParentFrame should do what you want.

Comment 4

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1548247 - Correct and limit scroll update renderroot usage r?kats

There's two things going on here. 1) nsGfxScrollFrame is getting the
wrong renderroot, because it's not correctly recursing up the frame
tree. 2) Hiding behind that problem is that if we do correctly assign
the renderroot, we end up blocking on both render roots updating if
we don't, say, have a horizontal scroll option, because that leaves
us with a wr::RenderRoot::Default. 2.1) We then still end up blocking
on the default renderroot because we initialize the selector with
WebRenderBridgeParent's mRenderRoot.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1548247 - Use aGuid's RenderRoot in RecvStart/StopAutoscroll r?kats

The work for the antecedent patch led me to stumble on a problem where
we were not correctly stopping autoscroll. This was also due to a
renderroot mismatch, which this patch addresses. The call comes through
nsBaseWidget no matter what, it seems, so using mWrRootId.mRenderRoot
seems to be incorrect. I couldn't see a more elegant fix than this.

Depends on D31858

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1548247 - Fully recurse frame tree from APZCCallbackHelper r?kats

I wasn't able to produce a situation in which this change matters, so
I'm not certain that it's necessary, but it seems to be the correct
thing to do given the problem fixed in nsGfxScrollFrame.cpp.

Depends on D31863

Comment 7

[Alex Thayer [:alexical] (she/her)]

Kats, I posted patches that solve the issue (and another issue uncovered while investigating this - could make a separate bug or retitle this one). But there's a remaining issue where scrolling about:support causes the view to occasionally jerk back to the top for about a frame. I'm not certain what's causing this and I feel like you might be able to take a look at it and more quickly get to the root of it. Would you mind looking into this?

Comment 8

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

I took a quick look at the patches you have so far and they seem sane enough. I can assign this bug to myself for now and take a look at it in more detail at some point in the next week or two.

Comment 9

[Alex Thayer [:alexical] (she/her)]

Attachment: incorrectscrolling.mp4

Attaching a video of the scrolling problem I'm still seeing. It doesn't seem to be jumping all the way to to the top anymore, but hopefully the general jumpiness is visible. This seems to only occur for me on a non-PGO build.

Comment 10

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

I can see the jumpiness in your video, yeah. I can't repro locally. I tried both autoscrolling and wheel scrolling on a local build. (From the video it looks like you're wheel scrolling since there's no autoscroll indicator, but maybe the video recording tool just didn't pick that up).

At any rate, you said on IRC yesterday that you were able to repro this even without your patches applied, so there's no need to conflate the jumpiness issue with this bug, which is specifically about the autoscroll jankiness, and which is fixed by (a subset of) your first patch. I was under the impression based on comment 7 that the jumpiness was related to these patches, which is why I held off on reviewing the patches fully. But given it's unrelated I'll review the patches and turn this bug back over to you. Please file another bug for the jumpiness issue, and feel free to assign that to me - although without STR it might be tough for me to track it down.

Comment 11

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

(BTW if you don't have cycles to address my review comments feel free to bounce this back to me and I can land the patches)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c86eb07fe740
Correct and limit scroll update renderroot usage r=kats
https://hg.mozilla.org/integration/autoland/rev/bf1be740ee4a
Use aGuid's RenderRoot in RecvStart/StopAutoscroll r=kats
https://hg.mozilla.org/integration/autoland/rev/e249cd75138b
Fully recurse frame tree from APZCCallbackHelper r=kats

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/c86eb07fe740
https://hg.mozilla.org/mozilla-central/rev/bf1be740ee4a
https://hg.mozilla.org/mozilla-central/rev/e249cd75138b

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 15

[Alexandru Trif, Desktop QA [:atrif]]

Hello,
Reproduced the issue using Firefox 69.0a1 (20190501215533) on Windows 10x64.
The issue is verified fixed with Firefox 69.0b6 (20190718172058) on Windows 10x64. Autoscroll work as expected with the prefs gfx.webrender.all:true and gfx.webrender.split-render-roots:true in about:support.