1507287 - Make sessionRestore work with session history living in the parent process

Status: RESOLVED FIXED

(Firefox :: Session Restore, task, P2)

Description

[Alphan Chen [:alchen]]

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.

Comment 1

[Alphan Chen [:alchen]]

Since session history will move into the parent process, this bug will be held and wait for bug 1438272 and bug 1445459.

Comment 2

[Alphan Chen [:alchen]]

Rename the bug as "Convert SessionHistoryListener to C++".
This bug will be served as the stage III of ContentSessionStore rewriting.

Comment 3

[Peter Van der Beken [:peterv]]

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.

Comment 4

[Alphan Chen [:alchen]]

Attachment: Bug 1507287 - Make sessionRestore work with session history living in the parent process. r=peterv

• Move StateChangeNotifier/SessionHistoryListener into SessionHistoryListener.jsm
• Implement sHistoryListener
  = (in C++) collect shistory when get "DOMTitleChanged" event, OnDocumentStart and OnDocumentEnd.
  = (in JS)
  1. AddSHistoryListener in parent process.
     (send "SessionStore:AddSHistoryListener" from child to parent)
  2. Store sHistoryListener into a new WeakMap()
     ("this._browserSHistoryListener" : Use browser.permanentKey as the key)
  3. Add uninstall() to remove the listener from BC
  4. Add notifySHistoryChanges() to notify SessionStoreListener about the changes
• Add collectFromParent() in SessionHistory.jsm

Comment 5

[Laurențiu Nicola]

// 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?

Comment 6

[Alphan Chen [:alchen]]

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);
....
}

Comment 7

[Alphan Chen [:alchen]]

(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.

Comment 8

[Alphan Chen [:alchen]]

After adding the restore part, I meet crash when doing restore().

Reproduce Step:

1. Open a random page. (e.g. https://en.wikipedia.org/wiki/Wiki)
2. 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

Comment 9

[Alphan Chen [:alchen]]

Attachment: Proposal-v1

Comment 10

[Alphan Chen [:alchen]]

Update current status,

Now there are two symptoms:

1. 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

2. 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.

Comment 11

[Alphan Chen [:alchen]]

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.

Comment 12

[Chris Peterson [:cpeterson]]

P2 because Alphan is actively working on this bug.

Comment 13

[Alphan Chen [:alchen]]

(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

Comment 14

[Alphan Chen [:alchen]]

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!"]

Comment 15

[Alphan Chen [:alchen]]

(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.

Comment 17

[Alphan Chen [:alchen]]

Attachment: Document for new sessionHistory listener v1.0.pdf

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

Comment 18

[Alphan Chen [:alchen]]

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!)

Comment 20

[Chris Peterson [:cpeterson]]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e51b4e883adc
Make sessionRestore work with session history living in the parent process. r=peterv

Comment 22

[Daniel Varga [:dvarga]]

Backed out changeset e51b4e883adc (Bug 1507287) for not having proper review

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05cda52b5ff9
Backed out changeset e51b4e883adc for not having proper review

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c6b9209306d
Make sessionRestore work with session history living in the parent process. r=peterv,mikedeboer

Comment 25

[Noemi Erli[:noemi_erli]]

Backed out changeset 4c6b9209306d (Bug 1507287) for causing geckoview failures CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=293394761&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&searchStr=5a673baa108368237aedf3c44025791ec3840b6c&revision=4c6b9209306d7e1b1db070492e1e705b7a2b4d0d

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

Comment 26

[Alphan Chen [:alchen]]

I will check this failure on geckoview.

Comment 27

[Alphan Chen [:alchen]]

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

Comment 28

[Alphan Chen [:alchen]]

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.

Comment 29

[Alphan Chen [:alchen]]

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a341b3aef1c6c41f83ff4219eae2b3611040ff60

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e90c2551e6d0
Make sessionRestore work with session history living in the parent process. r=peterv,mikedeboer

Comment 32

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/e90c2551e6d0

Comment 33

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

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.

Comment 34

[Alphan Chen [:alchen]]

No, I only change the behavior when pref(fission.sessionHistoryInParent) is on.
If pref is off, it works the same as before.

Comment 35

[Alphan Chen [:alchen]]

Attachment: Document for new sessionHistory listener v1.1.pdf

Update document.

Can add comments here.
https://docs.google.com/document/d/1vlTDfcqmLFqiOP3UNoW1HG-EE5TBSt3XXNE2-HtiAw0/edit?usp=sharing

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alphan Chen [:alchen] from comment #35)

Created attachment 9137110 [details]
Document for new sessionHistory listener v1.1.pdf

Update 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