1602486 - [SessionHistory in the parent] One failure in browser_scrollPositions.js

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Alphan Chen [:alchen]]

When doing mochitest with the attachment 9114930 [details] (patch of bug 1507287) and pref " fission.sessionHistoryInParent", there is still one failure in browser_scrollPositions.js.
TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_scrollPositions.js | scroll is still fine after navigating back - Got null, expected {"scroll":"184,215"}

The STR is like below:

1. Navigate URL1 and scroll the page. position = (SCROLL_X, SCROLL_Y)
2. Use the same tab to Navigate URL2 and scroll the page. position = (SCROLL2_X, SCROLL2_Y)
3. Close the tab and restore the tab. Now the tab is URL2.
4. Do "goBack()" and now the tab is URL1.
5. Check the position is (SCROLL_X, SCROLL_Y)

Comment 1

[Alphan Chen [:alchen]]

The problem is we don't get the correct shEntry.layoutHistoryState.
That's why the position is wrong in step 5.

Comment 2

[Alphan Chen [:alchen]]

Attachment: 0001-Bug-1602486-SessionHistory-in-the-parent-One-failure.patch

In this patch, I use "mDocShell->SynchronizeLayoutHistoryState()" from child process before collecting sessionHistory in the parent process.

Comment 3

[Alphan Chen [:alchen]]

Attachment: 0001-Bug-1602486-SessionHistory-in-the-parent-One-failure.patch

Comment 4

[Alphan Chen [:alchen]]

We can know the problem clearly by switching the pref "fission.sessionHistoryInParent".
When pref off, the sessionHistory is collected from the child process.
When pref on, the sessionHistory is collected from the parent process.
However, they both use the same collect function. (SessionHistory.collectCommon).

Comment 5

[Alphan Chen [:alchen]]

From the log, we need to collect sessionHistory before loading URL2. (before step 2)
Here is the correct behavior: we can get layoutHistoryState with scrollposition data.

(pref off)
0:12.82 PASS scroll on first page is fine -
0:12.82 GECKO(9515) =====browser_scrollPositions.js================== before load URL2
0:13.83 GECKO(9515) ====DEBUG==== @ SessionHistory.jsm Enter SessionHistory.collectCommon()
0:13.83 GECKO(9515) ====DEBUG==== @ SessionHistory.jsm Enter serializeEntry()
0:13.83 GECKO(9515) ====DEBUG==== @ SessionHistory.jsm layoutHistoryState.hasStates=true
0:13.83 GECKO(9515) ====DEBUG==== @ SessionHistory.jsm preState=[{"stateKey":"0>html>1","scroll":"7500,15360","scrollOriginDowngrade":false}]
0:13.83 GECKO(9515) ====DEBUG==== @ SessionHistory.jsm Enter serializeEntry()
0:13.84 GECKO(9515) ====DEBUG==== @ SessionHistory.jsm layoutHistoryState.hasStates=false
0:13.84 GECKO(9515) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
0:13.87 GECKO(9515) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.87 GECKO(9515) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
0:13.87 PASS scroll on second page is fine -
0:13.87 GECKO(9515) =====browser_scrollPositions.js================== before closeWindow(newWin)
0:13.92 GECKO(9515) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.92 GECKO(9515) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.92 GECKO(9515) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
0:13.92 GECKO(9515) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
0:13.94 GECKO(9515) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.94 GECKO(9515) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.94 GECKO(9515) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.95 GECKO(9515) =====browser_scrollPositions.js================== after await forceSaveState()
0:14.55 PASS There should be two tabs -

Comment 6

[Alphan Chen [:alchen]]

From the log, we also collect sessionHistory before loading URL2. (before step 2)
However, there is no state that we can collect here.

