Restored tabs are not at the correct scroll position.

With a tab open at this URL with an #anchor tag after quitting and restarting the browser, the page is restored scrolled to the top.

Expected result: instead of scrolled to the top, the tab is expected to be scrolled to line 205 where it was before the browser was shutdown and where the #anchor tag from the URL refers to.

Actual result: the tab is restored to the top of the page and not where it previously was scrolled to or where the URL #anchor refers to.

This is with "Restore previous session" set.

The priority flag is not set for this bug.
:mikedeboer, could you have a look please?

For more information, please visit auto_nag documentation.

So I looked into this, and this seems to be a session-restore bug.

We never get to scroll to the anchor because this is a history load (here's the code). But then we expect the scroll positions to be restored, but they don't, because SessionRestore has never saved that position in the first place.

SessionRestore is supposed to deal with the layout state...

Minimal STR:

• Open https://crisal.io/tmp/history-restore-non-root.html
• Scroll down to somewhere.
• Open another tab, but switch back to the URL above.
• Close the tab.
• Press Ctrl+Shift+T (open from frequently closed tabs).

We persist layout state on navigation just fine, that's what bug 1561900 was about. But here we get to nsDocShell::Destroy when closing the tab, in here, but there's nothing to gather that state anymore.

How is sessionrestore supposed to interact with the layout history of non-root scrollers? The root scroll position is stored separately, which is why this otherwise mostly works.

If this is not easily fixable from session restore, I guess we should tweak our heuristics to keep scrolling-to-anchor if the root scroller is not scrollable, or something.

Chrome seems to behave the same to us in this respect, fwiw.

Recently the content restore piece, which I think this state would be retrieved from, was converted to C++ by :alchen and it may actually be relatively straightforward to add support for now.
Let's ask Alphan!

I will check this later.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

So I looked into this, and this seems to be a session-restore bug.

We never get to scroll to the anchor because this is a history load (here's the code). But then we expect the scroll positions to be restored, but they don't, because SessionRestore has never saved that position in the first place.

SessionRestore is supposed to deal with the layout state...

Yes, sessionRestore is supposed to collect and restore the layout state in sessionHistoryChanges of sessionRestore data.
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/toolkit/modules/sessionstore/SessionHistory.jsm#136
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/toolkit/modules/sessionstore/SessionHistory.jsm#251

After doing the following steps, we don't collect any history changes due to there is no notification.

• Open https://crisal.io/tmp/history-restore-non-root.html
• Scroll down to somewhere.
• Close the tab.

We collect history changes if get notification from these cases.
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/browser/components/sessionstore/ContentSessionStore.jsm#128,136,152

1. addSHistoryListener():
   OnHistoryNewEntry(), OnHistoryGotoIndex(), OnHistoryPurge(), OnHistoryReload(), OnHistoryReplaceEntry()
   https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/browser/components/sessionstore/ContentSessionStore.jsm#210,216,221,225,230
2. webProgress.addProgressListener(NOTIFY_STATE_DOCUMENT)
   nsIWebProgressListener.STATE_START, nsIWebProgressListener.STATE_STOP
   https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/browser/components/sessionstore/ContentSessionStore.jsm#237,253,255
3. addEventListener("DOMTitleChanged")
   https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/browser/components/sessionstore/ContentSessionStore.jsm#206

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Minimal STR:

• Open https://crisal.io/tmp/history-restore-non-root.html
• Scroll down to somewhere.
• Open another tab,

After "open another tab", sessionRestore gets notifications. (onStateChange(STATE_START), OnHistoryNewEntry(), handleEvent("DOMTitleChanged"), onStateChange(STATE_STOP))
So sessionRestore did a SessionHistory.collect() after receiving the notifications.
In this data, we have "presState" in it.

{"data":
  {"historychange":
    {"entries":[
      {"url":"about:newtab",....},
      {"url":"https://crisal.io/tmp/history-restore-non-root.html", ..."presState":[{"stateKey":"0>div>0>1>1","scroll":"0,101640"}], ...},
      {"url":"https://www.wikipedia.org/"...}
    ]}
  }
}

but switch back to the URL above.

After "switch back to https://crisal.io/tmp/history-restore-non-root.html", sessionRestore also did a SessionHistory.collect().
In this data, the data are the same as the previous data except "presState".

{"data":
  {"historychange":
    {"entries":[
      {"url":"about:newtab",....},
      {"url":"https://crisal.io/tmp/history-restore-non-root.html", ...},
      {"url":"https://www.wikipedia.org/"...}
    ]}
  }
}

• Close the tab.

As I said in comment #8, there is no notification.
So sessionRestore data is the same as before.

• Press Ctrl+Shift+T (open from frequently closed tabs).

Since there is no "presState", sessionRestore cannot restore the scroll position as we expected.

Based on comment #8 and comment #9, we need the comments from people who know how sessionHistory works with layout.

After "switch back to https://crisal.io/tmp/history-restore-non-root.html", sessionRestore also did a SessionHistory.collect().

Why does this happen? Which notification triggers this? Does this collection happen synchronously? if not, maybe you had already closed the tab somehow? I expect the pres state not to be there if the pres shell has already been destroyed or such.

(In reply to Emilio Cobos Álvarez (:emilio) (PTO until Jul 10) from comment #11)

After "switch back to https://crisal.io/tmp/history-restore-non-root.html", sessionRestore also did a SessionHistory.collect().

Why does this happen? Which notification triggers this? Does this collection happen synchronously? if not, maybe you had already closed the tab somehow? I expect the pres state not to be there if the pres shell has already been destroyed or such.

The collection is also triggered by these notifications. (onStateChange(STATE_START), OnHistoryNewEntry(), handleEvent("DOMTitleChanged"), onStateChange(STATE_STOP)).
In this case, the collection is for "https://crisal.io/tmp/history-restore-non-root.html".

No, it does not happen synchronously.
When these notifications happened, we wait 1000 ms to batch multiple updates and do the collection at a time.
This means that we batch "(onStateChange(STATE_START), OnHistoryNewEntry(), handleEvent("DOMTitleChanged"), onStateChange(STATE_STOP))" into 1 collection.
I saw the data we collect from sessionHistory just after the tab is loaded successfully. (I don't need to close the tab)

I think the problem here is sessionRestore should be notified when there are preState changes.
Is there any event sessionRestore can listen to know these changes happen?
For normal scroll position changes, we listen "mozvisualscroll" event.
Or does sessionHistory know there are 'preState' changes?
If so, we can also add a callback in sessionHistoryListener.

1. addSHistoryListener():
   OnHistoryNewEntry(), OnHistoryGotoIndex(), OnHistoryPurge(), OnHistoryReload(), OnHistoryReplaceEntry()
   https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/browser/components/sessionstore/ContentSessionStore.jsm#210,216,221,225,230

3. addEventListener("DOMTitleChanged")
   https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/browser/components/sessionstore/ContentSessionStore.jsm#206

nsHTMLScrollFrames are nsIStatefulFrame objects, so https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/layout/base/nsFrameManager.cpp#125 should handle them.
Do we restore that information at wrong point? Or have we not even collected that data?