1395670 - Scrollbar runs away when APZ-scrolling in layers-free WR mode

Status: RESOLVED FIXED

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

Description

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

Scrolling on many pages (e.g. https://news.ycombinator.com/news for a concrete example) will result in the scrollbar outpacing the actual scroll distance. This can also be reproduced in the layout/reftests/async-scrolling/position-fixed-transformed-1.html reftest, if it is run with layers-free WR enabled.

I believe the same root cause is also responsible for the event regions running away from where they should be, which results in bad hit-testing and scrolling not working when it should.

I tracked down the problem, it's a regression from bug 1394011. The code in GetRootMetadata has a bit that checks if the scrollmetadata is already in layer tree, and sets ensureMetricsForRootId to false in that case. This code isn't running in layers-free WR mode because we don't have a layer tree. However it is still needed. Without it we end up with the root scroll id's metadata in two (maybe more) places in the APZ tree, and that results in this weird behaviour.

I have patches that I'm testing.

Comment 1

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

Attachment: Bug 1395670 - Update HitTestingTreeNode logging to indicate scrollbar and scrollthumb nodes.

Review commit: https://reviewboard.mozilla.org/r/175066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175066/

Comment 2

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

Attachment: Bug 1395670 - In webrender layers-free mode, don't add a root scroll metadata if we already have it elsewhere in the tree.

Review commit: https://reviewboard.mozilla.org/r/175068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175068/

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=542cb7a1d1038cd383f5585cf01a71f8658e04ac

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8903273 [details]
Bug 1395670 - Update HitTestingTreeNode logging to indicate scrollbar and scrollthumb nodes.

https://reviewboard.mozilla.org/r/175066/#review180134

Comment 5

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

Comment on attachment 8903273 [details]
Bug 1395670 - Update HitTestingTreeNode logging to indicate scrollbar and scrollthumb nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175066/diff/1-2/

Comment 6

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

Comment on attachment 8903274 [details]
Bug 1395670 - In webrender layers-free mode, don't add a root scroll metadata if we already have it elsewhere in the tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175068/diff/1-2/

Comment 7

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

This time without capturing a raw pointer to a refcounted object in a lambda:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cb870867b4f476637cd98ea08818f8da5e47e5b

Comment 8

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

And now passing a const-ref of std::function instead of the std::function. Some compilers are so picky.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd4c6e0352ba97cce90028c1a1320c552d56d3aa

Comment 9

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (Away Sep02-Sep10) (email:[e-mail]) from comment #8)
> And now passing a const-ref of std::function instead of the std::function.
> Some compilers are so picky.

That's actually our own static analysis [1] :)

[1] http://searchfox.org/mozilla-central/source/build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp#100

Comment 10

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

Comment on attachment 8903273 [details]
Bug 1395670 - Update HitTestingTreeNode logging to indicate scrollbar and scrollthumb nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175066/diff/2-3/

Comment 11

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

Comment on attachment 8903274 [details]
Bug 1395670 - In webrender layers-free mode, don't add a root scroll metadata if we already have it elsewhere in the tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175068/diff/2-3/

Comment 12

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

(In reply to Botond Ballo [:botond] from comment #9)
> That's actually our own static analysis [1] :)

Huh, I'm surprised that showed on the OS X build but not the linux64 static analysis build, then.

Comment 13

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (Away Sep02-Sep10) (email:[e-mail]) from comment #12)
> (In reply to Botond Ballo [:botond] from comment #9)
> > That's actually our own static analysis [1] :)
> 
> Huh, I'm surprised that showed on the OS X build but not the linux64 static
> analysis build, then.

It looks like the Linux64 static analysis build uses libstdc++ as the standard library, while the OS X build uses libc++.

The two libraries have different std::function implementations, and one of them triggers our checker and the other doesn't.

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8903274 [details]
Bug 1395670 - In webrender layers-free mode, don't add a root scroll metadata if we already have it elsewhere in the tree.

https://reviewboard.mozilla.org/r/175068/#review180540

Comment 15

[Markus Stange [:mstange]]

It would be nice to teach our static analysis the difference between a callback function that is only passed to something in order to be invoked synchronously during that call, and a callback function that is stored away somewhere.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c70beb7528f
Update HitTestingTreeNode logging to indicate scrollbar and scrollthumb nodes. r=botond
https://hg.mozilla.org/integration/autoland/rev/1027120d021b
In webrender layers-free mode, don't add a root scroll metadata if we already have it elsewhere in the tree. r=mstange

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c70beb7528f
https://hg.mozilla.org/mozilla-central/rev/1027120d021b