1561450 - Scrolling to the bottom on phoronix.com locks up scrolling

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Chris Peterson [:cpeterson]]

Steps to reproduce

1. Open https://www.phoronix.com/forums/forum/phoronix/latest-phoronix-articles/1099221-hands-on-with-the-atomic-pi-as-a-35-intel-atom-alternative-to-the-raspberry-pi
2. Scroll to the bottom
3. Attempt to scroll back up

Expected behavior

Scrolling back up works fine in Fennec and Chrome.

Actual behavior

Scrolling up is not possible in Fenix or RB.

Device information

Android device: Pixel 2
Fenix version: 1.0.1920

This bug was originally reported in the Fenix issue tracker:
https://github.com/mozilla-mobile/fenix/issues/2609

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can reproduce this. Bad handoff or something in APZ?

Comment 2

[Chris Peterson [:cpeterson]]

Jessie, can someone on the Graphics team please take a look at this Android bug?

Comment 3

[Chris Peterson [:cpeterson]]

[geckoview:fenix:m7] because the Fenix bug is a P1.

FWIW, I can't reproduce this bug in Fenix 6/27 on my Moto G5 Plus running Android 8.1.0.

Comment 4

[Miko Mynttinen]

Sounds very similar to bug 1556243.

Chris, does Fenix support Firefox profiler? If so, could we get a profile when the hang happens?

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Miko Mynttinen [:miko] from comment #4)

Chris, does Fenix support Firefox profiler? If so, could we get a profile when the hang happens?

Fenix does support the Firefox/Gecko Profiler. I wasn't able to reproduce this hang on my Moto G5 Plus in Fenix or R-B, so I can't gather a useful profile. I'll send this bug to the QF performance team who are experienced using the profiler.

Comment 6

[Jessie [:jbonisteel] pls NI]

Hey Jamie - curious to see if you can reproduce this?

Comment 7

[Jamie Nicol [:jnicol]]

Yes I can reproduce. It doesn't seem like a hang to me though, sometimes it scrolls up a few pixels then jumps back down. Zooming is also interesting: it zooms slightly and then gets stuck, but a new zoom gesture will resume it again.

This happens with or without webrender.

I'd guess it's a hit testing or apz problem. Maybe kats or botond would have a better idea

Comment 8

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

I can repro on Fenix. Will take a closer look. Leaving ni on me for now.

Comment 9

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

I can't reproduce on GVE though. Which means I can't really debug it locally. It might be a Fenix bug, or it might be a bug in the version of Gecko that Fenix is using, or something else. If somebody can make/find a test case that reproduces in recent GVE I can take a look.

Comment 10

[Markus Stange [:mstange]]

This does not look like a performance issue, just like a straight up bug, so I'm removing [qf].

Comment 11

[Jamie Nicol [:jnicol]]

I also couldn't reproduce in GVE, but could in Fenix and the Reference Browser. And the reason is because Fenix and the Reference Browser have tracking protection enabled by default. Disabling tracking protection makes the problem go away.

There is a cookie warning when you first go on to the page, which is hidden by tracking protection. Perhaps it is stealing the input events or something like that? Or the extra ads which appear at the bottom when tracking protection is disabled make the problem go away.

Comment 12

[Jamie Nicol [:jnicol]]

And likewise enabling tracking protection in Fennec (disabled by default) makes the problem appear.

Comment 13

[Chris Peterson [:cpeterson]]

