[SessionHistory in the parent] One failure in browser_scrollPositions.js
Categories
(Core :: DOM: Navigation, defect, P3)
Tracking
()
Fission Milestone | M6c |
People
(Reporter: alchen, Assigned: jesup)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
10.97 KB,
patch
|
Details | Diff | Splinter Review | |
71.77 KB,
text/plain
|
Details |
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:
- Navigate URL1 and scroll the page. position = (SCROLL_X, SCROLL_Y)
- Use the same tab to Navigate URL2 and scroll the page. position = (SCROLL2_X, SCROLL2_Y)
- Close the tab and restore the tab. Now the tab is URL2.
- Do "goBack()" and now the tab is URL1.
- Check the position is (SCROLL_X, SCROLL_Y)
Reporter | ||
Comment 1•4 years ago
•
|
||
The problem is we don't get the correct shEntry.layoutHistoryState.
That's why the position is wrong in step 5.
Reporter | ||
Comment 2•4 years ago
|
||
In this patch, I use "mDocShell->SynchronizeLayoutHistoryState()" from child process before collecting sessionHistory in the parent process.
Reporter | ||
Comment 3•4 years ago
|
||
Reporter | ||
Comment 4•4 years ago
|
||
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).
Reporter | ||
Comment 5•4 years ago
|
||
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 -
Reporter | ||
Comment 6•4 years ago
|
||
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 -
Reporter | ||
Comment 7•4 years ago
|
||
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.
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 8•4 years ago
|
||
Comment 10•4 years ago
|
||
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•4 years ago
|
||
Tracking for Fission Nightly (M6) because Session Restore doesn't need to block Fission dogfooding (M5).
Reporter | ||
Comment 12•4 years ago
|
||
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?
Reporter | ||
Updated•4 years ago
|
Comment 13•4 years ago
|
||
M6b
Test depends on fission-history bug.
Updated•4 years ago
|
Comment 14•4 years ago
|
||
Alphan tested and this seems to be passing with sessionHistoryInParent pref on. Keeping this on file to re-test once bug 1656208 is done.
Updated•4 years ago
|
Comment 15•4 years ago
|
||
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•4 years ago
|
||
This is the same problem as bug 1572084. ContentSessionStore::GetScrollPositions
tries to traverse BrowsingContext
trees and access documents on children.
Comment 17•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
The double load could be a bfcache thing I guess. What do you think Olli?
Comment 20•4 years ago
|
||
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•4 years ago
|
||
(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.
Updated•4 years ago
|
Comment 22•4 years ago
|
||
This is expected to get re-enabled in bug 1673617.
Updated•3 years ago
|
Comment 23•3 years ago
|
||
This test got re-enabled in bug 1673617.
Description
•