1312697 - Scroll position not saved on local page ( file:///)

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[rulumasi]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36

Steps to reproduce:

1. Open some page like
    file:///usr/share/doc/glibc-doc-reference/html/Advanced-Signal-Handling.html
2. Scroll vertically using mouse or keyboard.
3. Press F5.



Actual results:

Page reloaded.
Vertical scroll position (window.scrollY) is 0.


Expected results:

Store scroll position.
Reload page.
Restore scroll position.

Comment 1

[rulumasi]

This bug affects "file://" URIs.

Comment 2

[Loic]

Could you attach the page "/Advanced-Signal-Handling.html" to the bug report, please.

Comment 3

[Adrian Florinescu [:aflorinescu]]

Attachment: The GNU C Library: Concept Index.html

Comment 4

[Adrian Florinescu [:aflorinescu]]

Problem reproduced on both latest 49.0.2 and on latest 52.a01. 
The problem is reproducible with a local html, but not with the live version of it.

I used:
not reproducible -> live: http://www.gnu.org/software/libc/manual/html_node/Concept-Index.html#Concept-Index
reproducible -> local: (see attachment) file:///home/adrian.florinescu/Downloads/The%20GNU%20C%20Library:%20Concept%20Index.html


Last good revision: ceb2fd9beec76ec09fd89ba5d3dd736035877131
First bad revision: 9489784a0ea049b149ad0c6d22ac121ef2542239
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ceb2fd9beec76ec09fd89ba5d3dd736035877131&tochange=9489784a0ea049b149ad0c6d22ac121ef2542239

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1269539

Comment 5

[Alice0775 White]

Interestingly,
If url has a hash(any word ok like[1]) then the scroll position persists after scroll and reload(F5).

[1] file:///usr/share/doc/glibc-doc-reference/html/Advanced-Signal-Handling.html#foobarbaz

Comment 6

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

Thanks, I'll take a look.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Botond, can you take a look?

Comment 8

[Botond Ballo [:botond]]

I can repro this, but only intermittently. Pressing F5 twice in quick succession makes it somewhat easier to repro, although still not reliably.

Comment 9

[Botond Ballo [:botond]]

I spent a bunch of time debugging this today with Timothy's help. It looks like not only is frame reconstruction happening as the page is loading, but we're getting a new content node, and even a new pres shell. Don't understand why yet...

Comment 10

[Botond Ballo [:botond]]

Attachment: Stack trace for the creation of the nsDocumentOpenInfo that triggers the creation of the first pres shell

Comment 11

[Botond Ballo [:botond]]

Attachment: Stack trace for the creation of the nsDocumentOpenInfo that triggers the creation of the second pres shell (C++)

Comment 12

[Botond Ballo [:botond]]

Attachment: Stack trace for the creation of the nsDocumentOpenInfo that triggers the creation of the second pres shell (JS)

Comment 13

[Botond Ballo [:botond]]

So yeah, there's definitely two pres shells being created with the same content during the reload.

Attached are stack traces showing what is creating the nsDocumentOpenInfo objects that in turn trigger the creation of the pres shells (the stack trace from the PresShell construct itself isn't interesting, it's just called from nsInputStreamReadyEvent::Run). For the second one, chrome JS code is also involved, so I posted the JS stack trace as well.

If anyone has an explanation for why two pres shells are being created based on these stack traces, and whether this behaviour is expected, I would be interested to hear it.

Comment 14

[Timothy Nikkel (:tnikkel)]

nsHtml5TreeOpExecutor::NeedsCharsetSwitchTo is in the stack so I'm guessing that part way through parsing we page we realize we chose the wrong charset for the page and so we reload the page with the right charset. That seems to make sense.

So then the question remains as to why we are told the page is loaded but we haven't been notified about all the content of the page.

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1312697 - Do not consider the page to be finished loading if it's in the 'stopped' state.

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

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8837328 [details]
Bug 1312697 - Do not consider the page to be finished loading if it's in the 'stopped' state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112484/diff/1-2/

Comment 17

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #14)
> So then the question remains as to why we are told the page is loaded but we
> haven't been notified about all the content of the page.

It appears like this is expected behaviour from the document viewer. However, as you pointed out on IRC, we can discriminate between this situation and a real "load complete" of the sort we are interested in, by checking nsDocumentViewer::mStopped. This is what the posted patch does.

Comment 18

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8837328 [details]
Bug 1312697 - Do not consider the page to be finished loading if it's in the 'stopped' state.

https://reviewboard.mozilla.org/r/112484/#review114452

It seems like the ideal state would be to stop trying to restore the scroll position in a stopped document, but store the restore position (if there hasn't been a scroll position change by other means) in the frame state, so that if the document does get reloaded we do try to restore.

This is maybe too much of an edge case to care about. Is it at all reasonable to implement?

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8837328 [details]
Bug 1312697 - Do not consider the page to be finished loading if it's in the 'stopped' state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112484/diff/2-3/

Comment 20

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #18)
> It seems like the ideal state would be to stop trying to restore the scroll
> position in a stopped document, but store the restore position (if there
> hasn't been a scroll position change by other means) in the frame state, so
> that if the document does get reloaded we do try to restore.
> 
> This is maybe too much of an edge case to care about. Is it at all
> reasonable to implement?

It's definitely an edge case, and we probably shouldn't spend too much time on it, but I did make a fairly straightforward effort to support it, in the updated patch.

The idea here is that if we're in the stopped state, we do not scroll to the restore position, but we also don't clear the restore position, so that if we subsequently get back into the loading state, we can keep trying to restore.

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8837328 [details]
Bug 1312697 - Do not consider the page to be finished loading if it's in the 'stopped' state.

https://reviewboard.mozilla.org/r/112484/#review116186

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

Can we make this notxpcom, nostdcall?

::: layout/generic/nsGfxScrollFrame.cpp:4219
(Diff revision 3)
>      // if our desired position is different to the scroll position, scroll.
>      // remember that we could be incrementally loading so we may enter
>      // and scroll many times.
>      if (mRestorePos != mLastPos /* GetLogicalScrollPosition() */) {
> +      LoadingState state = GetPageLoadingState();
> +      if (state == LoadingState::Stopped) {

I think we also want to check if NS_SUBTREE_DIRTY(mOuter) because we may have a reflow pending just following the document getting "stopped" and so we want to try once the reflow is finished. But not after that.

Comment 22

[Timothy Nikkel (:tnikkel)]

This is another tall ask, but would it be possible to write a test for this? We've had a long trail of regressions in this area, and each time we fix a new bug we have to go back and make sure not to regress any of the previous changes. The original testcase is racy, but I think we could do something like this:

1) use an sjs file to create a long page that loads slowly
2) call stop on the load before it finishes loading
3) call reload on the document
4) check that the document scrolls to expected spot

Comment 23

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #21)
> ::: docshell/base/nsIContentViewer.idl:46
> (Diff revision 3)
> >  
> >    [noscript,notxpcom,nostdcall] void loadStart(in nsIDocument aDoc);
> >    void loadComplete(in nsresult aStatus);
> >    [noscript] readonly attribute boolean loadCompleted;
> >  
> > +  [noscript] readonly attribute boolean isStopped;
> 
> Can we make this notxpcom, nostdcall?

You asked :kats the same thing for 'loadCompleted' in bug 1269539 comment 5. I believe his answer in bug 1269539 comment 6 applies here as well.

> ::: layout/generic/nsGfxScrollFrame.cpp:4219
> (Diff revision 3)
> >      // if our desired position is different to the scroll position, scroll.
> >      // remember that we could be incrementally loading so we may enter
> >      // and scroll many times.
> >      if (mRestorePos != mLastPos /* GetLogicalScrollPosition() */) {
> > +      LoadingState state = GetPageLoadingState();
> > +      if (state == LoadingState::Stopped) {
> 
> I think we also want to check if NS_SUBTREE_DIRTY(mOuter) because we may
> have a reflow pending just following the document getting "stopped" and so
> we want to try once the reflow is finished. But not after that.

Will do.

(In reply to Timothy Nikkel (:tnikkel) from comment #22)
> This is another tall ask, but would it be possible to write a test for this?
> We've had a long trail of regressions in this area, and each time we fix a
> new bug we have to go back and make sure not to regress any of the previous
> changes. The original testcase is racy, but I think we could do something
> like this:
> 
> 1) use an sjs file to create a long page that loads slowly
> 2) call stop on the load before it finishes loading
> 3) call reload on the document
> 4) check that the document scrolls to expected spot

