742822 - make localStorage and indexedDB share quota mechanisms

Status: RESOLVED FIXED

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

Description

[Nathan Froyd [:froydnj]]

This bug is a prerequisite to removing the indexedDB first usage prompt.

My understanding of the steps necessary to do this are:

1. Move localStorage DBs underneath $PROFILE/indexedDB/ so that SQLite quota mechanisms kick in for them too. 
2. Move the quota initialization mechanisms from IndexedDatabaseManager up the stack somewhat, so they can be freely called from the localStorage bits.

The SQLite quota mechanism can only track DBs in a single directory and its children.  It is possible to add individual plain files, but those files have to live int he aforementioned directory hierarchy.  I suppose we could tweak the SQLite quota bits to be able to manage things in several hierarchies simultaneously; that might be an easier plan, though it would make upstream merges more difficult.

This is a perf team Q2 goal.

Comment 1

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

We can't make localStorage store it's data under $PROFILE/indexedDB until we've changed the policy used there to not prompt but rather use the new caching behavior. Currently the IndexedDB code puts up a prompt when we start writing files, we can't do that for localStorage, even temporarily, since it'll "break" a lot of websites.

I think it's fine to have all files that we manage using this new policy live under the single $PROFILE/indexedDB directory (though the name isn't ideal, but we can deal with that separately). So we shouldn't need to handle separate hierarchies. In fact, it seems nice for several reasons if all files for a given directory under $PROFILE/indexedDB/

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jonas Sicking (:sicking) from comment #1)
> We can't make localStorage store it's data under $PROFILE/indexedDB until
> we've changed the policy used there to not prompt but rather use the new
> caching behavior. Currently the IndexedDB code puts up a prompt when we
> start writing files, we can't do that for localStorage, even temporarily,
> since it'll "break" a lot of websites.

I am dense...what is the "new caching behavior" here?

And why does indexedDB prompt in the first place?  We already have quota mechanisms in place to make sure it doesn't overrun the disk.  I can totally get "this website has written 50MB of stuff to your drive, would you like to let it write  more", but what's the point of the first-run prompt with quotas?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Because even if you restrict the per-origin limit to a finite value, there are an infinite number of origins.

Comment 4

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

For the reason that Kyle mentioned, but also because we didn't want to leave permanent data on the users disk which may never get cleaned up (i.e. one of the problems that we are having with localStorage).

Regarding "the new caching behavior":

Currently we have a policy for both localStorage and IndexedDB to write files basically permanently to disk. They are not deleted unless the site does so intentionally. (Because of this permanence the IndexedDB implementation puts up a prompt). We also don't have a global limit for how much localStorage/IndexedDB data we can accumulate, we only have a per-site limit.

We want to switch to treating this data more like a cache. I.e. that we have have a global limit for how much data all of IndexedDB/localStorage can produce. Once we reach that limit, we throw out the IndexedDB/localStorage data for one of the domains according to some eviction policy (likely LRE for now).

So maybe "new caching behavior" is a bad description since there is no actual cache involved on our side. More like "use an eviction policy" :)

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> For the reason that Kyle mentioned, but also because we didn't want to leave
> permanent data on the users disk which may never get cleaned up (i.e. one of
> the problems that we are having with localStorage).

Do we think we are having a problem, or do we have data about this (e.g. Telemetry)?

> Currently we have a policy for both localStorage and IndexedDB to write
> files basically permanently to disk. They are not deleted unless the site
> does so intentionally. (Because of this permanence the IndexedDB
> implementation puts up a prompt). We also don't have a global limit for how
> much localStorage/IndexedDB data we can accumulate, we only have a per-site
> limit.
> 
> We want to switch to treating this data more like a cache. I.e. that we have
> have a global limit for how much data all of IndexedDB/localStorage can
> produce. Once we reach that limit, we throw out the IndexedDB/localStorage
> data for one of the domains according to some eviction policy (likely LRE
> for now).

You mean LRU?

Thanks for the explanation.  So this bug is tied up with bug 735415?  (IIUC, that bug is mistitled, then.)

Comment 6

[Nathan Froyd [:froydnj]]

And tied up with bug 729320 also.

Comment 7

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

I don't think we have telemetry data on how much data users are actually lugging around currently. But this was enough of a concern that we decided to put in the prompt in IndexedDB. This prompt is something that we really need to remove in order for IndexedDB to be a plausible alternative to localStorage.

