942736 - Page-position when going back a page is not always remembered when conducting a scroll on prior page and then hitting back

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P5)

Description

[Julian Parker]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

(Note: This is not like bug #154969, I mean something else.)
When you follow a link to another page and go back to the previous page, you normally go to the position where you was before.


Actual results:

When you scroll the page and go back while it is still scrolling and not "standing still" before you go back, the previous page is on top and not on the position where the followed link was.



Expected results:

Going back should always go to the correct position of the previous page and not only if the page that you go back from isn't moving any more.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Is there a specific page where this happens?

Comment 2

[Julian Parker]

On many pages, e.g. the German newssite www.spiegel.de

Comment 3

[Aaron Train [:aaronmt]]

Doesn't this depend on a if a site is using cache-control: no-store or a max-age in their http header?

Comment 4

[Julian Parker]

@kbrosnan:
Sorry, I meant the mobile site: m.spiegel.de
I don't know if this affects the classic site too.

@aaronmt:
But when the page isn't scrolling, it works (e.g. on the mentioned m.spiegel.de). Only if I give the page a swipe to scroll and then go back before the page stopped scrolling and stands totally still, this happens.

Comment 5

[Aaron Train [:aaronmt]]

Ah I see, that's weird. I was able to reproduce on my second attempt after giving the page a large scroll and then hitting back; the browser brought me back to the top of m.spiegel.de. I was able to reproduce on Nightly (28.0) so I imagine that this has been a long-standing bug maybe. Kats might have an idea here on the issue.

Comment 6

[Julian Parker]

And of course that not only affects m.spiegel.de but also many other sites. And sometimes it happens even when not scrolling, but in these cases I don't see any regularity that make this happen. While scrolling it always happens.

Comment 7

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

I'm not able to reproduce this. What I'm doing is:

1. Load m.spiegel.de
2. Scroll down to some article halfway down the page
3. Click on the link to the article
4. Once the article is loaded, do a large fling
5. While the page is still moving hit the back button

If I understand correctly the bug is that this renders m.spiegel.de back at the top. I'm not seeing this; I always see it go back to the scroll position I was at when I clicked on the link. Testing on a week-old nightly build on a Nexus 4.

Comment 8

[Pascal Fischer]

I can reproduce that on my Sony Xperia Ray. It happens not only on m.spiegel.de but also on m.heise.de (a German IT-newsletter) and many more sites. And sometimes Firefox even crashes when doing that.

Comment 9

[Pascal Fischer]

(In reply to Pascal Fischer from comment #8)
> I can reproduce that on my Sony Xperia Ray. It happens not only on
> m.spiegel.de but also on m.heise.de (a German IT-newsletter) and many more
> sites. And sometimes Firefox even crashes when doing that.

PS:

But on m.heise.de something very strange happens:
the described problem never happens, when you do this with the first half of the page and not on the last half of the page. And it not renders from the top, but from the middle of the page.

To make it clearer:
on the upper parts of m.heise.de all happens as it should. But then there is a point like a "magic border". When you get below that point and click articles and then go back, the page goes up to that point in the middle of the page. And not only when scrolling on the second page.

Comment 10

[Carlos F]

I always have this problem too on most pages. That is the most annoying bug in mobile Firefox. Only because of this bug i switched to another browser for mobile browsing although I love Firefox on the desktop.

Comment 11

[Aaron Train [:aaronmt]]

(In reply to Carlos F from comment #10)
> I always have this problem too on most pages. That is the most annoying bug
> in mobile Firefox. Only because of this bug i switched to another browser
> for mobile browsing although I love Firefox on the desktop.

Sorry to hear that. Did you have steps to reproduce? The reason this is open is that we don't have concrete steps to reproduce and it's apparently not reproducible on different devices. Which device were you using Firefox on? Do you recall which pages?

Comment 12

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

Unassigning since I'm not working on this, and was never able to reproduce it.

Comment 13

[[:philipp]]

i can reproduce it on a htc one s and current versions of firefox on android (but it was the same since as long as i can remember) like this:

- go to http://m.diepresse.com/home/index.do
- scroll down and tap on any article
- hit the back button, which will land you on the top of the prior page and not at the scroll position where you've left

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 15

[Eugen Sawin [:esawin]]

Attachment: fling-back-scroll-position.patch

This patch consistently fixes the described issues for all the mentioned sites.

Obtaining the scroll range via PresShell::GetScrollPositionClampingScrollPortSize within ScrollFrameHelper::ScrollToImpl seems to invalidate the previously stored scroll position (possibly out of range).
I don't see how this could be caused by a race condition though, which the STR more or less imply. Any ideas?

Comment 16

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

Comment on attachment 8509500 [details] [diff] [review]
fling-back-scroll-position.patch

Review of attachment 8509500 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not really comfortable with this code so I don't think I can meaningfully provide f+ or f- either way. However, some thoughts:

- Based on my admittedly poor understanding of this code, it looks like it will prevent scrolling into the bottom-right corner of pages when zoomed in. i.e. by ignoring the scroll-clamping-position-scroll-port-size here and using the regular scrollport, the scroll position will get clamped incorrectly, both for web content calling scrollTo and when we call scrollTo from browser.js

- The raciness described in the STR might be related to when exactly calls to setScrollPositionClampingScrollPortSize are made. I haven't looked in detail but it may be that after doing a fling and then hitting back, we end up calling setScrollPositionClampingScrollPortSize on the wrong document. The fling will trigger a series of viewport updates, so we'll have a stream of setSPCSPS calls going, and if the document changes while those are inflight it's possible we end up setting it on the other document. Then the restored scroll position might get clamped to the wrong thing.

Does that help at all?

Comment 17

[Eugen Sawin [:esawin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> I'm not really comfortable with this code so I don't think I can
> meaningfully provide f+ or f- either way. However, some thoughts:
> 
> - Based on my admittedly poor understanding of this code, it looks like it
> will prevent scrolling into the bottom-right corner of pages when zoomed in.
> i.e. by ignoring the scroll-clamping-position-scroll-port-size here and
> using the regular scrollport, the scroll position will get clamped
> incorrectly, both for web content calling scrollTo and when we call scrollTo
> from browser.js
You're right, there are a lot of ways to cure the symptoms here at different levels, this would be the lowest level and would break things.

> - The raciness described in the STR might be related to when exactly calls
> to setScrollPositionClampingScrollPortSize are made. I haven't looked in
> detail but it may be that after doing a fling and then hitting back, we end
> up calling setScrollPositionClampingScrollPortSize on the wrong document.
> The fling will trigger a series of viewport updates, so we'll have a stream
> of setSPCSPS calls going, and if the document changes while those are
> inflight it's possible we end up setting it on the other document. Then the
> restored scroll position might get clamped to the wrong thing.
Yes, that's what I've been chasing after, however it is not clear to me where the faulty viewport values come from, as they're only occurring in-between switching pages and are neither correct for the previous nor the next page loaded. I'm looking into the cause for this now.

> Does that help at all?
You're always helpful, thanks!

Comment 18

[Eugen Sawin [:esawin]]

Attachment: fling-back-scroll-position.patch

The issue is being caused by the margins animation during flinging. When we abort the animation to load the new page (triggered by the back button action) the margins are still zeroed, which results in invalid viewport metrics returned by JavaPanZoomController::getValidViewportMetrics (wrong scaleFactor for minZoomFactor due to missing marginTop and marginBottom values results in wrong zoom).

A way to fix this would be to abort the margins animation when necessary and immediately set the correct margins.

Comment 19

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

Comment on attachment 8512677 [details] [diff] [review]
fling-back-scroll-position.patch

Review of attachment 8512677 [details] [diff] [review]:
-----------------------------------------------------------------

This seems ok to me but it also looks like it will cause the toolbar to appear in some cases when it doesn't right now. For example I think if you're just static (not scrolling) on a page and the web content calls scrollTo then that will trigger a call to abortAnimations and that will now make the toolbar appear. Can you check to see if this is the case, and if so, if that makes sense or not?

If that is the case, it should be possible to avoid it by moving the showMargins call higher up in the call stack. For example you could add a bool argument to GeckoLayerClient.abortPanZoomAnimation that controls whether or not we show the margins, and do the call based on that argument at [1]. The call to abortPanZoomAnimation in setFirstPaintViewport could then pass true and the others could pass false, and that should handle the page-load case but leave the others as-is.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/GeckoLayerClient.java?rev=1f9b1f477588#376

Comment 20

[Eugen Sawin [:esawin]]

Attachment: fling-back-scroll-position.patch

The additional step in calculating the actual margins might be redundant, but that way we make sure to account for situations where we don't have the toolbar margins set (i.e., no direct dependence on LayerMarginsAnimator always having the max margins set).

Comment 21

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

Comment on attachment 8512951 [details] [diff] [review]
fling-back-scroll-position.patch

Review of attachment 8512951 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great to me, thanks!

::: mobile/android/base/gfx/LayerMarginsAnimator.java
@@ +92,5 @@
>                  "{ \"top\" : " + top + ", \"right\" : " + right
>                  + ", \"bottom\" : " + bottom + ", \"left\" : " + left + " }"));
>      }
>  
> +    public synchronized RectF getMaxMargins() {

drop the public, no need for it.

Comment 22

[Eugen Sawin [:esawin]]

Attachment: fling-back-scroll-position.patch

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=59b8144e9a21

https://hg.mozilla.org/integration/mozilla-inbound/rev/8081294c87ee

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8081294c87ee

Comment 24

[[:philipp]]

i still see this issue in the current nightly with the site mentioned in comment #13.

Comment 25

[Eugen Sawin [:esawin]]

(In reply to philipp from comment #24)
> i still see this issue in the current nightly with the site mentioned in
> comment #13.

That comment describes a different issue, which is not related to flinging.
It would be great if you could create a new bug with the specific STR for that!
DiePresse does not serve me the m.* version of their site and it's not producing on diepress.com.

Comment 26

[Kevin Brosnan [Ex-Mozilla]]

Do we want to uplift to Aurora? The patch seems well constrained and this has baked for two weeks and improves a longstanding issue.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #26)
> Do we want to uplift to Aurora? The patch seems well constrained and this
> has baked for two weeks and improves a longstanding issue.

Sure, why not. Eugen, please request approval, etc.

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: see comment 26

Comment 29

[Eugen Sawin [:esawin]]

Comment on attachment 8513446 [details] [diff] [review]
fling-back-scroll-position.patch

Approval Request Comment
[Feature/regressing bug #]: proper scroll position restoring
[User impact if declined]: scroll position is not restored when going back in history while flinging
[Describe test coverage new/current, TBPL]: local testing only
[Risks and why]: low, patch has no effect on the regular (non-flinging) case, has been on Nightly for two weeks 
[String/UUID change made/needed]: none

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8513446 [details] [diff] [review]
fling-back-scroll-position.patch

It's been around for a while but given the low risk, we can take this uplift.

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

No need to track this, it's been around way too long.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/079e6ec1b93c