1279050 - Scroll position and/or history is not retained after loading a redirected page

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Fanolian]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160608030219

Steps to reproduce:

1. Load https://www.mozilla.org/contact/ . It should redirect to one of the locales, e.g. https://www.mozilla.org/en-US/contact/
2. Do one of the following:
2a. Scroll down. Reload the page; or
2b. Scroll down. Left-click "I have questions about using Mozilla’s trademarks" so it opens in the same tab. Press Back; or
2c. Scroll down, then paste https://www.mozilla.org/en-US/foundation/trademarks/policy/ into URL bar and navigate to it. Press Back.


Actual results:

Scroll positions are not remembered for all cases. As for 2b and 2c, the history of last page is lost so I cannot press Forward.

Comment 1

[Fanolian]

From mozregression:

Last good Nightly: 2016-06-06
First bad Nightly: 2016-06-07
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=396b577b98e64fe95223cfce095f84fe0a67da01&tochange=7f7c7d24700eb80ce328b05fd260ec58e9725ca4

Further bisection:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0ac2f7a4fc731929c4872c147ee98dd03e9acd0a&tochange=eb3278ed4eb3e3924c556ef92ef4763008a537f3

Suspect: Bug 968273

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 3

[Dragana Damjanovic [:dragana]]

The code that needs to be change is here:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7466

probably checking if redirect is to the uri we are reloading...

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_1279050_v1.patch

Comment 5

[Dragana Damjanovic [:dragana]]

bz, you are the right person to review this. Sorry for needinfo you, when you have time can you take a look.

I am not 100% sure is the the right place. It fixes the problem, but I am not sure of side paths.
Thanks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'm not sure of the various side-effects either.  This is the first time I've seen this code in OnRedirectStateChange, and I'm not entirely sure what it's trying to do.

Olli, do you know what this stuff is about?

Dragana, what is the general goal of the patch? 

Are there no places between the redirect notification and STATE_STOP that examine mLoadType?

Comment 7

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

I'll be away this evening. I'll try to remember to look at this tomorrow.
I vaguely recall looking at some of the redirect stuff while fixing session history couple of years ago.

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> I'm not sure of the various side-effects either.  This is the first time
> I've seen this code in OnRedirectStateChange, and I'm not entirely sure what
> it's trying to do.
> 
> Olli, do you know what this stuff is about?
> 
> Dragana, what is the general goal of the patch? 
> 

On redirect if a load was a reload or history load, we use to change loadType to LOAD_NORMAL_REPLACE.
In bug 968273, I have changed to do the reload from originalURI. This will do a redirect again which triggers the changing loadType to LOAD_NORMAL_REPLACE but it should not.

I could do the check in nsDocShell::OnRedirectStateChange but there could be multiples redirects (except we save all of them)



> Are there no places between the redirect notification and STATE_STOP that
> examine mLoadType?

I figure out this is not a right place :(
I need notification when all redirects are done, probably when we get the first non-redirect response.

Any suggestions?
Thanks

Comment 9

[Dragana Damjanovic [:dragana]]

Option 1 is to remember all the redirects and if the chain mismatch consider reload to be a new load.

Option 2 I have tried with OnStateChange/OnStatusChange but they are sent to late.

Option 3: The first nsDocShell function that is called when new content arrives (when redirects are finished) is CreateContentViewer. It is possible to do checks there but this is not covering all cases, e.g. when an error page is loaded.

So I do not like option 1, option 2 does not work. So option 3 - to find a common place for all code paths.

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: bug_1279050_v1.patch

I added a function that is called when OnStartRequest is called. If we get OnStartRequest to the new channel redirect is completed and we can compare URIs.

Comment 11

[Dragana Damjanovic [:dragana]]

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

Comment 12

[Marcia Knous [:marcia]]

Tracking 50+ for this regression in scroll history.

Comment 14

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

(sorry, reviewing this may take some time since reviewing this means that one needs to review also bug 968273)

Comment 15

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

and note to myself, need to look at bug 536466 too.

Comment 16

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

Dragana, could you perhaps answer to https://bugzilla.mozilla.org/show_bug.cgi?id=968273#c43 and explain how our behavior changed there and what other browsers do.
(location.reload takes an optional bool param, which affects the behavior too)

Comment 17

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

Comment on attachment 8764355 [details] [diff] [review]
bug_1279050_v1.patch

I was testing the behavior a bit, and it is unclear to me why bug 968273 is a safe change.
Please explain, and ask review again for this patch, or
back out the patch for bug 968273 and then we'll need possibly different fix for that bug (something which doesn't affect to history API behavior).

Comment 18

[Dragana Damjanovic [:dragana]]

bug 968273 backed out.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Based on the backout assuming the problem is fixed in 50, updating status flag to reflect that.