1474130 - Convert content-sessionStore to C++ - stage I

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement, P2)

Description

[Kris Maglione [:kmag]]

content-sessionStore.js is currently loaded into every tab frameloader. Which means it gets loaded multiple times per process, which is not great. But even when loaded only once, it uses about 86K. Add to that 17K from ContentRestore.jsm and 12K from SessionHistory.jsm, and we're up to at least 120K per process, if none of the other helper JSMs get loaded.

The things that these scripts do can easily be done by C++ (some of them more easily), so there doesn't seem to be a good justification for loading this much JS into every process for the sake of session restore.

Comment 1

[Kris Maglione [:kmag]]

Ah, and of course another 12K for Utils.jsm.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

I'm probably going to be working on this a bit for fission in the coming weeks, so self-assigning.

Comment 3

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

I'd say this is also [qf] work. Session store shows up rather often in the performance profiles.

Comment 4

[Robert AB]

Bug 1474130 might end up blocking much work here, but for now bug 1378651 and bug 1364019 are possibly reasonable to do.  There are other priorities in the way.
[Source: https://bugzilla.mozilla.org/show_bug.cgi?id=1413525#c55]

Comment 5

[Robert AB]

Fixing bug 1474130 is needed before the work on session management API (bug 1427928) can be continued:
https://bugzilla.mozilla.org/show_bug.cgi?id=1427928#c48

Comment 6

[Robert AB]

(In reply to Robert Ab from comment #5)
> Fixing bug 1474130 is needed before the work on session management API (bug
> 1427928) can be continued:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1427928#c48

Do you know when this bug will be fixed?

Comment 7

[Alphan Chen [:alchen]]

I will take care of this bug.

Comment 8

[Alphan Chen [:alchen]]

Before starting the rewriting, I would like to rewrite the js modules which are used in ContentSessionStore.jsm.

Bug 1497144: DocShellCapabilities.jsm and ScrollPosition.jsm
Bug 1497146: FormData.jsm
Bug 1497147: Utils.jsm (rewrite mapFrameTree)

Comment 9

[Alphan Chen [:alchen]]

Attachment: Bug 1474130 - Rewrite DocShellCapabilities.jsm and ScrollPosition.jsm into C++

Get rid of DocShellCapabilities.jsm and ScrollPosition.jsm

Comment 10

[Alphan Chen [:alchen]]

In this rewriting, we will replace the communication between content and parent from message passing to IPC.

Comment 11

[Alphan Chen [:alchen]]

Attachment: Proposal-bug1474130.pdf

The attached file is the proposal for this rewriting.

There are several stages:

[Stage I] Rewrite several listeners into C++ and using IPDL to communicate from the content process to the parent process. For compatibility, we will construct the original data format(JSON format) in the parent process.

[Stage II] After we make sure the compatibility and stability of the stage I mechanism. In stage II, rewrite sessionStorageListener by using the same mechanism.

[Stage III] Rewrite sessionHistorListener if needed. (related bugs: Bug 1438272, Bug 1445459, Bug 1507287)

After "stage III", the rest of contentSessionStore.jsm is "restore" functionality. We will re-evaluate the rewriting again. Currently, "ContentRestore.jsm" is loaded lazily(bug 1474131).

Comment 12

[Alphan Chen [:alchen]]

Attachment: Bug 1474130 - Implement ScrollPosition/Privacy/DocCapability listeners in C++

Comment 13

[Alphan Chen [:alchen]]

In the current patch, I add three functions in the "nsIXULBrowserWindow.idl".
The three functions are implemented in "browser/base/content/browser.js".
The function "updateSessionStore()" will call SessionStore.updateSessionStoreFromCPP() with "tab" and "data".

Currently, I cannot update TabState correctly. The tab I get seems not the right one.
@TabParent.cpp
mozilla::ipc::IPCResult TabParent::RecvSessionStoreUpdate()
browser = mFrameElement->AsBrowser
@browser/base/content/browser.js
updateSessionStore: function XWB_updateSessionStore(aBrowser)
tab = gBrowser.getTabForBrowser(aBrowser)
@sessionStore.jsm
updateSessionStoreFromCPP(aTab, aData) {
let browser = aTab.linkedBrowser;
let win = browser.ownerGlobal;
TabState.update(browser, aData);
this.saveStateDelayed(win);
}

Originally, the sessionStore get "browser" from "aMessage.target".
https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/browser/components/sessionstore/SessionStore.jsm#829

receiveMessage(aMessage) {
// If we got here, that means we're dealing with a frame message
// manager message, so the target will be a <xul:browser>.
var browser = aMessage.target;
let win = browser.ownerGlobal;
....
switch (aMessage.name) {
case "SessionStore:update":
....
TabState.update(browser, aMessage.data);
this.saveStateDelayed(win);

Comment 14

[Alphan Chen [:alchen]]

Now investigating how it works on TabParent::ReceiveMessage().

Comment 15

[Alphan Chen [:alchen]]

The current patch is failed on "browser_privatetab.js".
In the test, it creates a new window and attaches a frame script.
Then create a new tab in the new window that will load the frame script.
https://searchfox.org/mozilla-central/rev/4763b8d576ce52625d245d1ab6d9404ea025b026/browser/components/sessionstore/test/browser_privatetabs.js#62

After "BrowserTestUtils.addTab", TabChid observe a "privateModeChanged" event and do an IPC to parent for updating the TabState. However, the tab by gBrowser.getTabForBrowser(aBrowser) is null.

Comment 16

[Alphan Chen [:alchen]]

In current implementation, I cannot observe the behavior of adding a new tab from tabBrowser.
Need to find another way to add "PrivacyTransitionObserver".

Comment 17

[Alphan Chen [:alchen]]

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

In current implementation, I cannot observe the behavior of adding a new tab from tabBrowser.
Need to find another way to add "PrivacyTransitionObserver".
In the end, it is caused by opening a "about:mozilla" tab.
We loaded it in the parent.

Comment 18

[Alphan Chen [:alchen]]

For the case of opening "about:mozilla" tab, we find a way to add the listener.
By this way, we can observe "PrivateModeChanged" and collect the value correctly.
However, I don't get the notification for the WebProgress state change.

Also, we need to observe the following events in our listener.

• Input event for formdata
• Mozvisualscroll event for scroll position
• MozSessionStorageChanged for SessionStorage

Comment 19

[Alphan Chen [:alchen]]

In the latest revision, three listeners are implemented with all tests pass.

• ScrollPosition/Privacy/DocCapability listeners

However, JSM implementation has a force update mechanism to update the value quickly.
The mochitest will use it before checking the Scroll position.
/*
async function checkScroll(tab, expected, msg) {
let browser = tab.linkedBrowser;
await TabStateFlusher.flush(browser);
...
*/

Ideally, we should also flush the changes from C++ listener at the same time.
Need to think about how to do it.

[Other possibilities]

1. Introduce another force update mechanism for our C++ implementation.
2. Use the existing pref for sessionStore, set it as false before doing the test.
   // This pref controls whether or not we send updates to the parent on a timeout
   // or not, and should only be used for tests or debugging.
   const TIMEOUT_DISABLED_PREF = "browser.sessionstore.debug.no_auto_updates";

Comment 20

[Jan Henning [:JanH]]

2. Use the existing pref for sessionStore, set it as false before doing the test.

a) Wouldn't that be inherently racy?
b) I suspect that there could be scenarios where we'd want to flush the state outside of tests, too.

Comment 21

[Alphan Chen [:alchen]]

(In reply to Jan Henning [:JanH] from comment #20)

2. Use the existing pref for sessionStore, set it as false before doing the test.

a) Wouldn't that be inherently racy?
b) I suspect that there could be scenarios where we'd want to flush the state outside of tests, too.

In the latest revision, we have added support for the flush function.

Try result with Current patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f653f4580d462eadb3b6074757468ac115407f18
There are two testcases in which we observed leakage of 1 docshell.
devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_devtoolstoolbox_focus.js | leaked 1 docShell(s) until shutdown
browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | leaked 1 docShell(s) until shutdown

If we don't add systemEventListener in parent only case, the leakage will disappear.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaeb4be71e3fe3f17b897d0cc5a7870021ffa65c

Comment 22

[Alphan Chen [:alchen]]

Rename this bug as "Convert content-sessionStore to C++ - Stage I"

There are some new mechanisms introduced in the current rewriting.
I would like to evaluate those mechanisms with three native listeners: PrivacyListener, DocShellCapabilitiesListener and ScrollPositionListener.

Stage II includes FormDataListener and SessionStorageListener rewriting.
Stage III will be the rewriting for SessionHistoryListener.

Comment 23

[Alphan Chen [:alchen]]

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

Stage II includes FormDataListener and SessionStorageListener rewriting.
Bug 1544371
Stage III will be the rewriting for SessionHistoryListener.
Bug 1507287

Comment 24

[Alphan Chen [:alchen]]

Try result with the latest revision.
It looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e7551b304bafc1e90c1da565ac1a5b910a05d76

Comment 25

[Alphan Chen [:alchen]]

Try is fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a4374a0e43f4bacede7e7d0f79c4082bd1730c4

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3288c43195a2
Implement ScrollPosition/Privacy/DocCapability listeners in C++ r=peterv

Comment 27

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3288c43195a2

Comment 28

[Robert AB]

Is any reason why content-sessionStore is converted to C++, but not to Rust?

Comment 29

[Alphan Chen [:alchen]]

(In reply to Robert Ab from comment #28)

Is any reason why content-sessionStore is converted to C++, but not to Rust?

In my opinion, there is no obvious advantage to do this rewriting in RUST.
Besides, it is not an independent module.
We need lots of interactions(with dom events/IPC communications...) in this rewriting.
Using C++ here is suitable.

Comment 30

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Hi! Looks like the following improvements came with your patch:
== Change summary for alert #20840 (as of Tue, 07 May 2019 12:27:35 GMT) ==

Improvements:

1% Base Content JS windows7-32-shippable opt 3,255,557.33 -> 3,224,362.67
0.26% Base Content JS windows10-64-shippable opt 4,142,121.33 -> 4,131,218.67
0.26% Base Content JS windows10-64-shippable-qr opt 4,142,052.00 -> 4,131,280.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20840