document splitting: Sluggish autoscrolling
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox66 | --- | unaffected |
firefox67 | --- | unaffected |
firefox68 | --- | disabled |
firefox69 | --- | verified |
People
(Reporter: jan, Assigned: dthayer)
References
Details
(Keywords: nightly-community, regression)
Attachments
(5 files)
Win10, GTX1060
mozregression --launch 2019-05-01 --pref gfx.webrender.all:true gfx.webrender.split-render-roots:true -a about:support
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
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•5 years ago
|
||
https://searchfox.org/mozilla-central/rev/8308eb7ea14318f53b55f3289c2bb9b712265318/gfx/thebes/gfxUtils.cpp#1511 should be using nsLayoutUtils::GetCrossDocParentFrame, probably
Comment 3•5 years ago
|
||
Yes, GetCrossDocParentFrame should do what you want.
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Comment 5•5 years ago
|
||
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
Assignee | ||
Comment 6•5 years ago
|
||
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
Assignee | ||
Comment 7•5 years ago
|
||
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•5 years ago
|
||
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 | ||
Comment 9•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
(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•5 years ago
|
||
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
Comment 13•5 years ago
|
||
bugherder |
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•5 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Updated•5 years ago
|
Comment 15•5 years ago
|
||
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.
Description
•