Open Bug 1507287 Opened 11 months ago Updated 7 days ago

Convert SessionHistoryListener to C++

Categories

(Firefox :: Session Restore, task, P3)

task

Tracking

()

Fission Milestone M4

People

(Reporter: alchen, Assigned: alchen)

References

(Depends on 3 open bugs, Blocks 4 open bugs)

Details

Attachments

(2 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
You need to log in before you can comment on or make changes to this bug.