Comment 8

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

Opps, submitted too quickly.

Yes, I meant LRU :-)

And yes, bug 735415 and bug 729320 were early explorations which has become our current thinking. I.e. into what is described in comment 4.

I don't particularly care which bug the work is done in. I'm happy to close out the other bugs and just have this one be where we focus our efforts.

Comment 9

[Nathan Froyd [:froydnj]]

JFTR, I think that you are setting too high a bar for what needs to be done before the prompt gets removed.  I understand the finite quota x infinite domains argument and agree that global limits should be put into place; I just don't think they need to be done prior to prompt removal.  Let's ditch the clunky UI first; some of the plumbing (for a feature that is still in draft) can be worked out later.

Working out the details of expiry, especially expiry between localStorage and indexedDB and/or expiry across domains for indexedDB databases, seems pretty ugly.  I suppose one could just subsume the localStorage entries into the .sqlite file for indexedDB for each domain for the former.

Comment 10

[(dormant account)]

(In reply to Nathan Froyd (:froydnj) from comment #9)
> JFTR, I think that you are setting too high a bar for what needs to be done
> before the prompt gets removed.  I understand the finite quota x infinite
> domains argument and agree that global limits should be put into place; I
> just don't think they need to be done prior to prompt removal.  Let's ditch
> the clunky UI first; some of the plumbing (for a feature that is still in
> draft) can be worked out later.

I think that's how we should proceed.

Comment 11

[Jan Varga [:janv]]

taking

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch 1 - getDirectoryForOrigin static

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch 2 - indentation

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch 3 - remove ShouldPreload

If we split the database in multiple files, this operation cannot be done anymore.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch 4 - notification via PContent

Currently, with 1 DOMStorageDBChild per process, we send notifications using PStorage. But in the future we will have 1 DOMStorageDBChild per window, and that means that we must have a different way to dispatch notifications.
With this patch we do it using PContent.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch 1 - getDirectoryForOrigin static

The previous version broke some xpcshell tests.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch 4 - notifications via PContent

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8702550 [details] [diff] [review]
patch 1 - getDirectoryForOrigin static

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

::: toolkit/forgetaboutsite/test/unit/head_forgetaboutsite.js
@@ +6,5 @@
>  var Ci = Components.interfaces;
>  var Cu = Components.utils;
>  
>  var dirSvc = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);
> +var profileDir = do_get_profile(true);

Similar changes has been done also in:

dom/indexedDB/test/unit/test_globalObjects_other.js
dom/indexedDB/test/unit/test_sandbox.js
dom/indexedDB/test/unit/xpcshell-head-child-process.js
dom/indexedDB/test/unit/xpcshell-head-parent-process.js
dom/push/test/xpcshell/test_register_error_http2.js

Comment 19

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

> patch 3 - remove ShouldPreload
> 
> If we split the database in multiple files, this operation cannot be done
> anymore.

Does that mean that we can't preload any more? The preload is pretty important for performance. We need to make sure that we make the correct tradeoffs here.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: WIP - part 5

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #19)
> > patch 3 - remove ShouldPreload
> > 
> > If we split the database in multiple files, this operation cannot be done
> > anymore.
> 
> Does that mean that we can't preload any more? The preload is pretty
> important for performance. We need to make sure that we make the correct
> tradeoffs here.

Exactly.  What are rationals for these patches?  Preload is a killing feature and I really don't understand why it should be removed according migration to the QM.

Comment 22

