915838 - Provide add-ons a standard directory to store data, settings

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Alex Vincent [:WeirdAl]]

Add-ons may want to store settings or other data on the local computer. In the past, add-ons would use preferences or an arbitrary location on the user's filesystem. Preferences are less than ideal, so each Addon object should provide built-in helpers to read and write data specific to the add-on.

Note bug 872980, which proposes to reduce the maximum size of a preference value to 4KB.  This could have an adverse effect on add-ons.

Please reference the URL field for design discussion.  I'd like to implement this within a few months, but we should come to a consensus on what to offer.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

> Note bug 872980, which proposes to reduce the maximum size of a preference value to 4KB.  This could have an adverse effect on add-ons.

Well, having a preference file that is larger than ~32kb has an adverse effect on Firefox itself, so I'm willing to defend that bug :)

Also, I seem to remember that gps filed a smiliar bug 1-2 months ago. I don't think that there has been any activity on that bug, though.

Comment 2

[Gregory Szorc [:gps]]

Bug 866238 is similar. We also had a meeting a few months back about providing something better than prefs but not SQLite. Notes at https://etherpad.mozilla.org/storage-in-gecko. In the time since, DeferredSave.jsm was implemented. IMO that would be an interesting base for a generic storage API for add-ons.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Storing in flat JSON would have the benefit of not being tied to a specific thread. We are slowly approaching sqlite on chrome workers, though (bug 853439).

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bug 866238 sounds great - however, that only solves the issue of small to medium sized data. That would be great for many add-ons, especially if we get some add-on specific helper APIs (ie, automatic namespacing based on the add-on ID).

But some add-ons want to store large amounts of data, and potentially have it automatically cleaned up on uninstall (and/or part of an add-on reset feature). Or a simple key/value store may not be the right tool for some other reason. For that use-case, I think just a managed directory would work nicely - you get a directory that's dedicated to just your add-on and you can put anything in there (large datasets usually mean each add-on's requirements will be different, so best not to tie it to a specific API). It'll be automatically cleaned up by the Add-on Manager when it's appropriate. And we could potentially add a flag to install.rdf to indicate the add-on uses a data directory - so the directory can be automatically created on install (only as a convenience - it can be created any time during runtime).

With a folder structure something like:
  <PROFILE-DIR>/extension-data/<ADDON-ID>

Comment 5

[David Teller [:Yoric] - still alive but not very active]

I like the idea of a managed directory. Simple, generic.

Comment 6

[Alex Vincent [:WeirdAl]]

Attachment: bug915838.diff

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 818153 [details] [diff] [review]
bug915838.diff

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

