1186774 - Scroll position (scrollX/scrollY) should be restored after popstate, not before (was: "When gone back a page the position is reset")

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Ray Satiro]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150720030213

Steps to reproduce:

https://github.com/tatsuhiro-t/nghttp2
Click README (not README.rst)
Click the back button


Actual results:

The page's position is reset, it does not reflect the position it was in before I clicked README


Expected results:

The page position should be the same. FF31 it is, Aurora and Nightly it isn't.

Comment 1

[Alice0775 White]

Firefox39(incl. Nightly42.01) and IE11 are broken.

However, Chrome 45 and Microsoft Edge are working as expected.

Comment 2

[Boris Zbarsky [:bzbarsky]]

If this worked in 31, can you find the regression range?

Comment 3

[Alice0775 White]

(In reply to Boris Zbarsky [:bz] from comment #2)
> If this worked in 31, can you find the regression range?

In my pc, it does not work on 31. So, I think this is not a regression

Comment 4

[Ray Satiro]

(In reply to Alice0775 White from comment #3)
> (In reply to Boris Zbarsky [:bz] from comment #2)
> > If this worked in 31, can you find the regression range?
> 
> In my pc, it does not work on 31. So, I think this is not a regression

I checked our desktops with 31 ESR and narrowed the correct behavior down to what it seems is a side effect of an old extension CanvasBlocker 0.1.4. So the steps to get the correct behavior are:

Install CanvasBlocker 0.1.4, no restart required.
https://addons.mozilla.org/en-US/firefox/addon/canvasblocker/versions/?page=1#version-0.1.4.1-signed

Go to https://github.com/tatsuhiro-t/nghttp2
There will be a message box "Do you want to allow invisible <canvas> readout?", Choose Cancel
Click README (not README.rst). There will be that same message box, Choose Cancel
Click the back button

I didn't notice that initially because our desktops are set to auto-deny the canvas in ESR 31 in a configuration that is system-wide and not per-profile.

I tried installing the extension in Nightly and I have the correct behavior there as well when I deny the canvas. Perhaps this issue has something to do with the canvas? Regrettably I do not have any more time to look into this.

Comment 5

[Alice0775 White]

According to Comment 4, NEW -> UNCONFIRMED

Comment 6

[Ray Satiro]

To make sure there isn't a miscommunication here what I am saying is the correct behavior (ie what firefox should do) is only present when using an extension that blocks the canvas. The incorrect behavior appears in 31, Aurora, Nightly.

Comment 7

[Alice0775 White]

Confuse me.

So, in other word, the problem is reproducible in 31, Aurora, Nightly without any addon.

And, the problem is reproducible since the build[1] at least.

[1] http://hg.mozilla.org/mozilla-central/rev/cd2365b0f710
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110504 Firefox/6.0a1 ID:20110504093241

Before the build[1], it is not able to test due to Bug 613634.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  So if I disable script, the scroll position is preserved correctly...

I tried setting a breakpoint in ScrollFrameHelper::ScrollToWithOrigin and when I go back I see several calls to this function, in this order:

1)  Scroll to y=35130 coming from nsDocShell::InternalLoad calling nsDocShell::SetCurScrollPosEx.  This is the history scroll restoration happening.

2)  Scroll to y=0 from APZCCallbackHelper::UpdateRootFrame

3)  Another scroll to y=0 from APZCCallbackHelper::UpdateRootFrame

The stack for calls 2 and 3 looks like so:

