776416 - Remove exceptions to 5MB quota rule in localStorage

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

While implementing bug 773373 I noticed that the localStorage code is quite complicated by the fact that we have various exceptions to the 5MB quota limit in localStorage.

First of all we grant 200MB of data for any page which has offline storage enabled.

Second, there was some tie-in with pages which are allowed to use the "persist" feature in *XUL*.

This made sense back when localStorage was the only storage mechanism we had. And before we realized the really bad performance problems we have due to the synchronous IO. But now we have IndexedDB as well as more experience with the problems that the synchronous IO is causing.

And XUL support has been turned off by default, so definitely no need to worry about that.

So let's limit localStorage to 5MB everywhere and remove any integration with offline apps.

As an added bonus, this makes the localStorage code significantly simpler.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 644806 [details] [diff] [review]
Patch to fix

So, with this patch, when I delete cookies using e.g. the Clear Recent History dialog, will also data stored in localStorage by offline-enabled app (origin) be deleted along with cookies?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes. The idea was to completely separate localStorage from offline apps and instead make it behave just like cookies.

Right now if you clear data for a specific domain we will clear the cookies for that domain, no matter if offline is enabled for the domain or not. So it seems ok to me to clear localStorage data as well.

Long term we should merge the offline storage policies with the indexedDB storage policies. Bug 742822 is laying the foundation for some of that.

Comment 4

[Honza Bambas (:mayhemer)]

Jonas, I'll take a look on the plans prior to review.

Until there is a mechanism to preserve localStorage data for offline-enabled domain, I am strongly against your patch.

Please also check this blog post, mainly the "(Not only) user’s data saved by apps" section: http://www.janbambas.cz/offline-application-cache-in-firefox/

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why is saving localStorage for offline a prerequisite? We already have indexedDB which won't be deleted when cookie data is deleted. We really want people to use indexedDB instead of localStorage due to the performance issues with localStorage.

Comment 6

[Honza Bambas (:mayhemer)]

Yes, we want, but at the moment there could be applications that use localStorage and store locally important data.  And we may delete those data just by deleting cookies.  I really cannot accept that.

Comment 7

[Honza Bambas (:mayhemer)]

On the hand, as I thinking of this more, *if this is well communicated*, it really can be a simplification.

I'll do the review of the patch tomorrow.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 644806 [details] [diff] [review]
Patch to fix

reverting the r flag (actually, r- was accidental)

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #0)
> And XUL support has been turned off by default, so definitely no need to
> worry about that.

In what bug(s)?

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 644806 [details] [diff] [review]
Patch to fix

Review of attachment 644806 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab.  I didn't test my self this time.

Sorry for the delay.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Remote xul was disabled in bug 546857

Thanks for the review!!

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Updated to tip

Forwarding review flag.

Comment 13

[Jan Varga [:janv]]

Attachment: Fix remaining tests

Comment 14

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd824db5b0df
https://hg.mozilla.org/integration/mozilla-inbound/rev/e564016b9de9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd824db5b0df
https://hg.mozilla.org/mozilla-central/rev/e564016b9de9

Comment 16

[Philip Chee]

You removed clearOfflineApps from the IDL but you didn't remove any of the callers.

http://mxr.mozilla.org/comm-central/search?string=.clearOfflineApps

Comment 17

[Jan Varga [:janv]]

will do that, sorry

Comment 18

[Jan Varga [:janv]]

Attachment: Remove remaining clearOfflineApps() callers

Comment 19

[Jan Varga [:janv]]

(In reply to Jonas Sicking (:sicking) from comment #0)
> So let's limit localStorage to 5MB everywhere and remove any integration
> with offline apps.
> 

I'll post additional patch to fix remaining offline integration bits.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Since nothing has been backed out, and there are fixes landed on m-i for the remaining problem I'm going to put this back into FIXED state

Comment 21

[pal-moz]

about:newtab (new tab page) is broken.

http://forums.mozillazine.org/viewtopic.php?p=12288671#p12288671

Comment 22

[Jan Varga [:janv]]

I think the problem is that new tab page stores some data (e.g. pinned links) in local storage and it used to be stored in chromeappsstore.sqlite before this bug.
Now it is stored in webappsstore.sqlite.
So essentially the data is lost :(

Comment 23

[Jan Varga [:janv]]

We should probably copy data from chromeappsstore.sqlite to webappsstore.sqlite during LS initialization and then import about:newtab specific data to IDB somewhere in browser initialization. We also need to convert about:newtab to use IDB of course.
Sigh.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jan Varga [:janv] from comment #22)
> Now it is stored in webappsstore.sqlite.

This sounds like a problem. chromeappstore.sqlite was explicitly created so that clearing localstorage data didn't wipe out about: page data (see https://hg.mozilla.org/mozilla-central/rev/737c80aa9134 ). Did this bug regress that?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

about: pages shouldn't be using localStorage at all due to it's sync-IO nature. They should migrate to IDB as soon as possible.

Of course, it's not ok that we have dataloss, so me and Jan has been talking about various options to fix the current situation.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #25)
> about: pages shouldn't be using localStorage at all due to it's sync-IO
> nature. They should migrate to IDB as soon as possible.

No objection here, we're already well aware of this problem. But we can't go busting the existing functionality until we've actually done that work.