1534222 - LSNG: Check for maximum message size before calling SendCheckpoint()

Status: RESOLVED WORKSFORME

(Core :: Storage: localStorage & sessionStorage, defect, P2)

Description

[Jan Varga [:janv]]

See this crash:
https://crash-stats.mozilla.org/report/index/7c83afc2-0f17-44b3-92f6-792bd0190311

Comment 1

[Neha Kochar [:neha]]

Jan, could you please provide an estimate timeline for the patch?

Comment 2

[Jan Varga [:janv]]

(In reply to Neha Kochar [:neha] from comment #1)

Jan, could you please provide an estimate timeline for the patch?

Couple of days, likely next Monday.

Comment 3

[Jan Varga [:janv]]

There's still the 5 MB limit, but a page can cause a lot of local storage changes during execution of a JS function.
The limit is not reached in the end, but all the changes can create a huge "database log".

For example, this loop will cause the crash (max message size is 256MB):
for (var i = 0; i < 26; i++) {
randomlyChange(largeAlmost5MegaByteBuffer);
localStorage.setItem("foo", largeAlmost5MegaByteBuffer);
}

Andrew, here are options for fixing this bug, we need to decide which approach to take:

1. Use the WriteOptimizer on the child side too
   This would decrease memory requirements I believe, but so far we always distributed all storage events across content processes. This could regress some web sites.

2. Use data compression
   This would also decrease memory requirements, but it's not trivial to implement and we need to watch for max message size anyway.

3. Support sending partial checkpoints
   The child would have to create a new separate write info array, once the size of existing array is close to max message size.
   Then when the stable state callback is called, we call SendCheckpoint multiple times and the last one would signalize that it's the last message. Parent would have allocate separate arrays too and apply changes only when everything has been received.
   The downside is that this can temporarily cause huge memory allocations if a page does something bad intentionally.
   The upside is that snapshots would be still consistent as we originally planned.

4. Send checkpoint when the size of write info array is close to max message size
   We would just track the size of current write info array and call SendCheckpoint when the size is close to max message size, we wouldn't wait for the stable state callback.
   The downside is that while the existing snapshot would still see the original state, any new snapshots created in other content processes would see "partial" changes from the original content process.
   The upside is that this doesn't need a lot of memory.

If you know about any other options, let me know.

Comment 4

[Andrew Overholt [:overholt]]

(LSNG isn't shipping in 67 so updating the tracking flags accordingly)

Comment 5

[Jan Varga [:janv]]

We still need to check the max message size, but fixing bug 1546310 and bug 1513937 may mitigate this issue.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

I think using the WriteOptimizer in the client is the right way to go.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Are we still planning to land a max-message-size test in 68?

Comment 8

[Jan Varga [:janv]]

No, we're going to implement a WriteOptimizer that will coalesce operations, so final IPC message would never reach the IPC maximum message size. That should happen right after bug 1540777.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

We were looking at this in Regression Triage today; are we still planning on fixing this in 68 (as it's a P1)? Thanks!

Comment 10

[Jan Varga [:janv]]

Yes.

Comment 11

[Jan Varga [:janv]]

Andrew, I'm afraid that we can't use write optimizer in the client, take a look at this test:
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webstorage/event_local_removeitem.html

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

As per the discussion in a vidyo call, I think only do the write optimizer for IPC purposes is a workable simplification. Behavior within the same process would be the same, so that test would still pass. And as we start doing origin-per-process, it's less likely that a site would perceive dubious behavior.

Alternately, we could allow some level of redundancy... but I think it's clear that if an origin is generating 256MB of deltas that it's appropriate to engage in some type of https://github.com/WICG/interventions.

Comment 13

[Jan Varga [:janv]]

Yeah, but it will take some time to have origin-per-process. Andrew also mentioned that parent could tell the child if there are cross-process listeners at the time the snapshot is created. If there are no listeners we can safely use the write optimizer on the child side too. I think that would be a good compromise since the chance of having two or more tabs open for the same origin and having a cross-process listener for the storage event and having a function that produces a lot of changes is not very high.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix in 68 but we could still likely take a patch for 69.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Dropping the issue from regression triage at this point; it has a priority set and the team is aware of the issue.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.