Closed Bug 1186774 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: raysatiro, Assigned: smaug)

References

Details

Attachments

(2 files)

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.
Component: Untriaged → Document Navigation
Product: Firefox → Core
Version: 42 Branch → Trunk
Firefox39(incl. Nightly42.01) and IE11 are broken.

However, Chrome 45 and Microsoft Edge are working as expected.
Status: UNCONFIRMED → NEW
Ever confirmed: true
If this worked in 31, can you find the regression range?
Flags: needinfo?(alice0775)
(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
Flags: needinfo?(alice0775)
(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.
According to Comment 4, NEW -> UNCONFIRMED
Status: NEW → UNCONFIRMED
Component: Document Navigation → Untriaged
Ever confirmed: false
Product: Core → Firefox
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → Document Navigation
Product: Firefox → Core
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...
Component: Document Navigation → Panning and Zooming
kats, do you have time to take a look?
Flags: needinfo?(bugmail.mozilla)
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.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
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
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.
Assignee: bugmail.mozilla → nobody
Component: Panning and Zooming → Document Navigation
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?
Flags: needinfo?(bugs)
Why would we scroll _after_ popstate? I'd expect page to update its state on popstate and scroll to the right position, if needed.
Flags: needinfo?(bugs)
Also, I can't reproduce this on linux.
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.
oh, wait, I can. my mistake. didn't click the right Readme
I see. Let me write a patch and test.
Attached patch wipSplinter Review
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.
Attachment #8640214 - Flags: review?(bzbarsky)
Comment on attachment 8640214 [details] [diff] [review]
wip

Yeah, like that.  r=me
Attachment #8640214 - Flags: review?(bzbarsky) → review+
This is against the spec though, and some blink folks want to change their behavior...
investigating.
Duplicate of this bug: 679458
Summary: When gone back a page the position is reset → Scroll position (scrollX/scrollY) should be restored after popstate, not before (was: "When gone back a page the position is reset")
I'll land this with a test.
Assignee: nobody → bugs
And file a spec bug
Apparently hg was behaving badly when I pushed to try.
Another push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6642214a418
https://hg.mozilla.org/mozilla-central/rev/b3bc092e919b
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1229928
You need to log in before you can comment on or make changes to this bug.