1269539 - Scroll position lost when reload page

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]:

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/77cead2cd20300623eea2416bc9bce4d5021df09
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 ID:20160502030207

Original report, see http://forums.mozillazine.org/viewtopic.php?p=14591285#p14591285

I can reproduce the problem with/without e10s.


Reproducible : always

Steps To Reproduce:
1. Open http://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win32/
2. Scroll to the bottom
3. Reload page(F5)

Actual Results:
Scroll position is lost. Scroll position is restored at random.

Expected Results:
Scroll position should be restored.


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=774c838c19e217e177ba22876d57a344b54c33e4&tochange=a12cdfe43e929dfa151e3ea1c35982cef674ff66

Regressed by: a12cdfe43e92	Kartikaya Gupta — Bug 1268195 - When restoring a scroll position outside of incremental load, don't keep trying in a loop - just do it once and stop. r=tnikkel

Comment 1

[Timothy Nikkel (:tnikkel)]

We create the (new) root scroll frame before LoadComplete is called, and we restore the previous state (mRestoringHistoryScrollPosition is false). We try to restore once and then clear the restore pos. Then EndLoad is called and sets mRestoringHistoryScrollPosition to true, but its too late, the restore pos is gone.

We should simplify this to: if the document is loading, keep trying to restore. When the scroll frame is created we can check the loading status of the document.

I also noticed that PresShell::LoadComplete isn't quite what we want to be tracking as it only gets called on success. We also want to stop restoring on error or if the document loading was stopped. nsDocumentViewer::LoadComplete sets mLoaded, which is what I think we should track as for "loading/not loading".

Comment 2

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

Thanks for investigating! I'll write a patch that does what you suggested and see how that works. Also I'm amazed we don't have a test that covers this scenario, makes me feel less bad about the state of APZ tests :)

Comment 3

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

Attachment: MozReview Request: Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r?tnikkel

Review commit: https://reviewboard.mozilla.org/r/50173/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50173/

Comment 4

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

^ This is mostly a backout of the previous patch, combined with the new implementation. I left the old test in place, and added a new one to cover this new scenario.

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8748175 [details]
MozReview Request: Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r?tnikkel

https://reviewboard.mozilla.org/r/50173/#review47061

::: docshell/base/nsIContentViewer.idl:44
(Diff revision 1)
>  
>    attribute nsIDocShell container;
>  
>    [noscript,notxpcom,nostdcall] void loadStart(in nsIDocument aDoc);
>    void loadComplete(in nsresult aStatus);
> +  [noscript] readonly attribute boolean loadCompleted;

Can we make this notxpcom as well? I'm not sure what nostdcall implies, but if it lets us get rid of cruft I'm for it.

Comment 6

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

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> Can we make this notxpcom as well? I'm not sure what nostdcall implies, but
> if it lets us get rid of cruft I'm for it.

notxpcom is illegal on attributes, and nostdcall is a no-op for them, I believe.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9489784a0ea0

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9489784a0ea0

Comment 9

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

Comment on attachment 8748175 [details]
MozReview Request: Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r?tnikkel

Approval Request Comment
[Feature/regressing bug #]: bug 1268195
[User impact if declined]: reloading long pages that load incrementally can fail to restore the scroll position properly.
[Describe test coverage new/current, TreeHerder]: patch includes a test.
[Risks and why]: low/medium risk. See also risk assessment on bug 1268195; the two patches must be uplifted together. This patch backs out some of the changes from that bug so the overall footprint of them combined is smaller than either one in isolation.
[String/UUID change made/needed]: none

Comment 10

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

47 is unaffected, and 48 will be affected if bug 1268195 is uplifted to 48. I'll flip it directly to fixed if we uplift both patches.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8748175 [details]
MozReview Request: Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r?tnikkel

This should uplift to aurora along with the patch in bug 1268195
Good for 48 since we aim to start rolling out apz/e10s in that release.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5aad362431f0