(pref on)
0:12.70 PASS scroll on first page is fine -
0:12.70 GECKO(7117) =====browser_scrollPositions.js================== before load URL2
0:13.83 GECKO(7117) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() with aData.sHistoryNeeded.!!!
0:13.83 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm Enter SessionHistory.collectCommon()
0:13.83 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm Enter serializeEntry()
0:13.83 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm layoutHistoryState.hasStates=false
0:13.84 GECKO(7117) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
0:13.84 PASS scroll on second page is fine -
0:13.84 GECKO(7117) =====browser_scrollPositions.js================== before closeWindow(newWin)
0:13.89 GECKO(7117) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() with aData.sHistoryNeeded.!!!
0:13.89 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm Enter SessionHistory.collectCommon()
0:13.89 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm Enter serializeEntry()
0:13.89 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm layoutHistoryState.hasStates=false
0:13.91 GECKO(7117) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.91 GECKO(7117) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
0:13.91 GECKO(7117) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
0:13.91 GECKO(7117) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() with aData.sHistoryNeeded.!!!
0:13.91 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm Enter SessionHistory.collectCommon()
0:13.91 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm Enter serializeEntry()
0:13.91 GECKO(7117) ====DEBUG==== @ SessionHistory.jsm layoutHistoryState.hasStates=false
0:13.91 GECKO(7117) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.91 GECKO(7117) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
0:13.93 GECKO(7117) =====browser_scrollPositions.js================== after await forceSaveState()
0:14.53 GECKO(7117) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:AddSHistoryListener)!!!
0:14.54 PASS There should be two tabs -

Comment 7

[Alphan Chen [:alchen]]

Comment on attachment 9114592 [details] [diff] [review]
0001-Bug-1602486-SessionHistory-in-the-parent-One-failure.patch

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

The log will become:

 0:12.98 PASS scroll on first page is fine - 
 0:12.98 GECKO(14651) =====browser_scrollPositions.js================== before load URL2
 0:14.06 GECKO(14651) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() with aData.sHistoryNeeded.!!!
 0:14.06 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm Enter SessionHistory.collectCommon()
 0:14.06 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm Enter serializeEntry()
 0:14.06 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm No shEntry.layoutHistoryState.
 0:14.06 GECKO(14651) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
 0:14.06 PASS scroll on second page is fine - 
 0:14.07 GECKO(14651) =====browser_scrollPositions.js================== before closeWindow(newWin)
 0:14.11 GECKO(14651) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() with aData.sHistoryNeeded.!!!
 0:14.11 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm Enter SessionHistory.collectCommon()
 0:14.11 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm Enter serializeEntry()
 0:14.11 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm No shEntry.layoutHistoryState.
 0:14.12 GECKO(14651) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
 0:14.13 GECKO(14651) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
 0:14.13 GECKO(14651) ====DEBUG==== @ SessionStore.jsm receiveMessage(SessionStore:update)!!!
 0:14.13 GECKO(14651) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() with aData.sHistoryNeeded.!!!
 0:14.13 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm Enter SessionHistory.collectCommon()
 0:14.13 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm Enter serializeEntry()
 0:14.13 GECKO(14651) ====DEBUG==== @ SessionHistory.jsm No shEntry.layoutHistoryState.
 0:14.13 GECKO(14651) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!
 0:14.13 GECKO(14651) ====DEBUG==== @ SessionStore.jsm updateSessionStoreFromTablistener() without aData.sHistoryNeeded.!!!

