Closed Bug 1507287 Opened 2 years ago Closed 4 months ago

Make sessionRestore work with session history living in the parent process

Categories

(Firefox :: Session Restore, task, P2)

task

Tracking

()

RESOLVED FIXED
Firefox 76
Fission Milestone M6
Tracking Status
firefox76 --- fixed

People

(Reporter: alchen, Assigned: alchen)

References

(Blocks 9 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.
Blocks: 1474130
Blocks: ss-feature
Priority: -- → P3

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

Depends on: 1438272
Depends on: 1445459

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

Summary: Convert SessionHistory.jsm to C++ → Convert SessionHistoryListener to C++
No longer blocks: 1474130
Depends on: 1474130
No longer blocks: Session_managers
Fission Milestone: --- → ?
Assignee: nobody → alchen
Type: enhancement → task
Fission Milestone: ? → M4
Blocks: 1564412
No longer blocks: ss-SM
No longer blocks: ss-perf

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.

  • 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

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

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

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

Depends on: 1583568
Depends on: 1547734

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

Attached file Proposal-v1 (obsolete) —
Depends on: 1588489
Depends on: 1588491

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.

Status: NEW → ASSIGNED
Depends on: 1595900
Depends on: 1596077
Fission Milestone: M4 → M5
Depends on: 1596776

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.

P2 because Alphan is actively working on this bug.

Priority: P3 → P2

(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

Blocks: 1601503

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

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

Depends on: 1601773
Attachment #9093568 - Attachment description: [WIP] Bug 1507287-Rewrite sHistoryListener by using new SHistory API(in parent process) → Bug 1507287-Rewrite sHistoryListener by using new SHistory API(in parent process)
Blocks: 1602486
Blocks: 1602501
Blocks: 1567283
Duplicate of this bug: 1602811
Summary: Convert SessionHistoryListener to C++ → Make sessionRestore work with session history living in the parent process
Attachment #9093568 - Attachment description: Bug 1507287-Rewrite sHistoryListener by using new SHistory API(in parent process) → Bug 1507287-Make sessionRestore work with session history living in the parent process
Blocks: 1588119

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

Attachment #9099594 - Attachment is obsolete: true
Blocks: 1599105
Blocks: 1582091

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

Depends on: 1566559
Attachment #9093568 - Attachment description: Bug 1507287-Make sessionRestore work with session history living in the parent process → Bug 1507287 - Make sessionRestore work with session history living in the parent process. r?peterv
Attachment #9093568 - Attachment description: Bug 1507287 - Make sessionRestore work with session history living in the parent process. r?peterv → Bug 1507287 - Make sessionRestore work with session history living in the parent process. r=peterv
Duplicate of this bug: 1445459

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

Fission Milestone: M5 → M6
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

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

Backout by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05cda52b5ff9
Backed out changeset e51b4e883adc for not having proper review
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

I will check this failure on geckoview.

Flags: needinfo?(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

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.

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
Duplicate of this bug: 1582091
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 76
Regressions: 1623934

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.

Flags: needinfo?(alchen)

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

Flags: needinfo?(alchen)

(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

Duplicate of this bug: 1564412
Regressions: 1636926
You need to log in before you can comment on or make changes to this bug.