I can give this a try.

Comment 24

[Botond Ballo [:botond]]

Attachment: Test WIP

Here's a WIP patch to add the requested test. I was hoping to finish this before leaving for my standards meeting, but it doesn't look like I'll have time to.

The test is currently passing (except for the error related to the use of a flaky timeout) even without the fix, because ScrollToRestoredPosition() never seems to get called in the "stopped" state, and thus the bad behaviour (clearing mRestorePos in the stopped state) is not triggered.

I'll keep investigating this when I get back, but if in the meantime you have any suggestins as to why this might be happening, please feel free to share them.

(Also, if you have a suggestion for how to avoid using a "flaky timeout", I'd be interested as well. If I don't use window.setTimeout() to wait between the reload() and stop() calls, the page does not even start to get reloaded by the time I check its scrollY.)

Comment 25

[Botond Ballo [:botond]]

Attachment: Bug 1312697 - Mochitest.

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

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8837328 [details]
Bug 1312697 - Do not consider the page to be finished loading if it's in the 'stopped' state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112484/diff/3-4/

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #24)
> Created attachment 8841142 [details] [diff] [review]
> Test WIP
> 
> Here's a WIP patch to add the requested test. I was hoping to finish this
> before leaving for my standards meeting, but it doesn't look like I'll have
> time to.
> 
> The test is currently passing (except for the error related to the use of a
> flaky timeout) even without the fix, because ScrollToRestoredPosition()
> never seems to get called in the "stopped" state, and thus the bad behaviour
> (clearing mRestorePos in the stopped state) is not triggered.
> 
> I'll keep investigating this when I get back, but if in the meantime you
> have any suggestins as to why this might be happening, please feel free to
> share them.

