Closed Bug 1548247 Opened 2 years ago Closed 1 year ago

document splitting: Sluggish autoscrolling


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

Windows 10



Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox68 --- disabled
firefox69 --- verified


(Reporter: jan, Assigned: dthayer)



(Keywords: nightly-community, regression)


(5 files)

Attached video 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

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)
Attached video 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.

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
Correct and limit scroll update renderroot usage r=kats
Use aGuid's RenderRoot in RecvStart/StopAutoscroll r=kats
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+

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.

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