One edge case we should cover here is when an add-on's ID changes when its updated. To do that, look for any place that calls location.installAddon() *and* passes in the |existingAddonID| argument.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5941,5 @@
>    /**
> +   * getDataDirectory tries to execute the callback with three arguments:
> +   * 1) the path of the data directory within the profile,
> +   * 2) whether it guaranteed the data directory's existence (true means it did)
> +   * 3) A helper object (currently null) for assisting addon developers with common tasks.

Remove this - file a dependent bug instead.

@@ +5943,5 @@
> +   * 1) the path of the data directory within the profile,
> +   * 2) whether it guaranteed the data directory's existence (true means it did)
> +   * 3) A helper object (currently null) for assisting addon developers with common tasks.
> +   */
> +  getDataDirectory: function(callback) {

Should follow suit with other methods, and define them like:
  this.method = function() {}

@@ +5953,5 @@
> +    function fail(reason) {
> +      callback(dirPath, false, null);
> +    }
> +
> +    parentPromise.then(

Bug 934283 would have made this so much easier.

@@ +5967,5 @@
> +      fail
> +    );
> +  },
> +  
> +  /**

Nit: Trailing whitespace.

Comment 8

[Alex Vincent [:WeirdAl]]

Attachment: #addons IRC discussion after review-

Comment 9

[Techlive Zheng]

Attachment: Contents of my profile directory

Here is a screenshot about the contents of my current profile directory which is in use about 5 yeas, as you can see, there are many files and directories created by addons, and there is no easy way to tell which files are leftovers of an already uninstalled addon. I think it should be stored in a unified way as mentioned in bug 851018.

Is it possible to make addons which have files left over in the profile directory to move these files to the new managed data directory?

Comment 10

[Alex Vincent [:WeirdAl]]

(In reply to Techlive Zheng from comment #9)
> Is it possible to make addons which have files left over in the profile
> directory to move these files to the new managed data directory?

From a technical perspective, no:  there really is no way to force an addon to use this new directory.  But I would definitely add it to "best practices" documentation for addons.

From a policy perspective, maybe:  first we have to provide the directory to the addon (this bug), then we would have to add the data directory as a requirement for addons, then define a migration period where existing addons have time to do the move (no less than a year, in my opinion).  Providing utilities to help them manage their own settings would be helpful (that's a follow-up bug, as Blair points out above).  I think it would be a good policy decision, but we couldn't just require it out of the blue without plenty of warning to add-on developers.

Part of the problem is that there's no way to tell which files belong to which addons.

Comment 11

[Alex Vincent [:WeirdAl]]

Attachment: bug915838.diff

(In reply to Blair McBride [:Unfocused] from comment #7)
> One edge case we should cover here is when an add-on's ID changes when its
> updated. To do that, look for any place that calls location.installAddon()
> *and* passes in the |existingAddonID| argument.

It turns out this leads to some deeper issues about moving files around, which I've raised with comments inside installAddon().

Most worrisome is that installAddon and SafeInstallOperation aren't really compatible with OS.File, so I have to fall back to FileUtils.  I'd feel more comfortable if there were a wholesale conversion of the XPIProvider module to OS.File...

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 832678 [details] [diff] [review]
bug915838.diff

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

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +6988,5 @@
>          moveOldAddon(aExistingAddonID);
>  
> +        {
> +          // Move the data directories.
> +          /* XXX ajvincent We can't use OS.File:  installAddon isn't compatible

Yea, this is fine. Changing all this to use OS.File is going to need to be part of a bigger project.

@@ +6993,5 @@
> +           * with Promises, nor is SafeInstallOperation.  That's a bigger change
> +           * than I want to undertake just yet.  Also, this could potentially add
> +           * much more strain to SafeInstallOperation than we really want... can
> +           * we consider copying all these files to a temp folder and then rename
> +           * the folder when we're done, a la SafeFileOutputStream.finish()?

Copying datadir could potentially b a *huge* amount of I/O - I don't think we can consider doing that until all this is async via OS.File.

@@ +7005,5 @@
> +            );
> +
> +            /* transaction.move wants to create oldFile.leafName under
> +             * newDataDir... so we can't call transaction.move(oldDataDir)
> +             * directly.

Better to just add a param to to transaction.move() to allow moving to a different leafname, rather than handing it manually here. As a bonus, that will mean we can rollback the operation.

@@ +7011,5 @@
> +            let entries = getDirectoryEntries(oldDataDir, true);
> +            entries.forEach(function(aEntry) {
> +              /* XXX ajvincent What should happen if newDataDir contains a file
> +               * with the same name?  Right now the transaction would abort.
> +               * (This has to include files in subdirectories too...)

If newDataDir exists, we should be it to the trash folder first. Then any rollback of this transaction can restore it.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 832678 [details] [diff] [review]
bug915838.diff

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

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +6097,5 @@
> +   * 2) whether it guaranteed the data directory's existence (true means it did)
> +   */
> +  getDataDirectory: function(callback) {
> +    let parentPath = OS.Path.join(OS.Constants.Path.profileDir, "extension-data");
> +    let dirPath = OS.Path.join(parentPath, this.id);

Nit: using Task.jsm would make this much more readable.

@@ +7001,5 @@
> +          );
> +          if (oldDataDir.exists()) {
> +            let newDataDir = FileUtils.getDir(
> +              KEY_PROFILEDIR, ["extension-data", aId], true, true
> +            );

Note that this call to getDir is going to cause main thread I/O.

Comment 14

[Alex Vincent [:WeirdAl]]

It's probably going to be over a week before I can work on this again.  Also, regarding main thread I/O:  we know.  That's why my comment said we can't convert to OS.File yet.  Yes, it will make the problem worse.  We need a follow-up bug for the conversion.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

I'm not sure why the OS.File conversion should be a follow-up. Shouldn't it rather be a blocker?

Note: I was outlining the main thread I/O in getDir because it's more subtle than the rest of the main thread I/O in the code.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to David Rajchenbach Teller [:Yoric] <currently on training, will be back on Friday 6th> from comment #15)
> I'm not sure why the OS.File conversion should be a follow-up. Shouldn't it
> rather be a blocker?

IMO, no. 

It's a tiny amount of added sync I/O, compared to what we're already doing there - it's not going to make it noticeably worse. We need to convert this to async, but I don't see a need to make that block this feature.

Comment 17

[Alex Vincent [:WeirdAl]]

Attachment: bug915838.diff

Can I get someone to write a couple tests exercising the data directory move code?  I'm not used to dealing with addons changing ID's.

Comment 19

[Alex Vincent [:WeirdAl]]

Attachment: JSONStore.jsm

I wrote this module for APN and have their written permission to submit it to Mozilla under my name.  Standard MPL/LGPL/GPL for Mozilla code applies.

This module is for storing and retrieving JSON values on the filesystem under the extension-data directory for an addon.  We go two levels deep (namespace, name, value) because our particular needs required that.  This should be a very acceptable alternative to preferences for addons to use.

I'm not submitting it as a patch just yet.  First, we'd have to determine where in the source tree and in the final build it should live.  Second, there is a known bug whereby we wait up to five seconds before flushing to the disk.  A FF crash or shutdown could cause data loss.  AsyncShutdown.jsm is probably the cure for that.

Third, I think we should consider moving some JSM's around, especially addon-related ones.  resource://gre/modules/ at the top level is getting kind of crowded, and I think certain modules should be in subdirectories of that to distinguish "public" modules (those others are supposed to use) from "private" modules (which addon users should avoid talking to directly).

feedback? Yoric:  I'd like you to take a look at this and tell me what you think.  I tried very hard to use OS.File correctly.  :)

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Alex Vincent [:WeirdAl] from comment #19)
> I wrote this module for APN and have their written permission to submit it
> to Mozilla under my name.  Standard MPL/LGPL/GPL for Mozilla code applies.

This seems like it would be helpful, thanks :)

