1519621 - Perma [tier2] /css/cssom-view/scroll-behavior-smooth.html | Smooth scrolling while doing history navigation. - assert_not_equals: Shouldn't be scrolled to top anymore. got disallowed value 0

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P2)

Description

[Treeherder Bug Filer]

Filed by: nbeleuzu [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=221476784&repo=mozilla-central

https://queue.taskcluster.net/v1/task/JcjfZs94RlCyESr-sU85xw/runs/0/artifacts/public/logs/live_backing.log

[task 2019-01-12T10:31:37.092Z] 10:31:37 INFO - TEST-PASS | /css/cssom-view/scroll-behavior-smooth.html | Instant scrolling while doing history navigation.
[task 2019-01-12T10:31:37.092Z] 10:31:37 INFO - TEST-UNEXPECTED-FAIL | /css/cssom-view/scroll-behavior-smooth.html | Smooth scrolling while doing history navigation. - assert_not_equals: Shouldn't be scrolled to top anymore. got disallowed value 0
[task 2019-01-12T10:31:37.092Z] 10:31:37 INFO - smooth/window.onhashchange/<@http://web-platform.test:8000/css/cssom-view/scroll-behavior-smooth.html:126:9
[task 2019-01-12T10:31:37.092Z] 10:31:37 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1566:20
[task 2019-01-12T10:31:37.092Z] 10:31:37 INFO - smooth/window.onhashchange@http://web-platform.test:8000/css/cssom-view/scroll-behavior-smooth.html:124:7
[task 2019-01-12T10:31:37.092Z] 10:31:37 INFO - TEST-OK | /css/cssom-view/scroll-behavior-smooth.html | took 979ms
[task 2019-01-12T10:31:37.863Z] 10:31:37 INFO - Closing logging queue
[task 2019-01-12T10:31:37.873Z] 10:31:37 INFO - queue closed
[task 2019-01-12T10:31:37.880Z] 10:31:37 INFO - Setting up ssl
[task 2019-01-12T10:31:37.907Z] 10:31:37 INFO - certutil |
[task 2019-01-12T10:31:37.931Z] 10:31:37 INFO - certutil |
[task 2019-01-12T10:31:37.946Z] 10:31:37 INFO - certutil |
[task 2019-01-12T10:31:37.946Z] 10:31:37 INFO - Certificate Nickname Trust Attributes
[task 2019-01-12T10:31:37.947Z] 10:31:37 INFO - SSL,S/MIME,JAR/XPI
[task 2019-01-12T10:31:37.947Z] 10:31:37 INFO -
[task 2019-01-12T10:31:37.947Z] 10:31:37 INFO - web-platform-tests CT,,
[task 2019-01-12T10:31:37.947Z] 10:31:37 INFO -
[task 2019-01-12T10:31:39.798Z] 10:31:39 INFO - adb Granting important runtime permissions to org.mozilla.fennec_aurora
[task 2019-01-12T10:31:41.681Z] 10:31:41 INFO - adb launch_application: am start -W -n org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp -a android.intent.action.VIEW --es env9 MOZ_PROCESS_LOG=/tmp/tmpPJj18Dpidlog --es env8 MOZ_CRASHREPORTER_NO_REPORT=1 --es args "-no-remote -profile /sdcard/tests/profile --marionette about:blank" --es env3 STYLO_THREADS=4 --es env2 MOZ_HIDE_RESULTS_TABLE=1 --es env1 R_LOG_VERBOSE=1 --es env0 MOZ_CRASHREPORTER=1 --es env7 MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 --es env6 R_LOG_DESTINATION=stderr --es env5 MOZ_CRASHREPORTER_SHUTDOWN=1 --es env4 MOZ_LOG=signaling:3,mtransport:4,DataChannel:4,jsep:4,MediaPipelineFactory:4 --es env10 R_LOG_LEVEL=6
[task 2019-01-12T10:31:43.470Z] 10:31:43 INFO - Starting runner

Comment 1

[Intermittent Failures Robot]

8 failures in 2627 pushes (0.003 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 8

Platform breakdown:

• android-em-7-0-x86: 8

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1519621&startday=2019-01-07&endday=2019-01-13&tree=all

Comment 2

[Cosmin Sabou [:CosminS]]

This is permafailing since this merge to central: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=testfailed%2Cexception%2Csuccess&searchStr=android%2C7.0%2Cx86%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-android-em-7.0-x86%2Fopt-web-platform-tests-23%2Cw%28wpt23%29&revision=cb35977ae7a4a5a1d2fe6a543e15d49cd2fd47c8

This is the pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=cb35977ae7a4a5a1d2fe6a543e15d49cd2fd47c8
and it looks like Bug 1498812 might be the culprit here.
Jan, can you please take a look over this? Thank you.

Comment 3

[Jan Henning [:JanH]]

Maybe something similar to bug 1509575 comment 20? Hmm...

Comment 4

[Jan Henning [:JanH]]

Whoops, didn't mean to clear the flag.

Comment 5

[Botond Ballo [:botond]]

(In reply to Jan Henning [:JanH] from comment #3)

Maybe something similar to bug 1509575 comment 20? Hmm...

The failure started before bug 1509575 landed. Looking at the test, it seems to happen before we even perform the history navigation.

Comment 6

[Jan Henning [:JanH]]

Yes, through backing out it seems that something in the PresState/nsGfxScrollFrame changes in https://hg.mozilla.org/integration/autoland/rev/a99bf382e5f7 seems to be interfering.

We do actually set the layout scroll position and trigger a scroll event, but for some reason by the time the test queries the scroll position we return 0,0 again.

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)

it seems to happen before we even perform the history navigation.

But RestoreState() is still being called before the failure, I guess due to the location.hash change?

Comment 8

[Botond Ballo [:botond]]

I debugged this a bit, here's what I found:

• The scroll frame's layout scroll offset gets set to the anchor position, presumably in response to the location.hash change. This also sets the visual scroll offset to the same value, likely due to this code.
• Then something calls ScrollFrameHelper::RestoreState(), which sets mRestorePos to that position.
• Then ScrollFrameHelper::ReloadChildFrames() is called, which among other things gives mScrolledFrame a new value. This has the side effect of resetting the layout scroll position to (0,0), since it's kind of stored implicitly in the position of mScrolledFrame.
• The scroll frame is now in a weird state where its visual scroll offset is the anchor position, while the layout scroll offset is (0,0), without any message having been sent to APZ to eventually reconcile them.
• On the next reflow, ScrollToRestoredPosition() queries the visual position (this is the part that's new in bug 1498812), finds that it's equal to mRestorePos, so there's nothing more to do. Meanwhile, the layout position is stuck at (0,0) (and that's what the test queries shortly afterward).

Comment 9

[Botond Ballo [:botond]]

A couple of possible fix approaches based on the above:

• Have ScrollFrameHelper::ReloadChildFrames() clear the visual scroll position, too, for good measure. (Presumably, the thing that triggers it is also what triggers the RestoreState() call, so it knows the position will be restored. We'd want to double-check this though.)
• Have ScrollToRestoredPosition() keep scrolling until the visual and layout offsets reach the target position (with the code being tweaked in bug 1516056 to use distinct target positions for visual and layout).

Comment 10

[Jan Henning [:JanH]]

Thanks very much for figuring this out. I have to admit I'm somewhat out of my depth here and it would have taken me a longer time to figure this out as well.

with the code being tweaked in bug 1516056 to use distinct target positions for visual and layout

Note that in the ideal end state the PresState should probably contain both layout and visual viewport scroll positions (bug 1499210), such as to permit completely recreating the previous scroll state when restoring the state. You'd probably have a better overview over this than I, but might that be an argument towards the latter solution?

Comment 11

[Botond Ballo [:botond]]

(In reply to Jan Henning [:JanH] from comment #10)

with the code being tweaked in bug 1516056 to use distinct target positions for visual and layout

Note that in the ideal end state the PresState should probably contain both layout and visual viewport scroll positions (bug 1499210), such as to permit completely recreating the previous scroll state when restoring the state. You'd probably have a better overview over this than I, but might that be an argument towards the latter solution?

Yeah, I think you're right. We can do it in three stages:

1. This bug: change the stopping condition in ScrollToRestoredPosition() to be "visual offset == mRestorePos AND layout offset == mRestorePos".
2. As part of bug 1516056: Split mRestorePos into mLayoutRestorePos and mVisualRestorePos. Initialize mVisualRestorePos from the pres state as today. Initialize mLayoutRestorePos by clamping mVisualRestorePos to the layout scroll range. Adjust the stopping condition to "visual offset == mVisualRestorePos AND layout offset == mLayoutRestorePos".
3. Bug 1499210: Store the layout offset in the pres state, and initialize mLayoutRestorePos based on the stored value.

Comment 12

[Botond Ballo [:botond]]

[Tracking Requested - why for this release]:

We should probably track this for 66 as the issue described in comment 8 has the potential for causing user-visible regressions when e.g. scrolling to an anchor.

Comment 13

[Intermittent Failures Robot]

17 failures in 476 pushes (0.036 failures/push) were associated with this bug yesterday.

Repository breakdown:

• try: 9
• mozilla-central: 8

Platform breakdown:

• android-hw-p2-8-0-arm7-api-16: 2
• android-hw-g5-7-0-arm7-api-16: 2
• android-em-7-0-x86: 13

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1519621&startday=2019-01-17&endday=2019-01-17&tree=all

Comment 14

[Intermittent Failures Robot]

49 failures in 2378 pushes (0.021 failures/push) were associated with this bug in the last 7 days.

This is the #15 most frequent failure this week.

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **

Repository breakdown:

• try: 11
• mozilla-central: 38

Platform breakdown:

• android-hw-p2-8-0-arm7-api-16: 2
• android-hw-g5-7-0-arm7-api-16: 2
• android-em-7-0-x86: 45

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1519621&startday=2019-01-14&endday=2019-01-20&tree=all

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, I can track this, and it might make sense for me to have a look at the other bugs you mention in comment 11.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

(In reply to Botond Ballo [:botond] from comment #11)

Yeah, I think you're right. We can do it in three stages:

1. This bug: change the stopping condition in ScrollToRestoredPosition()
   to be "visual offset == mRestorePos AND layout offset == mRestorePos".

Noting, from discussion with Botond, that this is what we need to fix this test failure for 66.

Comment 17

[Intermittent Failures Robot]

25 failures in 2383 pushes (0.01 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 12
• mozilla-central: 13

Platform breakdown:

• android-em-7-0-x86: 23
• android-em-4-3-armv7-api16: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1519621&startday=2019-01-21&endday=2019-01-27&tree=all

Comment 18

[Sean Voisen (:svoisen)]

Hi Botond – Can I assign this to you to stay on top of? Do you plan to address changing the stopping condition to fix this test failure in 66?

Comment 19

[Botond Ballo [:botond]]

Sure, I'll take this one.

Comment 20

[Botond Ballo [:botond]]

Attachment: Bug 1519621 - Make sure ScrollToRestoredPosition() restores both the layout and visual scroll positions. r=tnikkel

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Did this ever land? I think it may not have landed on 67 because 67 wasn't marked affected.

Comment 22

[Botond Ballo [:botond]]

It hasn't landed because it's not reviewed. I'm waiting on Timothy for review.

Comment 23

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] [standards meeting Feb 18-23] from comment #8)

• Then ScrollFrameHelper::ReloadChildFrames() is called, which among other
  things gives mScrolledFrame a new value. This has the side effect of
  resetting the layout scroll position to (0,0), since it's kind of [stored
  implicitly](https://searchfox.org/mozilla-central/rev/
  c21d6620d384dfb13ede6054015da05a6353b899/layout/generic/nsGfxScrollFrame.
  h#200) in the position of mScrolledFrame.
• The scroll frame is now in a weird state where its visual scroll offset is
  the anchor position, while the layout scroll offset is (0,0), without any
  message having been sent to APZ to eventually reconcile them.

If we can get into this state where there is no restore to be done (because there is no history for this scroll frame), or just in a situation where ScrollToRestorePosition is not called then the proposed patch isn't a full solution. From the description it seems like that is possible.

(In reply to Botond Ballo [:botond] [standards meeting Feb 18-23] from comment #9)

• Have ScrollFrameHelper::ReloadChildFrames() clear the visual scroll
  position, too, for good measure. (Presumably, the thing that triggers it is
  also what triggers the RestoreState() call, so it knows the position will
  be restored. We'd want to double-check this though.)

Yeah, so we should probably understand this better to make sure we are fixing it fully.

The attached patch seems fine as a partial fix (and potentially full fix depending on the investigation), if we need to fix this asap I can give review.

Comment 24

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #23)

The attached patch seems fine as a partial fix (and potentially full fix depending on the investigation), if we need to fix this asap I can give review.

I would prefer to get this patch landed sooner rather than later, for two reasons:

• This bug is tracked for 66.

• The patch is blocking the dependency chain bug 1517895 --> bug 1516056 --> bug 1519013. Bug 1519013 means the fix for bug 1423013 is not being applied consistently. Bug 1423013 is the most user-visible of the viewport compat bugs we've been prioritizing over the last few months.

I would be happy to investigate the ReloadChildFrames() issue further as a follow-up.

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)

I would be happy to investigate the ReloadChildFrames() issue further as a follow-up.

Filed bug 1531106 for this.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c0ca241bd4b
Make sure ScrollToRestoredPosition() restores both the layout and visual scroll positions. r=tnikkel

Comment 27

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b973064ca57
Backed out changeset 2c0ca241bd4b for frame-reconstruction-scroll-clamping.html high frequency failures CLOSED TREE

Comment 28

[Robin Steuber (she/her) [:bytesized]]

https://hg.mozilla.org/projects/oak/rev/2c0ca241bd4b662f7022beb535509164ab7cde51
Bug 1519621 - Make sure ScrollToRestoredPosition() restores both the layout and visual scroll positions. r=tnikkel

https://hg.mozilla.org/projects/oak/rev/1b973064ca5778532190fef25d81b75e8cf77281
Backed out changeset 2c0ca241bd4b (bug 1519621) for frame-reconstruction-scroll-clamping.html high frequency failures CLOSED TREE

Comment 29

[Botond Ballo [:botond]]

(In reply to Pulsebot from comment #27)

Backed out changeset 2c0ca241bd4b for
frame-reconstruction-scroll-clamping.html high frequency failures CLOSED TREE

The failures in questions are unexpected passes, i.e. this patch is improving the test's behaviour. Bug 1517895, which I hope to land shortly after this, will make the test pass consistently. Until then, I'm just going to mark it random-if.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5c0cad448f9
Make sure ScrollToRestoredPosition() restores both the layout and visual scroll positions. r=tnikkel

Comment 31

[Razvan Maries]

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

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift? Thanks!

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 9040766 [details]
Bug 1519621 - Make sure ScrollToRestoredPosition() restores both the layout and visual scroll positions. r=tnikkel

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1498812
• User impact if declined: Navigation to an anchor (e.g. page.html#anchor) can sometimes fail to result in scrolling to the anchor position.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Based on comment 23, there is an aspect of the problem here that we may not fully understand yet (to be investigated in bug 1531106), so I would categorize this as medium risk.
• String changes made/needed:

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Hmm. Do you think it's worth uplifting then? Would this only affect scrolling position when a user restores?

Comment 35

[Botond Ballo [:botond]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #34)

Hmm. Do you think it's worth uplifting then? Would this only affect scrolling position when a user restores?

In the testcase, the issue is triggered simply by JS code performing location.hash = "bar", before any history navigation or anything else. (The "restore scroll position" codepath is hit because that code seems to trigger a frame reconstruction.)

I can't rule out the possibility that this could occur in a production scenario, but straightfoward attempts to trigger the issue with similar JS code fail, and we haven't had user reports of running into this bug either.

Earlier in the beta cycle, I would have argued for uplift, but as the resolution of this bug has dragged on late into the cycle, I would be fine with being conservative and not uplifting.

Timothy, do you have an opinion here?

Comment 36

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #35)

Earlier in the beta cycle, I would have argued for uplift, but as the
resolution of this bug has dragged on late into the cycle, I would be fine
with being conservative and not uplifting.

I'm fine with this too.

Comment 37

[Botond Ballo [:botond]]

Comment on attachment 9040766 [details]
Bug 1519621 - Make sure ScrollToRestoredPosition() restores both the layout and visual scroll positions. r=tnikkel

Canceling beta uplift request based on previous comments.