document splitting: Sluggish autoscrolling

VERIFIED FIXED in Firefox 69

Status

()

defect
P2
normal
VERIFIED FIXED
4 months ago
27 days ago

People

(Reporter: darkspirit, Assigned: dthayer)

Tracking

(Blocks 2 bugs, {nightly-community, regression})

Trunk
mozilla69
x86_64
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67 unaffected, firefox68 disabled, firefox69 verified)

Details

Attachments

(5 attachments)

Win10, GTX1060

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

Blocks: wr-68
Priority: -- → P2
Assignee: nobody → dothayer

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?

Flags: needinfo?(matt.woodrow)

Yes, GetCrossDocParentFrame should do what you want.

Flags: needinfo?(matt.woodrow)
Summary: Sluggish autoscrolling → [webrender] Sluggish autoscrolling
Summary: [webrender] Sluggish autoscrolling → document splitting: Sluggish autoscrolling

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.

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

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

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?

Flags: needinfo?(kats)

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.

Assignee: dothayer → kats
Flags: needinfo?(kats)

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.

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.

Assignee: kats → dothayer

(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)

Pushed by dothayer@mozilla.com:
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

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

Keywords: regression
Flags: qe-verify+

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.