Remove exceptions to 5MB quota rule in localStorage

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

({dev-doc-needed})

Trunk
mozilla18
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P3])

Attachments

(2 attachments, 2 obsolete attachments)

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.
Created attachment 644806 [details] [diff] [review]
Patch to fix
Assignee: nobody → jonas
Attachment #644806 - Flags: review?(honzab.moz)
OS: Mac OS X → All
Hardware: x86 → All
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?
Attachment #644806 - Flags: review?(honzab.moz) → review-
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.
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/
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.
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.

Updated

5 years ago
Whiteboard: [Snappy:P3]
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 on attachment 644806 [details] [diff] [review]
Patch to fix

reverting the r flag (actually, r- was accidental)
Attachment #644806 - Flags: review- → review?
(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 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.
Attachment #644806 - Flags: review? → review+
Remote xul was disabled in bug 546857

Thanks for the review!!
Created attachment 659090 [details] [diff] [review]
Updated to tip

Forwarding review flag.
Attachment #644806 - Attachment is obsolete: true
Attachment #659090 - Flags: review+
Blocks: 781866

Comment 13

5 years ago
Created attachment 660911 [details] [diff] [review]
Fix remaining tests
Attachment #660911 - Flags: review?(jonas)
Attachment #660911 - Flags: review?(jonas) → review+

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd824db5b0df
https://hg.mozilla.org/integration/mozilla-inbound/rev/e564016b9de9
https://hg.mozilla.org/mozilla-central/rev/fd824db5b0df
https://hg.mozilla.org/mozilla-central/rev/e564016b9de9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 16

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

http://mxr.mozilla.org/comm-central/search?string=.clearOfflineApps
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

5 years ago
will do that, sorry

Comment 18

5 years ago
Created attachment 661142 [details] [diff] [review]
Remove remaining clearOfflineApps() callers
Attachment #661142 - Flags: review?(jonas)

Updated

5 years ago
Attachment #661142 - Attachment is obsolete: true
Attachment #661142 - Flags: review?(jonas)

Comment 19

5 years ago
(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.

Updated

5 years ago
Blocks: 791232
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
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 21

5 years ago
about:newtab (new tab page) is broken.

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

Comment 22

5 years ago
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

5 years ago
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.
Depends on: 791447
(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?

Updated

5 years ago
Depends on: 791565
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.
(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.

Updated

5 years ago
Keywords: dev-doc-needed
Depends on: 817104
You need to log in before you can comment on or make changes to this bug.