#0  mozilla::ScrollFrameHelper::ScrollToWithOrigin (this=0x15028fe90, aScrollPosition={<mozilla::gfx::BasePoint<int, nsPoint, int>> = {x = 0, y = 0}, <No data fields>}, aMode=nsIScrollableFrame::INSTANT, aOrigin=0x11a735be0, aRange=0x7fff5fbfbf58, aSnap=nsIScrollbarMediator::DISABLE_SNAP) at nsGfxScrollFrame.cpp:2007
#1  0x0000000105fd77cf in mozilla::ScrollFrameHelper::ScrollToCSSPixelsApproximate (this=0x15028fe90, aScrollPosition=@0x7fff5fbfc0e8, aOrigin=0x11a735be0) at nsGfxScrollFrame.cpp:1985
#2  0x000000010600a1b7 in nsHTMLScrollFrame::ScrollToCSSPixelsApproximate (this=0x15028fe08, aScrollPosition=@0x7fff5fbfc0e8, aOrigin=0x11a735be0) at nsGfxScrollFrame.h:752
#3  0x000000010600b342 in non-virtual thunk to nsHTMLScrollFrame::ScrollToCSSPixelsApproximate(mozilla::gfx::PointTyped<mozilla::CSSPixel> const&, nsIAtom*) (this=0x15028fe78, aScrollPosition=@0x7fff5fbfc0e8, aOrigin=0x11a735be0) at nsGfxScrollFrame.h:753
#4  0x000000010344e092 in mozilla::layers::ScrollFrameTo (aFrame=0x15028fe78, aPoint=@0x7fff5fbfc160, aSuccessOut=@0x7fff5fbfc16f) at APZCCallbackHelper.cpp:96
#5  0x000000010344abe9 in mozilla::layers::ScrollFrame (aContent=0x13f3e8580, aMetrics=@0x7fff5fbfc218) at APZCCallbackHelper.cpp:123
#6  0x000000010344aabf in mozilla::layers::APZCCallbackHelper::UpdateRootFrame (aMetrics=@0x7fff5fbfc218) at APZCCallbackHelper.cpp:235
#7  0x0000000103478aaa in mozilla::layers::ChromeProcessController::RequestContentRepaint (this=0x1437a6550, aFrameMetrics=@0x7fff5fbfc3c8) at ChromeProcessController.cpp:51
#8  0x0000000103462bf3 in _ZN25nsRunnableMethodArgumentsIJN7mozilla6layers12FrameMetricsEEE5applyINS1_22GeckoContentControllerEMS5_FvRKS2_EEEvPT_T0_ (this=0x150805ca0, o=0x1437a6550, m=not implemented: member type in c_val_print
) at nsThreadUtils.h:634

It look like in frame 5 the aMetrics has an incorrect scroll offset hanging off it.

Oh, for extra fun if I actually _stop_ at the breakpoint in ScrollFrameHelper::ScrollToWithOrigin the first time it's called as opposed to just having it print the stack, the bug disappears.  So this suggests some weird race between the thread the async pan/zoom stuff happens on and layout...

Comment 9

[Boris Zbarsky [:bzbarsky]]

kats, do you have time to take a look?

Comment 10

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

Yup, I can take this. I can repro on both OS X and Linux so I can probably use rr to track down the race if there is one.

Comment 11

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

I misread the bug and thought this was an APZ regression, but now I see it's not. I can reproduce this both with and without APZ. In both cases I see the call from nsDocShell::SetCurScrollPosEx (as you mentioned in comment 8) trying to restore the scroll position, but I also see that the scroll position is getting clamped to 0 in that call at [1], because at the point that the scroll position is restored, the scroll range is 0.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=34ade6cc41e1#2014

Comment 12

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

So actually I think this page is using pushState/popState and possibly manipulating the scroll offset internally, and the bug (if there is one) is not really in the APZ or any scrolling-specific code. It might be in docshell doing things differently from what the page expects but that's way outside of my area of expertise so I'm going to pass this back.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, interesting.

If I disable script I see us going through things like PresShell::RestoreRootScrollPosition from PresShell::Initialize, which are not hit with script enabled.  So I think you're correct about pushState being involved.

OK, so we land in mozilla::ScrollFrameHelper::GetScrollRange.  GetScrolledRect() has a height of 48000.  aHeight (which is scrollPortSize.height) is also 48000.  So we end up with a vertical scroll range of 0.

So I think the issue is that the SetScrollPosEx call in docshell happens as part of the "same-document traversal" changes, immediately after the ScrollToAnchor bits.  That sort of makes sense for anchor scroll traversals, but for traversal across a pushstate, we'd want to do the scroll after firing popstate and letting the page rebuild itself or something.  Olli, does that make sense?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Why would we scroll _after_ popstate? I'd expect page to update its state on popstate and scroll to the right position, if needed.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Also, I can't reproduce this on linux.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Because we're scrolling to the pre-pushstate scroll position.  The page in this case is updating its state on popstate but not scrolling anywhere.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

oh, wait, I can. my mistake. didn't click the right Readme

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I see. Let me write a patch and test.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

This is regression prone stuff, and tryserver may give plenty of failures
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a391a75aa6c9

Need to write a test, and I should test other browsers too, but getting late here now.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640214 [details] [diff] [review]
wip

Yeah, like that.  r=me

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

This is against the spec though, and some blink folks want to change their behavior...
investigating.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

See also https://code.google.com/p/chromium/issues/detail?id=474579

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

I'll land this with a test.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

And file a spec bug

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +test

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

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Apparently hg was behaving badly when I pushed to try.
Another push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6642214a418

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3bc092e919b

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

https://github.com/whatwg/html/issues/39

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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