I'm not sure. You could try seeing how/why ScrollToRestoredPosition is called in the reproducing testcase, and then see why that isn't happening in your test.

> (Also, if you have a suggestion for how to avoid using a "flaky timeout",
> I'd be interested as well. If I don't use window.setTimeout() to wait
> between the reload() and stop() calls, the page does not even start to get
> reloaded by the time I check its scrollY.)

You can call Simpletest.requestFlakyTimeout to silence the warning.

Comment 28

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #27)
> (In reply to Botond Ballo [:botond] from comment #24)

Sorry if this wasn't clear, but the issues described in comment 24 are already addressed in the updated patch posted on March 17.

Comment 29

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8848666 [details]
Bug 1312697 - Mochitest.

https://reviewboard.mozilla.org/r/121568/#review133532

Thank you for writing this test!!

Comment 30

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8837328 [details]
Bug 1312697 - Do not consider the page to be finished loading if it's in the 'stopped' state.

https://reviewboard.mozilla.org/r/112484/#review133542

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b4f12acb0bc
Do not consider the page to be finished loading if it's in the 'stopped' state. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/e3fd79ba263d
Mochitest. r=tnikkel

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b4f12acb0bc
https://hg.mozilla.org/mozilla-central/rev/e3fd79ba263d

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Is this something that can ride the 55 train or should we consider it for uplift to Beta?

Comment 34

[Botond Ballo [:botond]]

I think the STR are sufficiently niche, and the code being touched sufficiently tricky, that I'd prefer letting it ride the trains. I set firefox54:wontfix accordingly.

Comment 35

[Ciprian Georgiu, Desktop QA]

I've reproduce this issue using the test case from comment 3 and STR from comment 0, on Nightly 52.0a1.

This is verified fixed on 55.0b2 (20170615133456) under Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 LTS.