Could you put this in a separate bug? I don't want to detract from the main work here. That it's not in patch form yet is perfectly fine. Toolkit::General seems appropriate.


> Third, I think we should consider moving some JSM's around, especially
> addon-related ones.  resource://gre/modules/ at the top level is getting
> kind of crowded, and I think certain modules should be in subdirectories of
> that to distinguish "public" modules (those others are supposed to use) from
> "private" modules (which addon users should avoid talking to directly).

I've been thinking the same :\

Comment 21

[Alex Vincent [:WeirdAl]]

(In reply to Blair McBride [:Unfocused] from comment #20)

> Could you put this in a separate bug? I don't want to detract from the main
> work here. That it's not in patch form yet is perfectly fine.
> Toolkit::General seems appropriate.

Filed bug 952304 for JSONStore.jsm.

Comment 22

[Alex Vincent [:WeirdAl]]

(In reply to Alex Vincent [:WeirdAl] from comment #19)
> Third, I think we should consider moving some JSM's around, especially
> addon-related ones.  resource://gre/modules/ at the top level is getting
> kind of crowded, and I think certain modules should be in subdirectories of
> that to distinguish "public" modules (those others are supposed to use) from
> "private" modules (which addon users should avoid talking to directly).

Filed bug 952307 for moving modules to subdirectories.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8350337 [details]
JSONStore.jsm

Discussion started as part of bug 952304.

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8346284 [details] [diff] [review]
bug915838.diff

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

Finally getting my review queue down to single digits for the first time in many many months. Apologies for the glacial pace.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +6261,5 @@
> +  getDataDirectory: function(callback) {
> +    let parentPath = OS.Path.join(OS.Constants.Path.profileDir, "extension-data");
> +    let dirPath = OS.Path.join(parentPath, this.id);
> +
> +    Task.spawn(function() {

If you make this a new-style ES6 generator function:
  function*() {
... you don't need to throw StopIteration.

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Landed on fx-team:
 https://hg.mozilla.org/integration/fx-team/rev/3fa7b987fb9e

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3fa7b987fb9e

Comment 27

[Alex Vincent [:WeirdAl]]

This will be a new feature for addon developers.  We want to encourage addons to use the data directory instead of preferences.  However, there's still some policy decisions to be made (see comment 10), and it'd be nice to provide some tools to help developers store their data here (bug 952304 or something similar).

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8346284 [details] [diff] [review]
bug915838.diff

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

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +6257,5 @@
> +   * getDataDirectory tries to execute the callback with two arguments:
> +   * 1) the path of the data directory within the profile,
> +   * 2) any exception generated from trying to build it.
> +   */
> +  getDataDirectory: function(callback) {

What is the point of making this callback-based while we're moving all our APIs to Promise-based?

@@ +7172,5 @@
> +              transaction.moveUnder(newDataDir, trashData);
> +            }
> +
> +            transaction.moveTo(oldDataDir, newDataDir);
> +          }

I count 5 cases of main thread I/O in this chunk. If bug 945540 needs to be fixed to let us move this OMT, I would suggest prioritizing it.

Comment 29

[Kris Maglione [:kmag]]

(In reply to David Rajchenbach Teller [:Yoric] (away until August 20th - use "needinfo") from comment #28) 
> What is the point of making this callback-based while we're moving all our
> APIs to Promise-based?

The rest of the AddonManager API is callback-based, so I think it makes sense for this to follow the same pattern. It would be nice, though, to have all of the APIs return a promise in the absence of a passed callback, and deprecate the callback pattern at some point in the future.