Closed Bug 1519621 Opened 9 months ago Closed 8 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox65 --- unaffected
firefox66 + wontfix
firefox67 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure, Whiteboard: [retriggered])

Attachments

(1 file)

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

Flags: needinfo?(jh+bugzilla)
Summary: Intermittent /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 → 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
Whiteboard: [retriggered]

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

Flags: needinfo?(jh+bugzilla)

Whoops, didn't mean to clear the flag.

Flags: needinfo?(jh+bugzilla)

(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.

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.

(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?

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).

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).

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?

Flags: needinfo?(jh+bugzilla)

(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.

[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.

Blocks: 1498812

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.

Priority: P5 → P1

(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.

Priority: P1 → P2

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?

Flags: needinfo?(botond)

Sure, I'll take this one.

Assignee: nobody → botond
Flags: needinfo?(botond)
Blocks: 1516056

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

Flags: needinfo?(botond)

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

Flags: needinfo?(botond) → needinfo?(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.

Flags: needinfo?(tnikkel)

(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:

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

Blocks: 1531106

(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.

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c0ca241bd4b
Make sure ScrollToRestoredPosition() restores both the layout and visual scroll positions. r=tnikkel
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b973064ca57
Backed out changeset 2c0ca241bd4b for frame-reconstruction-scroll-clamping.html high frequency failures CLOSED TREE
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

(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.

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5c0cad448f9
Make sure ScrollToRestoredPosition() restores both the layout and visual scroll positions. r=tnikkel
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Can you request uplift? Thanks!

Flags: needinfo?(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:
Flags: needinfo?(botond)
Attachment #9040766 - Flags: approval-mozilla-beta?

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

Flags: needinfo?(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?

Flags: needinfo?(botond) → needinfo?(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.

Flags: needinfo?(tnikkel)

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.

Attachment #9040766 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.