::: toolkit/components/sessionstore/SessionStoreListener.h
@@ +70,5 @@
>    bool GetAndClearSHistoryCanged() {
>      bool ret = mSHistoryCanged;
> +    if (ret) {
> +      mSHistoryCanged = false;
> +      mDocShell->SynchronizeLayoutHistoryState();

If we don't do this, we will find that there is no layoutHistoryState in the shEntry.

Comment 8

[Alphan Chen [:alchen]]

Attachment: Base_PATCH_For_THIS_BUG

Comment 9

[Chris Peterson [:cpeterson]]

Tracking for Fission mochitests (M4.1)

Comment 10

[Neha Kochar [:neha]]

Since this is only with fission.sessionHistoryInParent which isn't enabled by default yet, it does not belong to M4.1. Moving this to M5.

Comment 11

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6) because Session Restore doesn't need to block Fission dogfooding (M5).

Comment 12

[Alphan Chen [:alchen]]

For this bug, it is a mochitest test failure when enabling sessionHistory in the parent.
This is related to sessionHistory in parent implementation.
Per the previous discussion with Peter, we should check the fission mochitest result after the new implementation is landed.
Since I won't have any action in a short time, unassign the bug now.

Hi Peter,
I cannot find a suitable bug that I can add it into dependency to show this bug block "enabling sessionHistory in the parent".
Could you help?

Comment 13

[Chris Peterson [:cpeterson]]

M6b
Test depends on fission-history bug.

Comment 14

[Neha Kochar [:neha]]

Alphan tested and this seems to be passing with sessionHistoryInParent pref on. Keeping this on file to re-test once bug 1656208 is done.

Comment 15

[Neha Kochar [:neha]]

Andreas, can you look into this one? Comment 14 suggests it might be fixed with SHIP but SHIP-failures spreadsheet has this test so might not be fixed just yet.

Comment 16

[Andreas Farre [:farre]]

This is the same problem as bug 1572084. ContentSessionStore::GetScrollPositions tries to traverse BrowsingContext trees and access documents on children.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/sessionstore/SessionStoreListener.cpp#476-481

Comment 17

[u608768]

Unless I'm mistaken, all the documents in this test are same-origin (and SessionStoreListener should work fine for those). I think the problem is that we're failing to restore scroll positions for background tabs (which again, I can't really reproduce with manual testing).

Comment 18

[Andreas Farre [:farre]]

Right, so it turns out that there are two errors. The first is that ContentSessionStore::OnDocumentStart() fires twice in the test when testing undoCloseWindow. Adding:

   // Now check to see if the background tab remembers where it
   // should be scrolled to.
   newWin.gBrowser.selectedTab = tab;
   await promiseTabRestored(tab);
 
+  // For some reason we get two loads in Fission. Waiting for the
+  // extra one is needed to not try to check scroll inbetween
+  // ContentSessionStore::OnDocumentStart and
+  // ContentSessionStore::OnDocumentEnd.
+  if (Services.appinfo.sessionHistoryInParent) {
+    await BrowserTestUtils.browserLoaded(browser);
+  }
+

"fixes" that. The correct thing to do is probably find out why we do two loads?

Then there's another issue, that happens because scroll restoration fails. I have two pernosco instances, one with Fission and SHIP[1] and one without[2].

Not at all sure why this happens.

[1] https://pernos.co/debug/25vpjrG-1iOGIfqqmvAYrA/index.html#f{m[Au3W,FJu1_,t[AuU,A3mh_,f{e[Au3W,FJur_,s{af2LPXuAA,bAXk,oHRzAYQ,uHPLQXQ___
[2] https://pernos.co/debug/nwfI4hfAnkAhcbYxhGi4uQ/index.html#f{m[Aiz8,Ktxs_,t[Ao0,A4BS_,f{e[Aiz8,KjOo_,s{afwWB/JAA,bAXk,oHIuKeQ,uHHWOTQ___

Comment 19

[Andreas Farre [:farre]]

The double load could be a bfcache thing I guess. What do you think Olli?

Comment 20

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

Both browser_scrollPositions_sample.html and browser_scrollPositions_sample2.html can go to bfcache.

But comment 18 seems to be about some tab restoring. That shouldn't be bfcache related.
And when calling goBack() that seems to be for a case when we have just restored from sessionstore, so nothing should be in
bfcache in that case either.

Comment 21

[u608768]

(In reply to Andreas Farre [:farre] from comment #18)

The correct thing to do is probably find out why we do two loads?

The two loads we're seeing are actually for different documents.

The first is from ContentRestore.onNewHistoryEntry, after we set selectedTab on the restored window. SessionStore sends a "SessionStore:restoreHistory" to ContentSessionStore, which is forwarded to ContentRestore. ContentRestore sets some internal state and sends a "SessionStore:restoreSHistoryInParent" back to SessionStore. SessionStore then adds a shistory listener to the browser (in addSHistoryListenerForRestore). Here we'll see a OnHistoryNewEntry event, and we'll send a "SessionStore:OnHistoryNewEntry" message back to ContentSessionStore. It's forwarded to ContentRestore again, where we'll kick off the load for the document that we just navigated away from.

Now for the second load, which is, somehow, even more confusing: this load is kicked off in nsSHistory::ReloadCurrentEntry(), which is called by SessionStore in response to a "SessionStore:reloadCurrentEntry" message from ContentRestore. This happens after SessionStore sends ContentSessionStore a "SessionStore:finishRestoreHistory" (I don't quite understand how the timing for this works out yet).

I'm not sure what to do here yet, but I think fixing this will also fix that second issue from comment #18. I'm also not sure if/why any of this has to actually happen in content when SHIP is enabled.

This also sounds like it may be the reason for the load loops we're seeing in bug 1668087.

Comment 22

[Neha Kochar [:neha]]

This is expected to get re-enabled in bug 1673617.

Comment 23

[Neha Kochar [:neha]]

This test got re-enabled in bug 1673617.