[Jan Varga [:janv]]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> (In reply to Jonas Sicking (:sicking) from comment #19)
> > > patch 3 - remove ShouldPreload
> > > 
> > > If we split the database in multiple files, this operation cannot be done
> > > anymore.
> > 
> > Does that mean that we can't preload any more? The preload is pretty
> > important for performance. We need to make sure that we make the correct
> > tradeoffs here.
> 
> Exactly.  What are rationals for these patches?  Preload is a killing
> feature and I really don't understand why it should be removed according
> migration to the QM.

I already spoke with Andrea about this, he removed the preload just temporarily to make coding/reviewing easier. He/we will reintroduce it in another patch.

Comment 23

[Honza Bambas (:mayhemer)]

Hmm... it seems you want to preload all the time, right?  If so, you will perform preloads where it's not needed (and LS is never touched).  It means waste of IPC rounds.

Unless really critical, I would leave the preloads implementation as is.

Comment 24

[Andrea Marchesini [:baku]]

> Unless really critical, I would leave the preloads implementation as is.

If the plan is to split the database in multiple ones per origin, the current preload implementation cannot work as it is.
I disabled it because my plan is to reintroduce it later in a separate patch to land at the same time.
The 'new' preload implementation will use QM to iterate between all the origins and check if there is something to preload.

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini (:baku) from comment #24)
> > Unless really critical, I would leave the preloads implementation as is.
> 
> If the plan is to split the database in multiple ones per origin, the
> current preload implementation cannot work as it is.
> I disabled it because my plan is to reintroduce it later in a separate patch
> to land at the same time.
> The 'new' preload implementation will use QM to iterate between all the
> origins and check if there is something to preload.

Just interested: do you really need to remove it now?  Cannot you rather replace it when your new implementation is done?

Comment 26

[Andrea Marchesini [:baku]]

> Just interested: do you really need to remove it now?  Cannot you rather
> replace it when your new implementation is done?

This is exactly what I want to do :)
These patches are not ready to land. I uploaded them in order to receive the first feedback from janv, but I want to land all together and not in intermediate steps.

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini (:baku) from comment #26)
> > Just interested: do you really need to remove it now?  Cannot you rather
> > replace it when your new implementation is done?
> 
> This is exactly what I want to do :)
> These patches are not ready to land. I uploaded them in order to receive the
> first feedback from janv, but I want to land all together and not in
> intermediate steps.

Sounds good, thanks!

Comment 28

[Andrea Marchesini [:baku]]

Janv, can you give me a quick feedback (maybe not a fully review) of the first patches?
In order to know if I'm going in the right direction. Thanks!

Comment 29

[Jan Varga [:janv]]

baku, can you give me a few days ? I need to finish a patch for bug 1182987

Comment 31

[Andrea Marchesini [:baku]]

Attachment: patch 1 - getDirectoryForOrigin static

Comment 32

[Andrea Marchesini [:baku]]

Attachment: patch 2 - indentation

Comment 33

[Andrea Marchesini [:baku]]

Attachment: patch 3 - remove ShouldPreload

Comment 34

[Andrea Marchesini [:baku]]

Attachment: patch 4 - notifications via PContent

Comment 35

[Andrea Marchesini [:baku]]

Attachment: patch 1 - getDirectoryForOrigin static

Comment 36

[Andrea Marchesini [:baku]]

Attachment: patch 2 - indentation

Comment 37

[Andrea Marchesini [:baku]]

Attachment: patch 3 - remove ShouldPreload

Comment 38

[Andrea Marchesini [:baku]]

Attachment: patch 2 - indentation

Comment 39

[Andrea Marchesini [:baku]]

Attachment: patch 3 - remove ShouldPreload

Comment 40

[Andrea Marchesini [:baku]]

Attachment: patch 4 - notifications via PContent

Comment 41

[Jan Varga [:janv]]

I'm working on a new architecture in bug 1286798. However, it seems I might be able to reuse some of the work done here.

Comment 42

[Michael REMY]

hi,
this bug still happens, so the data manageris bugging : long script...
my webappsstore.sqlite is only 25MB....
so i delete it, then data manager works fine again.

Comment 43

[Michael REMY]

Attachment: Image1.jpg

the data manager frozen until this error box

Comment 44

[Marion Daly [:mdaly]]

:Janv can you give a text update on the bug here?

Comment 45

[Jan Varga [:janv]]

Yeah, this will be fixed by bug 1286798, no need to do anything special here.

Comment 46

[Jan Varga [:janv]]

LSNG has been enabled on Release in bug 1599979, so this can be finally resolved as fixed.

Here are some LSNG features relevant for this bug:

• Quota checks and usage reporting for LS is shared with IDB and Cache API (and will be shared with any future quota clients like OPFS)
• There’s no 5MB for entire eTLD+1 group as in the old implementation, the group limit is now 10GB (given/managed by QM).
• The size of LS data is included in navigator.storage.estimate()
• The logical size of LS data is tracked (this is different from all other quota clients like IDB and Cache API which track the physical size of files on disk)
• LS data is evicted along with IDB and Cache API (the new caching behavior with LRU eviction, no prompts asking for a permission are required)
• Alll data is stored under $PROFILE/storage (under the same directory hierarchy)