This seems to be one of the cases of bug 1722583: We call ActivityOccurred even on scroll frames without scrollbars.

Backed out for causing reftest failures on iframe-modify-scrolling-attr-to-yes.html.

Push with failures

Failure log for wr3
Failure log for R2

Backout link

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mstange, could you have a look please?
For more information, please visit auto_nag documentation.

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #4)

There's a r+ patch which didn't land

It did land, but it was backed out.

There are two Android-only test failures. That makes sense because Android is the only platform where we test the overlay scrollbar configuration.
The test failures are in layout/reftests/scrolling/iframe-scrolling-attr-2.html and in iframe-modify-scrolling-attr-to-yes.html. In both tests, the difference is that the reference has visible scrollbars but the test does not.

My guess is that in the past, we were showing scrollbars in both the test and the reference. And somehow the patch makes us not show the scrollbars in the test anymore. I don't have an idea yet for why that would happen.

I tried to reproduce one of the test failures on macOS with overlay scrollbars enabled, but the test was passing. I don't remember which of the two tests I tried.

The next steps would be to: Try both tests again on macOS; maybe debug the good state to get an idea for which NotifyActivity call is needed and why it might be missing in the bad state; set up an Android testing and debugging environment if all else fails.

Hi Botond, I think this bug is somewhat important to fix but I don't think I'll have time for a deeper investigation. Do you think someone from the APZ team could take it?

(In reply to Markus Stange [:mstange] from comment #7)

I think this bug is somewhat important to fix

Could you say a bit more about what the user-noticeable effects of the bug are? (Based on bug 1722583, it looks like performance? Is it noticeable though?)

It directly affects power usage to some degree because we're ticking the refresh driver unnecessarily (which comes with CPU wakeups and CSS restyle costs, example profile). Fixing this also has an indirect benefit of making it easier to find other stray refresh observers, because the profiler marker chart won't be cluttered by the scrollbar ones. This would help the power usage effort.

Discussed this in today's APZ planning meeting. We're going to try and look at it as part of our current "stabilization sprint".

Note that I just tried to run the reftests locally with D121784 on local Android emulator, the test passed properly. I pushed a new try run to see if it still fails on try. https://treeherder.mozilla.org/jobs?repo=try&revision=3b1eedb8637fe7d807f97c1e7d62dc98b242b60c

I am idiot. Now I can see the failure locally. An excuse is that I have a local cloned repo for Android build but last night moz-phab broke the repo for some reasons so I had to re-clone the repo locally and forgot restoring .mozconfig there. :/

I did track down what the difference between desktops with ui.useOverlayScrollbars=1 and Android is. The difference is this if (gfxPlatform::UseDesktopZoomingScrollbars()) {} block in ScrollFrameHelper::ReflowFinished.

On Android apz.allow_zoomiing pref is disabled both on wpt reftest and our in-tree reftest, thus the block doesn't run through on Android, so the scroll range for iframes in question is wrongly computed in the first place in the case of scrolling=no iframes. Thus when the scrolling attribute is changed to "yes", we unfortunately bail out from this aElement->AttrValueIs() check in ScrollFrameHelper::SetCoordAttribute, thus ScrollbarActivity->ActivityOccurred() wont't get called.

A couple of comments I can tell;

1. D121784 revealed a pre-existing issue in ScrollFrameHelper::ReflowFinished() with disabling desktop zooming, you can easily see same failures on desktop with ui.useOverlayScrollbars=1 and apz.allow_zooming=false
2. Given that desktop zooming has been enabled for a while by default, the test failures are not a big problem
3. We should take some amount of time to make our in-tree reftests and wpt reftests pass with apz.allow_zooming=true since it's by default

I think we can land D121784 with explicit apz.allow_zooming=true for the reftests, but I'd defer the final decision to Markus or Botond.

Thanks for investigating!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)

2. Given that desktop zooming has been enabled for a while by default, the test failures are not a big problem

Agreed.

3. We should take some amount of time to make our in-tree reftests and wpt reftests pass with apz.allow_zooming=true since it's by default

Yep, I think that's a good idea. We have bug 1650080 on file for our reftests.

I think we can land D121784 with explicit apz.allow_zooming=true for the reftests, but I'd defer the final decision to Markus or Botond.

+1

I did upload the new changeset with the pref setting.

I've also pushed a try run to see if it's okay; https://treeherder.mozilla.org/jobs?repo=try&revision=0f6d26c02c233f8858e5e4cb65b74679cfb875a4, there are two unknown failures, but they are not related to the changeset, I did double-check in another try without the change; https://treeherder.mozilla.org/jobs?repo=try&revision=3976f5310bb1e315a5ddab8c2ee8613278579b03, there are also two failures.

Backed out changeset 9682c9f452e6 (bug 1724088) for causing android wr failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/82563b380afb5b8952e044a0f0f5d29a1585aa89

Push with failures

Failure log WR2

Failure log WR6

Looks like I did push the try run based on a wrong revision. :/

(In reply to Sandor Molnar from comment #17)

Failure log WR2

This one, grid-container-auto-margins-scrollbars-001.html is actually another test case we need to specify apz.allow_zooming pref.

Failure log WR6

But, this one isn't. This one looks odd, it's scaled as if the minimum-scale viewport is applied.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)

(In reply to Sandor Molnar from comment #17)

Failure log WR2

This one, grid-container-auto-margins-scrollbars-001.html is actually another test case we need to specify apz.allow_zooming pref.

Failure log WR6

But, this one isn't. This one looks odd, it's scaled as if the minimum-scale viewport is applied.

Filed bug 1755598 for this issue. There seems to be an issue in our wpt runner that pref values once set in a .ini file persists.

That said, given that we'd eventually want to enable apz..allow_zooming all over the place (bug 1650080). The test should be passed with apz.allow_zooming=true.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)

Failure log WR6

But, this one isn't. This one looks odd, it's scaled as if the minimum-scale viewport is applied.

Filed bug 1755598 for this issue. There seems to be an issue in our wpt runner that pref values once set in a .ini file persists.

That said, given that we'd eventually want to enable apz..allow_zooming all over the place (bug 1650080). The test should be passed with apz.allow_zooming=true.

Filed bug 1755600.

I've pushed a new try run to make sure there's no failure caused by this change; https://treeherder.mozilla.org/jobs?repo=try&revision=cabd6a2afc46df847e7de4254d0a7fda2f496645. There are two failures, but I believe both are known, bug 1607713 and bug 1712012. I'd hope there's no more overlooked failures.

https://hg.mozilla.org/mozilla-central/rev/80dea4887372