Moving to the Tracking Protection component because this problem appears to be triggered by Tracking Protection. Ehsan wondered whether bug 1516552 (first contentful paint takes 4+ seconds because tracking protection doesn't support surrogate replacements) might be related.

Comment 14

[(no longer active)]

No, this one is unrelated.

Comment 15

[(no longer active)]

Looking at the web console, I see:

This site appears to use a scroll-linked positioning effect. This may not work well with asynchronous panning; see https://developer.mozilla.org/docs/Mozilla/Performance/ScrollLinkedEffects for further details and to join the discussion on related tools and features!

While looking very carefully, you'll see that that the scrolling getting stuck is actually a little bit of scrolling happening and then the page quickly snapping back into the same position, as was noted in comment 7. Both of these suggest some kind of bad interaction with APZ. It's "caused" by Tracking Protection in the sense that blocking some scripts has put the page in the path of running whatever the problematic code is, but presumably we want to try to fix the actual scrolling problem here? In case we may want to move the bug to APZ and try to reduce this page load under TP into a minimized test case.

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to :Ehsan Akhgari from comment #15)

While looking very carefully, you'll see that that the scrolling getting stuck is actually a little bit of scrolling happening and then the page quickly snapping back into the same position, as was noted in comment 7. Both of these suggest some kind of bad interaction with APZ. It's "caused" by Tracking Protection in the sense that blocking some scripts has put the page in the path of running whatever the problematic code is, but presumably we want to try to fix the actual scrolling problem here? In case we may want to move the bug to APZ and try to reduce this page load under TP into a minimized test case.

Thanks for testing, Ehsan. In that case, I will move this bug to the "Core :: Panning and Zooming" component.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jamie Nicol [:jnicol] from comment #11)

I also couldn't reproduce in GVE, but could in Fenix and the Reference Browser. And the reason is because Fenix and the Reference Browser have tracking protection enabled by default. Disabling tracking protection makes the problem go away.

There is a cookie warning when you first go on to the page, which is hidden by tracking protection. Perhaps it is stealing the input events or something like that? Or the extra ads which appear at the bottom when tracking protection is disabled make the problem go away.

May this be a scroll anchoring regression? Any chance that setting layout.css.scroll-anchoring.enabled to false fixes this?

Comment 18

[Jamie Nicol [:jnicol]]

Yes, disabling scroll anchoring also fixes this problem.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is this something we should keep on the radar for GV68, Chris? Seems like it'd be a wontfix for Fx68 otherwise.

Comment 20

[Chris Peterson [:cpeterson]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)

Is this something we should keep on the radar for GV68, Chris? Seems like it'd be a wontfix for Fx68 otherwise.

If this is a regression from scroll anchoring, then we've presumably had this bug since Fennec 66. I don't think we need to urgently fix this bug in a GV 68.0.x dot release. As for fixing this bug in Fennec ESR 68, I guess it depends on how invasive the fix is.

Comment 21

[Chris Peterson [:cpeterson]]

Ryan, this scroll anchoring bug is a P3, meaning it won't get fixed any time soon. But the bug behavior is pretty bad when it does happen because the site scrolling locks up. The user can no longer scroll the page. Do you think P3 is still an appropriate priority?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Ryan has moved to the WASM team and I've been taking over some of the scroll anchoring bugs lately. I can take a look at this... I've landed a few scroll anchoring fixes lately, would there be any chance to test with the latest nightly and see if it repros? In particular, bug 1543599 seems somewhat similar.

If it repros feel free to bounce the ni? back and I'll get to it. I agree this is not great.

STR for desktop if possible would be appreciated, since then I can take a look on rr which should speed up stuff considerably.

Comment 23

[Chris Peterson [:cpeterson]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)

Ryan has moved to the WASM team and I've been taking over some of the scroll anchoring bugs lately. I can take a look at this... I've landed a few scroll anchoring fixes lately, would there be any chance to test with the latest nightly and see if it repros? In particular, bug 1543599 seems somewhat similar.

Thanks. I'll ask the original bug reporter to test with GeckoView 70 Nightly. (We no longer have Fennec 70, but users can test GeckoView 70 using the Reference Browser app on the Google Play Store.)

https://github.com/mozilla-mobile/fenix/issues/2609

Comment 24

[Chris Peterson [:cpeterson]]

[geckoview:fenix:m7] bugs should be priority P1.

I'm editing a bunch of GeckoView bugs. If you'd like to filter all this bugmail, search and destroy emails containing this UUID:

e88a5094-0fc0-4b7c-b7c5-aef00a11dbc9

Comment 25

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #23)

I'll ask the original bug reporter to test with GeckoView 70 Nightly. (We no longer have Fennec 70, but users can test GeckoView 70 using the Reference Browser app on the Google Play Store.)

This hang is still reproducible in Fenix with GV 69 and R-B with GV 70.

Comment 26

[Chris Peterson [:cpeterson]]

This hang is still reproducible in Fenix with GV 69 and R-B with GV 70.

Emilio, what do you recommend as the next steps for this bug? The Fenix team considers this bug a high priority because it locks up page scrolling. However, we only know of one affected website.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

I can repro and can look into it.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

So what's going on is something like the following. The page has a minified script which roughly looks after beautifying like this:

$(window).on("scroll." + I, function() {
    if (N.is(":hidden")) {
        return
    }
    var U = D(this).scrollTop();
    if (H.limit > 1 && U > H.limit && N.hasClass(L)) {
        N.removeClass(L).css({
            position: "absolute",
            left: "auto",
            right: "auto",
            top: "auto",
            bottom: "auto",
            width: M.width
        })
    } else {
        if ((H.limit > 0 && U < H.limit && U > M.offset.top && !N.hasClass(L)) || (H.limit <= 0 && U > M.offset.top && !N.hasClass(L))) {
            P.css({
                display: N.css("display"),
                width: N.outerWidth(true),
                height: N.outerHeight(true),
                "float": N.css("float")
            });
            var R = (vBulletin.Responsive && vBulletin.Responsive.scrollToFixedBreakPoint) ? vBulletin.Responsive.scrollToFixedBreakPoint : 0;
            if (D("#vb-page-body").width() > R) {
                N.addClass(L).width(M.width).css({
                    left: M.offset.left,
                    right: "auto",
                    width: M.width,
                    top: (H.fixedAt == "top") ? 0 : "auto",
                    bottom: (H.fixedAt == "bottom") ? 0 : "auto",
                    position: ""
                })
            } else {
                P.hide()
            }
            var T = M["margin-" + H.fixedAt];
            if (T) {
                var S = {};
                S[H.fixedAt] = -T + "px";
                N.animate(S, "fast")
            }
        } else {
            if (U <= M.offset.top && N.hasClass(L)) {
                P.hide();
                N.removeClass(L).css({
                    position: "",
                    width: "",
                    left: "",
                    top: "",
                    right: "",
                    bottom: ""
                })
            }
        }
    }
})

For the record, M here is .scrolltofixed-floating, the floating bar on bigger screens, which is hidden on the smaller viewport sizes where the bug reproduces.

By the time we get to the bottom, we start hitting the first else branch (so we show the .scrolltofixed-filler element, #hidden in the example). Then we update layout (calling width() in the original script, calling offsetTop in the reduced test-case, doesn't matter), then we hide it again (second else branch). The bar has no margin so nothing else happens.

The reduced test-case also breaks Chrome pretty badly.

So why doesn't it show up in Chrome, or when tracking protection is completely disabled? Because when you get to the bottom they load a bunch of ads, which make the scroll range big enough again, and those ads conveniently don't get selected as anchor nodes so this doesn't happen.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

So the good part is that Chrome also has the same issue, though it happens not to happen in this website because of Ads.

The bad part is that I'm not quite sure what the best way to fix this is yet.

Comment 30

[Emilio Cobos Álvarez (:emilio)]

I don't have better ideas than bug 1529702 to fix this, tbh.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Hmm, well, maybe I do.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1561450 - Suppress scroll anchoring adjustments from scroll event listeners. r=dholbert

See https://github.com/w3c/csswg-drafts/issues/4239

This fixes what is in my opinion one of the biggest issues of scroll anchoring
as specified / currently implemented.

It's trivial to flush layout on a scroll handler and create scroll adjustments,
which in turn would trigger other scroll events to be fired. This situation,
which is what has happened in most of the scroll anchoring regressions I've
fixed, at best gets pages to get stuck in an infinite CPU loop. At worst, it
causes scrolling to be unusable because the page keeps reacting to scroll
events.

An alternative, slightly more elegant but not clear to me if 100% implementable
approach would be bug 1529702.

This should enormously reduce the need for scroll anchoring suppression
triggers[1], I'd think, which in turn would allow me to investigate removing
some of that complexity.

https://drafts.csswg.org/css-scroll-anchoring/#suppression-triggers

The setup to set / unset the boolean is a bit awkward but I couldn't come up
with anything better.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Filed https://github.com/w3c/csswg-drafts/issues/4239

Comment 34

[Chris Peterson [:cpeterson]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #29)

So the good part is that Chrome also has the same issue, though it happens not to happen in this website because of Ads.

If this bug also affects Chrome, then maybe we can just notify the website author and reduce the priority of this bug..? I don't know how common this scroll handler case might be on other websites.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cdcb21809ab6
Suppress scroll anchoring adjustments from scroll event listeners. r=dholbert

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18655 for changes under testing/web-platform/tests

Comment 37

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 38

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/cdcb21809ab6

Comment 39

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot