Make sessionRestore work with session history living in the parent process
Categories
(Firefox :: Session Restore, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox76 | --- | fixed |
People
(Reporter: alchen, Assigned: alchen)
References
(Blocks 4 open bugs)
Details
Attachments
(2 files, 2 obsolete files)
Before doing bug 1474130, I would like to rewrite the js modules which are used in ContentSessionStore.jsm. In this bug, I will focus on SessionHistory.jsm.
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Since session history will move into the parent process, this bug will be held and wait for bug 1438272 and bug 1445459.
Assignee | ||
Comment 2•5 years ago
|
||
Rename the bug as "Convert SessionHistoryListener to C++".
This bug will be served as the stage III of ContentSessionStore rewriting.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 3•4 years ago
|
||
This work should happen on the ash branch, where we've moved session history into the parent. The listeners are already called in the parent process on that branch, so the session store's listener should just be registered in the parent process too.
Looking at the existing code, we get the session history from a docshell and then register the listener. I think what needs to happen is instead to send that docshell's BrowsingContext over IPC to the parent process. In the parent process you'll then have a CanonicalBrowsingContext (might need to use CanonicalBrowsingContext::Cast to get the right type). From the CanonicalBrowsingContext you can get the session history object (CanonicalBrowsingContext::GetSessionHistory), and register the listener using that.
You have access to the entries in the parent process, so it should be straightforward to convert the code that gathers data from the entries. There are some places where we get data from the docshell, in the parent process we should get it from the CanonicalBrowsingContext instead. Please talk to nika or farre if anything is missing on CanonicalBrowsingContext that you need access to.
Assignee | ||
Comment 4•4 years ago
|
||
- Move StateChangeNotifier/SessionHistoryListener into SessionHistoryListener.jsm
- Implement sHistoryListener
= (in C++) collect shistory when get "DOMTitleChanged" event, OnDocumentStart and OnDocumentEnd.
= (in JS)- AddSHistoryListener in parent process.
(send "SessionStore:AddSHistoryListener" from child to parent) - Store sHistoryListener into a new WeakMap()
("this._browserSHistoryListener" : Use browser.permanentKey as the key) - Add uninstall() to remove the listener from BC
- Add notifySHistoryChanges() to notify SessionStoreListener about the changes
- AddSHistoryListener in parent process.
- Add collectFromParent() in SessionHistory.jsm
Comment 5•4 years ago
|
||
// Don't try to restore framesets containing wyciwyg URLs.
// Note that these may be left over from pre-wyciwyg-removal profiles.
That seems to have happened some 11-12 years ago, is the check still needed?
Assignee | ||
Comment 6•4 years ago
•
|
||
The collect() part is almost finished.
However we will meet crashes when getting "shEntry.layoutHistoryState" and "shEntry.BFCacheEntry".
I have no chance to try "shEntry.stateData" when running the test. (browser/components/sessionstore/test/browser_scrollPositions.js)
shEntry is from the following steps:
@SessionStore.jsm
sHistory = aBrowser.frameLoader.browsingContext.sessionHistory
@SessionHistory.jsm
let shistory = sHistory.QueryInterface(Ci.nsISHistory);
for (; entryCount < count; entryCount++) {
let shEntry = shistory.getEntryAtIndex(entryCount);
....
}
Assignee | ||
Comment 7•4 years ago
|
||
(In reply to Laurentiu Nicola from comment #5)
// Don't try to restore framesets containing wyciwyg URLs.
// Note that these may be left over from pre-wyciwyg-removal profiles.
That seems to have happened some 11-12 years ago, is the check still needed?
Thanks for your reminding.
We don't need this anymore.
In bug 1489308, we remove all the "wyciwyg" bits.
Assignee | ||
Comment 8•4 years ago
|
||
After adding the restore part, I meet crash when doing restore().
Reproduce Step:
- Open a random page. (e.g. https://en.wikipedia.org/wiki/Wiki)
- Trigger a restore by "Duplicate Tab".
[Call stack]
Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
mozilla::ipc::IPDLParamTraits<mozilla::dom::PSHEntryParent*>::Write (aMsg=0x7fffd2b725f0, aActor=0x7fffd5bd4240, aVar=@0x7fffffffbc50: 0xe5e5e5e5e5e5e5e5) at PSHEntryParent.cpp:3115
3115 id = aVar->Id();
(gdb) bt
#0 0x00007fffe733556d in mozilla::ipc::IPDLParamTraits<mozilla::dom::PSHEntryParent*>::Write(IPC::Message*, mozilla::ipc::IProtocol*, mozilla::dom::PSHEntryParent* const&) (aMsg=0x7fffd2b725f0, aActor=0x7fffd5bd4240, aVar=@0x7fffffffbc50: 0xe5e5e5e5e5e5e5e5) at PSHEntryParent.cpp:3115
#1 0x00007fffe710766f in mozilla::ipc::IPDLParamTraits<mozilla::dom::MaybeNewPSHEntry>::Write(IPC::Message*, mozilla::ipc::IProtocol*, mozilla::dom::MaybeNewPSHEntry const&) (aMsg=0x7fffd2b725f0, aActor=0x7fffd5bd4240, aVar=...)
at NewPSHEntry.cpp:809
#2 0x00007fffe733659a in mozilla::ipc::IPDLParamTraits<mozilla::dom::LoadSHEntryData>::Write(IPC::Message*, mozilla::ipc::IProtocol*, mozilla::dom::LoadSHEntryData const&) (aMsg=0x7fffd2b725f0, aActor=0x7fffd5bd4240, aVar=...)
at PSHistory.cpp:290
#3 0x00007fffe733659a in mozilla::ipc::WriteIPDLParam<mozilla::dom::LoadSHEntryData const&>(IPC::Message*, mozilla::ipc::IProtocol*, mozilla::dom::LoadSHEntryData const&) (aMsg=0x7fffd2b725f0, aActor=0x7fffd5bd4240, aParam=...)
at /home/alphan/src/SessionHistory/ash/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:59
#4 0x00007fffe73364b2 in mozilla::ipc::IPDLParamTraits<mozilla::dom::LoadSHEntryResult>::Write(IPC::Message*, mozilla::ipc::IProtocol*, mozilla::dom::LoadSHEntryResult const&) (aMsg=0x7fffd2b725f0, aActor=0x7fffd5bd4240, aVar=...)
at PSHistory.cpp:561
#5 0x00007fffe733b7bb in mozilla::dom::PSHistoryParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (this=0x7fffd5bd4240, msg__=..., reply__=@0x7fffffffbff8: 0x7fffd2b725f0) at PSHistoryParent.cpp:741
#6 0x00007fffe718ce4f in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (this=0x7fffd61ab000, msg__=..., reply__=<optimized out>) at PContentParent.cpp:11731
#7 0x00007fffe707308a in mozilla::ipc::MessageChannel::DispatchSyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&, IPC::Message*&) (this=0x7fffd61ab100, aProxy=0x7fffd614b6e0, aMsg=..., aReply=@0x7fffffffbff8: 0x7fffd2b725f0) at /home/alphan/src/SessionHistory/ash/ipc/glue/MessageChannel.cpp:2154
#8 0x00007fffe707206d in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (this=0x7fffd61ab100, aMsg=...) at /home/alphan/src/SessionHistory/ash/ipc/glue/MessageChannel.cpp:2105
#9 0x00007fffe7072827 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) (this=0x7fffd61ab100, aTask=...) at /home/alphan/src/SessionHistory/ash/ipc/glue/MessageChannel.cpp:1954
#10 0x00007fffe7072c2e in mozilla::ipc::MessageChannel::MessageTask::Run() (this=0x7fffd0c64e20) at /home/alphan/src/SessionHistory/ash/ipc/glue/MessageChannel.cpp:1985
#11 0x00007fffe6a3ee72 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7fffe0d32200, aMayWait=<optimized out>, aResult=<optimized out>) at /home/alphan/src/SessionHistory/ash/xpcom/threads/nsThread.cpp:1225
#12 0x00007fffe6a40d48 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0xe5e5e5e5e5e5e5e5, aMayWait=<optimized out>) at /home/alphan/src/SessionHistory/ash/xpcom/threads/nsThreadUtils.cpp:486
#13 0x00007fffe707571a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7fffe0d1f500, aDelegate=0x7ffff6a6a3c0) at /home/alphan/src/Se
Assignee | ||
Comment 9•4 years ago
|
||
Assignee | ||
Comment 10•4 years ago
|
||
Update current status,
Now there are two symptoms:
-
In some test cases, I cannot get sessionHistory object in the parent process.
If I cannot get sessionHistory object, the sessionHistory restore cannot be complete.
[TEST] browser_467409-backslashplosion.js -
NS_ERROR_FAILURE on "nsISHistory.reloadCurrentEntry()" in the parent process.
[TEST] browser_586068-multi_window.js
JavaScript error: resource:///modules/sessionstore/SessionStore.jsm, line 1430: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHistory.reloadCurrentEntry]
See more details in https://phabricator.services.mozilla.com/D46281.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
•
|
||
I found some mochitest test failures are caused by wrong behavior of doing "sessionHistory.reloadCurrentEntry()" in the parent process.
"aLoadState->SHEntry()" is lost when sending aLoadState from parent to child.
I file Bug 1596776 for this problem.
Assignee | ||
Comment 13•4 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #11)
I found some mochitest test failures are caused by wrong behavior of doing "sessionHistory.reloadCurrentEntry()" in the parent process.
"aLoadState->SHEntry()" is lost when sending aLoadState from parent to child.
I file Bug 1596776 for this problem.
With current patch, there are some random ipdl errors when running the whole sessionStore tests.
However, it can reduce the number of permanent failures to 4.
browser/components/sessionstore/test/browser_447951.js
browser/components/sessionstore/test/browser_500328.js ---> File Bug 1599105
browser/components/sessionstore/test/browser_scrollPositions.js ---> bug 1596776 - Synchronize layouthistorystate to parent process
browser/components/sessionstore/test/browser_swapDocShells.js
Assignee | ||
Comment 14•4 years ago
|
||
After Bug 1583614, the pref "fission.rebuild_frameloaders_on_remoteness_change" sets to true.
When running sessionStore mochitest with fission.sessionHistoryInParent, we meet failures in the following two mochitests.
browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js
FAIL The expected number of notifications was received. - Got 4, expected 5
browser/components/sessionstore/test/browser_tab_label_during_restore.js
FAIL observed tab label changes - Got ["about:robots","New Tab","Gort! Klaatu barada nikto!"], expected ["about:robots","Gort! Klaatu barada nikto!"]
Assignee | ||
Comment 15•4 years ago
•
|
||
(In reply to Alphan Chen [:alchen] from comment #14)
After Bug 1583614, the pref "fission.rebuild_frameloaders_on_remoteness_change" sets to true.
When running sessionStore mochitest with fission.sessionHistoryInParent, we meet failures in the following two mochitests.browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js
FAIL The expected number of notifications was received. - Got 4, expected 5
In this case, reloadCurrentEntry() doesn't work as expected.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 17•4 years ago
|
||
Create the document for this implementation.
You can also add the comments in the following link.
https://docs.google.com/document/d/1L_Elw0pKzBpDaeBfNDk3D1dNmDWsFSgm0EveIddlvYc/edit?usp=sharing
Assignee | ||
Comment 18•4 years ago
|
||
I meet crash when running browser_586068-window_state_override.js based on the latest codebase.
Mozilla crash reason: MOZ_RELEASE_ASSERT(!aParam->IsDiscarded()) (Cannot send discarded BrowsingContext between processes!)
Updated•4 years ago
|
Updated•4 years ago
|
Comment 20•4 years ago
|
||
Tracking for Fission Nightly (M6) because Session Restore doesn't need to block Fission dogfooding (M5).
Comment 21•4 years ago
|
||
Pushed by ccoroiu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e51b4e883adc Make sessionRestore work with session history living in the parent process. r=peterv
Comment 22•4 years ago
|
||
Backed out changeset e51b4e883adc (Bug 1507287) for not having proper review
Comment 23•4 years ago
|
||
Backout by dvarga@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/05cda52b5ff9 Backed out changeset e51b4e883adc for not having proper review
Comment 24•4 years ago
|
||
Pushed by aiakab@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c6b9209306d Make sessionRestore work with session history living in the parent process. r=peterv,mikedeboer
Comment 25•4 years ago
|
||
Backed out changeset 4c6b9209306d (Bug 1507287) for causing geckoview failures CLOSED TREE
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=293394761&repo=autoland&lineNumber=7311
Backout: https://hg.mozilla.org/integration/autoland/rev/9ec693701ab01fb22ba14289d5342fcd8dac17f5
Assignee | ||
Comment 27•4 years ago
|
||
Only the changes in SessionHistory.jsm have effects on geckoview.
So I trigger a try only contain these changes. From it, we can find the same failure.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bdc0b7defbb0faf525ffc8411da306bce6e6fe6
Then I divided the patch into two parts.
(rewrite restore only) Do Find the same failure.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf26e8f85428d1290f614b9f08fc358e5c132add
(rewrite collect only) Don't find the failure as before.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1836e0697f881e297770c5369d9578aa1000bdc9
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e48722e45be25dcdf85f335abac4aef0da9e44d1
Right now I don't understand why the changes of restore rewriting would cause the problem on gv-junit test.
org.mozilla.geckoview.test.NavigationDelegateTest.processSwitching | org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutException: Timed out after 30000ms
I will keep investigating the symptom today
Assignee | ||
Comment 28•4 years ago
|
||
I found out the root cause of this failure.
Only geckoview uses the return value of SessionHistory.restore().
That's why I don't see any failure on the tests of firefox desktop.
Now I am re-running a full test.
Assignee | ||
Comment 29•4 years ago
|
||
Comment 30•4 years ago
|
||
Pushed by nerli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e90c2551e6d0 Make sessionRestore work with session history living in the parent process. r=peterv,mikedeboer
Comment 32•4 years ago
|
||
bugherder |
Comment 33•4 years ago
|
||
It is possible that this has caused us to save data to disk more often?
I'm seeing disk write spikes coming from firefox process every 15 seconds even if I don't do anything with Firefox.
Assignee | ||
Comment 34•4 years ago
•
|
||
No, I only change the behavior when pref(fission.sessionHistoryInParent) is on.
If pref is off, it works the same as before.
Assignee | ||
Comment 35•4 years ago
|
||
Update document.
Can add comments here.
https://docs.google.com/document/d/1vlTDfcqmLFqiOP3UNoW1HG-EE5TBSt3XXNE2-HtiAw0/edit?usp=sharing
Comment 36•4 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #35)
Created attachment 9137110 [details]
Document for new sessionHistory listener v1.1.pdfUpdate document.
Can add comments here.
https://docs.google.com/document/d/1vlTDfcqmLFqiOP3UNoW1HG-EE5TBSt3XXNE2-HtiAw0/edit?usp=sharing
This is great! I think we can also consider put contents into https://firefox-source-docs.mozilla.org/dom
Updated•3 years ago
|
Description
•