1654080 - Move SessionStorage to parent process

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, enhancement, P1)

Description

[Neha Kochar [:neha]]

The initial plan was to make SessionStorage persistent (bug 1445464) for Fission but because that seems to be blocked on QuotaManager v4, we could do an intermediate step of moving SessionStorage state in parent which can be fetched over IPC.

Comment 1

[Tom Tung [:tt, :ttung]]

Take this from Eden since he is on PTO.

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug 1654080 - P1 PSessionStorageCache IPC protocol for moving SessionStorage to parent process

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1654080 - P2 Integrating PSessionStorageCache into SessionStorage implementation

Depends on D87197

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Bug 1654080 - P3 Removing SessionStorageService implementation

Depends on D87198

Comment 5

[Tom Tung [:tt, :ttung]]

Spoke to Eden and I put my understanding below:

The issue here is that SessionHistory is going to move to the parent process and we need to provide a way for SessionStore to collect on the parent process.

SessionRestore now collects SessionStorage data for documents on the content process through a docshell tree. However, when it comes to a cross-origin iframe/docshell, SessionRestore cannot find the cross-origin/cross-process SessionStorage data properly.

Comment 6

[Tom Tung [:tt, :ttung]]

The pref for testing SessionHistory in the parent process is fission.sessionHistoryInParent.

Comment 7

[Tom Tung [:tt, :ttung]]

The remaining work here is to move the "collect" operation in SessionRestore to the parent process and check the failure tests on try.

Comment 8

[Tom Tung [:tt, :ttung]]

try (without enabling the pref): https://treeherder.mozilla.org/#/jobs?repo=try&revision=10dade5427ae27833db3e638d30d2858e5f6a432

I will try to fix the existing test failures.
Then, polish patches.
Last, collect the SessionStroage on the parent process and push patches to try with enabling the pref.

Comment 9

[Tom Tung [:tt, :ttung]]

It seems that it's related to/blocked bug 1572084

Comment 10

[Tom Tung [:tt, :ttung]]

try with removing the code for deleting child actors from parent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a96cce7d77e96b779d1291acfa0e24cbbec1e09&selectedTaskRun=fwqTMYtKRRe8zV0vAZnuCg.0

Comment 11

[Tom Tung [:tt, :ttung]]

Have a meeting with Nika and Yaron today.

So, the main things that Nika was worried about actually are fixed by Yaron's previous work (https://bugzilla.mozilla.org/show_bug.cgi?id=1593246)
We still want to have a test to verify it and it should still be done in M6b.

The main work here that sync SessionStorage to the parent process for each operation or each a few seconds can be done in M6c rather than M6b

Comment 12

[Tom Tung [:tt, :ttung]]

I have separated the work that should be done before M6b out to bug 1660485, so here is M6c.
And Yaron will work on it :)

Comment 13

[Tom Tung [:tt, :ttung]]

Take this back from Yaron

Comment 14

[Tom Tung [:tt, :ttung]]

I have a draft that sync SessionStorageCache's write operations (SetItem, RemoveItem, and Clear) to the parent process per stable state asynchronous. So that the parent process has copies of SessionStorageCaches. It uses LSValue, and LSWriteOptimizer so that the size of IPC messages should be reduced.

I will update the patch once I polish the patch.

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1654080 - Sync SessionStorageCache to the parent process per write operation;

Comment 16

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1654080 - Make SessionStorage sync less often by synchronizing SessionStorageCache at the next stable state;

Depends on D89340

Comment 17

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1654080 - Use PBackground for syncing SessionStorageCache and use LSWriteOptimizer to send data changes;

Depends on D89341

Comment 18

[Tom Tung [:tt, :ttung]]

D87197, D87198, and D87199 can achieve moving SessioinStorage to the parent process. However, we can reduce the changes by reusing the current code for SessionStorage and reduce the size of IPC messages by LSValue and LSWriteOptimizer.

I'm going to abandon D87197, D87198, and D87199.

Comment 19

[Jan Varga [:janv]]

Before we start reviewing the patches, could you provide a high level description of the approach ?
It doesn't have to be long, just a few sentences, like what top level protocol will be used for communication, what will be re-used from LSNG, etc.

Comment 20

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #19)

Before we start reviewing the patches, could you provide a high level description of the approach ?
It doesn't have to be long, just a few sentences, like what top level protocol will be used for communication, what will be re-used from LSNG, etc.

Sure!

Goal:
Move SessionStorage to parent process because SessionStore/SessionHistory will need to access SessionStorage data for any origin on the parent process during runtime.

What we have right now:

• All sessionStorage data is sent from a content process to the parent process when the content process is shutting down
• SessionStorage data is sent from the parent process to content when the corresponding content process is re-created

Changes here:

• D89340: SessionStorage data for an origin is sent from a content process to the parent process per write operation (SetItem, RemoveItem, and Clear)
• D89341: Similar to LSSnapshot, accumulate SessionStorage data for an origin and send them to the parent process in a stable state.
• D89342: Use LSWriteOptimizer and LSValue to coalesce SessionStorage data. So that the IPC message size can be reduced.

Overall/after applying current changes:

• Once there is a write operation for SessionStorage, SessionStorage will accumulate the wirteInfos until the next stable state and then send them together to the parent process.
• While a current process is shutting down, SessionStorage doesn't need to send all its session storage to the parent process since we should have had a snapshot of SessionStorageCaches for the content process in the parent process.

what top level protocol will be used for communication

The IPC protocol is still the same. (PContent).
However, the content of the IPC message sent from content process to the parent process is changed to SSWriteInfo.

what will be re-used from LSNG

LSValue, and LSWriteOptimizer are re-used.
The concept of LSSnapshot is re-used as well, but the functionalities are divided into SessionStorageSyncHelper and SeesionStorageCache.

Comment 21

[Tom Tung [:tt, :ttung]]

I will update the comment in SessionStorageManager once the changes are finalized (the patches are reviewed)

Comment 22

[Jan Varga [:janv]]

So, it's almost like LSNG regarding communication between parent/child, except:

• PContent is used instead of PBackground
• All data is sent from parent to content at once (during process creation) instead of using a snapshot which would load data on demand.

I'll take a closer look, but it looks this could be a good compromise for now.

Comment 23

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #22)

So, it's almost like LSNG regarding communication between parent/child, except:

• PContent is used instead of PBackground
• All data is sent from parent to content at once (during process creation) instead of using a snapshot which would load data on demand.

I'll take a closer look, but it looks this could be a good compromise for now.

Right, and thank you!

Comment 24

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1654080 - Remove unsed SessionStorageService;

Depends on D89342

Comment 25

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0f2703149bd714c3801bce91a1848ef51a1ad64

Comment 26

[Jan Varga [:janv]]

Attachment: impl.diff

Comment 27

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cd4ab2c0af72e3ab87d388e3e9903a034ea4505

Comment 28

[Tom Tung [:tt, :ttung]]

lastest try and it looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61c6b1c638fad3b1991327413fa0e7e39c94f03e
I will land the patches after getting r+ from all reviewers .

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eccd53004a47
Sync SessionStorageCache to the parent process per write operation; r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/e04b853cf5d2
Make SessionStorage sync less often by synchronizing SessionStorageCache at the next stable state; r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/44541aaaa9f0
Use PBackground for syncing SessionStorageCache and use LSWriteOptimizer to send data changes; r=dom-workers-and-storage-reviewers,janv,nika

Comment 31

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/eccd53004a47
https://hg.mozilla.org/mozilla-central/rev/e04b853cf5d2
https://hg.mozilla.org/mozilla-central/rev/44541aaaa9f0