Remove localstorage limit

[ccorcos]

• Electron Version: 2.0.2
• Operating System (Platform and Version): MacOS
• Last known working Electron version: 1.8.1 (never had this issue before in an older version of the app)

Sorry for the extremely vague bug report, but I found a serious bug in our app where localStorage seems to have stopped working without throwing any errors causing using users to lose data.

localStorage.setItem("x", "10")
> undefined
localStorage.getItem("x")
> undefined

Upon deleting the Application Support files, everything started working again. Is it possible that we are not raising an error in some cases when localStorage doesn't work? Perhaps the data has been corrupted or something?

I apologize that I don't have reproducible steps, but I think its worth documenting in case others have a similar issue.

[ccorcos]

Alright, I have a repro!

With the Electron 2.0.2 application, from the dev tools:

localStorage.length
> 635
for (let i = 0; i < 10000; i++) { localStorage.setItem(i.toString(), Array(1000).fill(1).toString()) }
> undefined
localStorage.length
> 5630
for (let i = 10000; i < 20000; i++) { localStorage.setItem(i.toString(), Array(1000).fill(1).toString()) }
> undefined
localStorage.length
> 5630

I when I try to add more, it doesn't work and there are no errors!

In Electron 1.8.1 application, from the dev tools:

localStorage.length
> 359
for (let i = 0; i < 10000; i++) { localStorage.setItem(i.toString(), Array(1000).fill(1).toString()) }
> undefined
localStorage.length
> 10359
for (let i = 10000; i < 20000; i++) { localStorage.setItem(i.toString(), Array(1000).fill(1).toString()) }
> undefined
localStorage.length
> 20008
for (let i = 0; i < 800000; i++) { localStorage.setItem(i.toString(), Array(1000).fill(1).toString()) }
> undefined
localStorage.length
> 800008

Now, I'm not sure why we appear to have lost some entries -- its possible that the application itself removed some stuff -- but for the most part, it worked. It took about 2 minutes to add the 800000 entries, so its definitely working.

Sounds like a bug to me!

[bpasero]

@ccorcos does it reproduce if you run this early before app.on("ready"):

app.commandLine.appendSwitch('disable-mojo-local-storage');

[ccorcos]

localStorage.length
> 155
for (let i = 0; i < 10000; i++) { localStorage.setItem(i.toString(), Array(1000).fill(1).toString()) }
> undefined
localStorage.length
> 10155
for (let i = 10000; i < 20000; i++) { localStorage.setItem(i.toString(), Array(1000).fill(1).toString()) }
> undefined
localStorage.length
> 20155
for (let i = 0; i < 800000; i++) { localStorage.setItem(i.toString(), Array(1000).fill(1).toString()) }
> undefined
localStorage.length
> 800009

Seems to be the culprit!

[bpasero]

This looks like an issue with localStorage using levelDB then (which can be turned off using that command line switch). @ccorcos could you please try to reproduce your original issue with Electron 3.0 beta?

[ccorcos]

Hey @bpasero would you mind trying? 3.0.0-beta.1 keeps crashing :/

[bpasero]

@ccorcos I think this still reproduces in Electron 3.0.0-beta.1. Output:

[ccorcos]

Hmm yeah, that's no good!

[ccorcos]

Just tried this in Electron 3.0.9 and its still an issue and app.commandLine.appendSwitch('disable-mojo-local-storage'); does not fix the problem either. Any ideas @bpasero?

[ccorcos]

@deepak1556 any idea what happened to this command line option to turn off mojo?

[bpasero]

@ccorcos disable-mojo-local-storage will not work anymore starting with Electron 3.0.x due to the Chrome version used within.

[deepak1556]

Seems like the localstorage limit is not completely removed, during commit operation the size constraint is still maintained at 5MB.

[SwiTool]

I checked that electron v4.0.4 is ok

Sorry but you failed, Array(1000).fill(1)/toString() gives NaN as Array(1000).fill(1).toString() gives 1,1,1,1,1 1000 times.
I still have the issue :

Using electron 4.0.6

[richjun]

I checked that electron v4.0.4 is ok

Sorry but you failed, Array(1000).fill(1)/toString() gives NaN as Array(1000).fill(1).toString() gives 1,1,1,1,1 1000 times.
I still have the issue :

Using electron 4.0.6

You're right.
It's my mistake.

[jacobq]

I think the test code used above is misleading. (In fact, so is the test I originally contributed). I found that within a script I can call setItem and getItem back-to-back for as large of a size as I like (provided there is enough memory and the electron version being used includes #15596). e.g.

localStorage.setItem('junk', new Array(50*1024*1024/2).fill('x').join('')); console.log(localStorage.getItem('junk').length)

However, the data isn't necessarily persisted. If the getItem isn't run back-to-back (in the same closure / context / whatever) -- e.g. if it's run from the dev tools console after setItem finishes -- then it will return null or "".

As a more thorough test, I've forked the quick start app and scripted it to run a series of tests separated by setTimeout. You can view the code at jacobq/electron-quick-start#storageLimit.

For current versions (5.0.1 and 6.0.0-beta.4) the limit appears to be about 21176304 bytes (i.e. 20MiB + about 1% grace).

[ccorcos]

Hmm. That's weird because localStorage is supposed to be synchronous

[jacobq]

Hmm. That's weird because localStorage is supposed to be synchronous

I'd have to review the spec again, but I think it's synchronous from the perspective of the script. That is, calling setItem then getItem must give back the new value. However, I don't think the underlying structures are required to be, so if something goes wrong (e.g. disk full, limit exceeded, etc.) then future calls to getItem may differ.

[jacobq]

Maybe it got deleted now, but I got an email saying that this issue appeared to be out-of-date / only applicable to old versions of Electron. This is not the case. I just rebased my test repo (see storageLimit-20190809 branch) against electron-quick-start and made sure that it uses current versions of all dependencies. A quick test of Electron v6.0.1 & 7.0.0-beta.2 on linux x86_64 shows that there is still a problem (fails write-delay-read test at approximately 20679.986 KiB).

[jacobq]

I started looking into this again and think I found a clue as to what is going wrong with the current patch. The dom_storage_limits patch only disables enforcement of the quota. Unfortunately, these parameters (e.g. kPerStorageAreaQuota) are still used for allocating memory, e.g. here. One can easily test this by raising or lowering kPerStorageAreaQuota and observing the behavior of the test method I mentioned above.

Update 1: Actually, the value passed to StorageAreaMap is only used for quota enforcement, so that's OK. However, I did find another place where the size quota is enforced (not fixed by the patch): it's in StorageAreaImpl::Put. This could explain why data appears OK at first (cache) but not later (from persistent storage); I'm checking it out now.

Update 2: Circumventing the quota check in StorageAreaImpl::Put indeed resolves the problem I described above. However, so doing still does not allow for arbitrarily sized DOM storage operations. For example, I found that attempting to do a big transaction like localStorage.setItem('Y', 'X'.repeat(256 * 1024 * 1024)) from the Dev Tools console caused a crash due to exceeding kMaximumMessageSize = 128MiB. Hence I think that rather than continue to attempt to bypass the 10MiB storage quota altogether we should simply raise it to 100MiB since higher values may be problematic (regarding stability alone, not to mention performance).