Last Comment Bug 776416 - Remove exceptions to 5MB quota rule in localStorage
: Remove exceptions to 5MB quota rule in localStorage
Status: RESOLVED FIXED
[Snappy:P3]
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on: 791447 791565 817104
Blocks: 781866 791232
  Show dependency treegraph
 
Reported: 2012-07-22 17:43 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2012-11-30 12:21 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (106.69 KB, patch)
2012-07-22 17:44 PDT, Jonas Sicking (:sicking) PTO Until July 5th
honzab.moz: review+
Details | Diff | Splinter Review
Updated to tip (108.97 KB, patch)
2012-09-06 19:34 PDT, Jonas Sicking (:sicking) PTO Until July 5th
jonas: review+
Details | Diff | Splinter Review
Fix remaining tests (9.59 KB, patch)
2012-09-13 11:20 PDT, Jan Varga [:janv]
jonas: review+
Details | Diff | Splinter Review
Remove remaining clearOfflineApps() callers (2.27 KB, patch)
2012-09-13 23:32 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2012-07-22 17:43:33 PDT
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) PTO Until July 5th 2012-07-22 17:44:05 PDT
Created attachment 644806 [details] [diff] [review]
Patch to fix
Comment 2 Honza Bambas (:mayhemer) 2012-07-23 08:34:12 PDT
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) PTO Until July 5th 2012-07-23 11:55:24 PDT
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) 2012-07-23 13:44:44 PDT
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) PTO Until July 5th 2012-07-23 14:07:14 PDT
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) 2012-07-23 14:14:00 PDT
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) 2012-07-23 15:23:50 PDT
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) 2012-08-03 09:25:30 PDT
Comment on attachment 644806 [details] [diff] [review]
Patch to fix

reverting the r flag (actually, r- was accidental)
Comment 9 Honza Bambas (:mayhemer) 2012-08-08 08:45:42 PDT
(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) 2012-08-08 11:17:20 PDT
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) PTO Until July 5th 2012-08-08 13:26:28 PDT
Remote xul was disabled in bug 546857

Thanks for the review!!
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2012-09-06 19:34:12 PDT
Created attachment 659090 [details] [diff] [review]
Updated to tip

Forwarding review flag.
Comment 13 Jan Varga [:janv] 2012-09-13 11:20:29 PDT
Created attachment 660911 [details] [diff] [review]
Fix remaining tests
Comment 16 Philip Chee 2012-09-13 22:32:41 PDT
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] 2012-09-13 23:09:59 PDT
will do that, sorry
Comment 18 Jan Varga [:janv] 2012-09-13 23:32:06 PDT
Created attachment 661142 [details] [diff] [review]
Remove remaining clearOfflineApps() callers
Comment 19 Jan Varga [:janv] 2012-09-14 00:56:39 PDT
(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) PTO Until July 5th 2012-09-14 17:42:41 PDT
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 2012-09-14 22:58:08 PDT
about:newtab (new tab page) is broken.

http://forums.mozillazine.org/viewtopic.php?p=12288671#p12288671
Comment 22 Jan Varga [:janv] 2012-09-15 02:37:06 PDT
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] 2012-09-15 02:45:43 PDT
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: gavin@gavinsharp.com] 2012-09-16 09:20:25 PDT
(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) PTO Until July 5th 2012-09-16 10:56:12 PDT
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: gavin@gavinsharp.com] 2012-09-18 01:36:41 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.