:Irving Reid (No longer working on Firefox) Reporter
	Description • 11 years ago

+++ This bug was initially created as a clone of Bug #853388 +++

There are a number of issues with SQLITE in the add-ons manager and XPI provider, including jank (bug xxx), main thread slow SQL (bug 729330), memory and disk space (bug 726556, bug 768312)

For details about the planned change please see the etherpad at https://etherpad.mozilla.org/snappy-addon-manager

A lot of the concerns discussed in the comments to bug 699839 are relevant.

	:Irving Reid (No longer working on Firefox) Reporter
	Comment 1 • 11 years ago

See https://bugzilla.mozilla.org/show_bug.cgi?id=853388#c1 for additional discussion.

Other possibly relevant bugs:

Bug 791987 - Don't rely on extensions.ini to rebuild database
Bug 793143 - extensions.enabledAddons should use JSON as the data format
Bug 793139 - Provide a simple sync API to just get an add-on's version, without requiring database access

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 2 • 11 years ago

*ahem*

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 3 • 11 years ago

Couple of things to keep in mind with this:
* AddonRepository just generally needs a redesign - the current code only supports a single request at a time, and there's no proper way to (only) cancel a request. So keep that in mind - don't want to be further boxed in there.
* addon.sqlite stores the compatibility overrides (the compatibility_override table) needed for compatible-by-default add-ons to be safe. That's the only really important thing that should be migrated, everything can safely be re-fetched from AMO (not saying you need to to throw that away... but its probably much easier to do so).
* AddonRepository is considered to be effectively a private API - its safe to change the API if it makes sense to do so (AFAICT, only 1 add-on on AMO uses it).

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 4 • 11 years ago

Posting some work in progress with the patches from my queue. This is the first patch with an example JSON structure that reflects what will be read/written to disk. Keeping it as a separate patch so it's easy to simply not apply it afterwards.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 5 • 11 years ago

This patch contains the conversions of the main SQL operations in AddonDatabase to operations on the JSON structure, which simplifies things a lot in some cases.

Note that this patch avoids changing the callback behavior (even though it could if wanted), by using executeSoon such that all callbacks still go through a spin in the event loop. I'm planning on changing this selectively on a case-by-case as I do more testing

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 6 • 11 years ago

*poof* [1]

This removes the SQL queries and supporting code left unused by the changes from the previous patches. There's only some SQL code left in openConnection as that function will be dealt in the patch related to file handling.

[1] *poof* reserved for all-removal patches

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 7 • 11 years ago

Now some overview about the work in progress:

* At first I began with an approach to store the data in AddonDatabase, and when the data was requested, to return a copy of the JSON object to guarantee that no outside consumers could change it. But I've since decided to use Object.freeze in the structure and its sub-objects and do a direct handover of the object to the consumer, which should save both time and memory. This is almost complete in the main patch but still needs some extra touches.


* I'm doing a crude handling of the internalId property in the current WIP because I suspect it was only necessary for the indexes between tables in the DB and won't be necessary anymore here.


* The sub-objects for each add-on are using the classes from AddonManagerPrivate (e.g. AddonAuthor, AddonScreenshot, etc.), but the top-level object representing each add-on is a vanilla JS object. It almost seems that I should rename and reuse AddonSearchResult for this purpose, although I'm pondering if this would be really necessary. Would be happy to hear some opinions!


* In the current AddonDatabase API, openConnection is sync, but retrieveStoredData is async, meaning that a consumer of the data already needs to wait for the asynchronousness. With that I've been thinking of converting openConnection to async as well, so that I could do the file reading and (possibly) JSON parsing off the main thread.
The only problem is that would leave the databaseOk field with unknown data while the file is being read, and databaseOk is used to decide if caching is enabled or not.
However, i'm not sure if this is would be a problem (or even if it's working properly at the moment), because openConnection is only lazily called by the first getAsyncStatement, at which moment I believe AddonRepository has already called .cacheEnabled()
Blair, any thoughts?

> * AddonRepository just generally needs a redesign - the current code only supports a
> single request at a time, and there's no proper way to (only) cancel a request. So keep
> that in mind - don't want to be further boxed in there.

fwiw I'm intentionally only changing the inner workings of AddonDatabase and avoiding doing more changes to not go out of scope, so while I think this can help ease a redesign of AddonRepository, I imagine it probably won't include one (at least on first step). But certainly (i hope) it won't box things further

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 8 • 11 years ago

(In reply to :Felipe Gomes from comment #7)
> But I've since decided
> to use Object.freeze

Me like.


> * I'm doing a crude handling of the internalId property in the current WIP
> because I suspect it was only necessary for the indexes between tables in
> the DB and won't be necessary anymore here.

Correct. AMO's API does return it's own internal ID, but we ignore it - our DB's internal ID is completely unrelated, and used only to join tables. 


> It almost
> seems that I should rename and reuse AddonSearchResult for this purpose,

Yep, lets do that. Gives us a nice self-documenting API, and forces us to be more intentional when handling data from AMO.


> * In the current AddonDatabase API, openConnection is sync, but
> retrieveStoredData is async, meaning that a consumer of the data already
> needs to wait for the asynchronousness. With that I've been thinking of
> converting openConnection to async as well, so that I could do the file
> reading and (possibly) JSON parsing off the main thread.
> The only problem is that would leave the databaseOk field with unknown data
> while the file is being read, and databaseOk is used to decide if caching is
> enabled or not.
> However, i'm not sure if this is would be a problem (or even if it's working
> properly at the moment), because openConnection is only lazily called by the
> first getAsyncStatement, at which moment I believe AddonRepository has
> already called .cacheEnabled()
> Blair, any thoughts?

Yea, that seems fine. It's kinda broken at the moment anyway, as .cacheEnabled isn't guaranteed to be accurate until after the DB is opened - making it async isn't going to change that. And in theory, it should be a very rare edge case.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 9 • 11 years ago

Comment on attachment 742930 [details] [diff] [review]
Sample JSON structure

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +18,5 @@
>                                    "resource://gre/modules/NetUtil.jsm");
>  
>  this.EXPORTED_SYMBOLS = [ "AddonRepository" ];
>  
> +let DATA = { addons: [

For future-proofing, could you include a schema version property here (like what bug 853388 is doing). Just in case we ever make a breaking change we don't foresee here.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 10 • 11 years ago

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #8)
> (In reply to :Felipe Gomes from comment #7)
> > But I've since decided
> > to use Object.freeze
> 
> Me like.

FWIW, I might need to back off a bit from this plan, due to consumers of the returned objects possibly wanting to modify the JS object and its values (which they have always expected to be fine since the object was a copy from the DB -- and not *the* DB itself).

But there's still some hope. There are two scenarios to consider:

[1] Consumers wanting to stick their own properties in the object to keep track of the internal state of things
[2] Consumers wanting to change the values of the existing properties


Now, [1] seems very likely (at least it's clear it happens in XPIProvider but I haven't proven yet it happens here. But i'm not sure how far the objects returned here make into other modules without being transformed into other objects)
It's less clear if [2] happens or not.

If only [1] is a concern, we can still keep a similar approach and mark all of the existing properties as non-writable, while allowing [1] to keep working. If [2] is also to be considered then we'll need to copy the objects.

I know this is a private API but since objects are returned from AddonRepository to the outside world, they may make it far into other code that needs to make modifications to them.


> For future-proofing, could you include a schema version property here (like
> what bug 853388 is doing). Just in case we ever make a breaking change we
> don't foresee here.

Absolutely, I've since done that and it's in an updated version of the patch. Irving and I talked about some migration cases in the future, and we realized that, in order to support downgrading/multiple-versions-usage between versions with the json DB, we cannot do what the current patches do which is to read only the expected properties and discard the rest, because we might be discarding new fields that were added in a later schema.

So we came up with the following proposal:

- Read all properties from the file DB and keep them
- Validate the known ones that are worth validating
- Don't write to disk any property that starts with "_", these will be reserved for internal usage that does not need to make it back to disk
- Write all other properties back.


Blair, what do you think?  Alternatives would be to keep a list of all properties that were read at the beginning and write these ones back; or a more detailed schema upgrade system

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 11 • 11 years ago

Comment on attachment 742932 [details] [diff] [review]
AddonDatabase main operations

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1920,5 @@
>     *         The add-on to insert into the database
>     * @param  aCallback
>     *         The callback to call once complete
>     */
>    _insertAddon: function AD__insertAddon(aAddon, aCallback) {

aAddon is already an AddonSearchResult (from _parseAddon) - so you won't need to do all this processing to clone the data.

Also, be aware that some AddonSearchResult objects will be sparse objects, not containing the usual full data set. This happens when an add-on isn't hosted on AMO, but AMO has a list of compatibility overrides for it. See near the end of _beginGetAddons.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 12 • 11 years ago

(In reply to :Felipe Gomes from comment #10)
> I know this is a private API but since objects are returned from
> AddonRepository to the outside world, they may make it far into other code
> that needs to make modifications to them.

We don't support that on normal Addon objects handed out by XPIProvider or other providers - I don't think we should support it here either. Hardly any add-on uses AddonRepository - if any that do, I'm not worried about breaking them because of this (if you mess with our objects, you're gonna have a bad time). And I can't off the top of my head think of any case where XPIProvider does it.

> - Read all properties from the file DB and keep them
> - Validate the known ones that are worth validating
> - Don't write to disk any property that starts with "_", these will be
> reserved for internal usage that does not need to make it back to disk
> - Write all other properties back.

Yep, that sounds good.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 13 • 11 years ago

Posting the patches for review. Not many changes from what was discussed; details on each patch.

I broke down the the patches in logical steps to make it easier to understand. The code removal ended up a bit spread out as I tried to remove each part as they were no longer needed..

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 14 • 11 years ago

This patch makes the necessary changes and prepare the way to use the AddonSearchResult class both for the search code and for the DB code, as discussed in comment 8.

All the parsing code from a vanilla JS obj (and the conversions needed to stringify) are included here.

While in-memory, the expected fields (through the self-documenting prototype) are keep as regular fields, and everything else in the JSON (that might be part of a later schema) is put in the _remainingProperties obj, to avoid accidental clashes. They are merged again when stringifying through toJSON
[this is not strictly necessary but seemed nice to have]

The _make* functions are kept around in a dual mode (accepting rows as well) because we can reuse the same function during normal operation and on the code that will do the migration from SQL to JSON.

The only thing that this patch doesn't do is actually *renaming* AddonSearchResult to something else. It doesn't felt really necessary and I couldn't think of a better name. It should be an easy grep later to replace if wanted.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 15 • 11 years ago

This patch makes the main DB access operations to operate in the in-memory JS obj and remove the SQL counterparts. There's no file access here yet.

Worth noting is that the JSON representation is slightly different from what was showed before. It was:

  DB = { addons: [], schema: 5 }

now

  DB = { addons: {}, schema: 5 }

i.e., addons became an obj (vs an array), with the keys being their ids. I went back and forth on this but in the end this was chosen because it's closer to what AddonRepository expects to be returned at getCachedAddonByID so we can pass it more directly.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 16 • 11 years ago

This makes AddonDatabase read and write to addons.json.

There's a bit of lifting to preserve the previous API and its behavior that could be simplified in the future but I tried not to bring this into scope here (to also minimize test changes)

Writing is done using OS.File and operations are batched within  50ms using DeferredTask

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 17 • 11 years ago

This implements the SQLite -> JSON migration when addons.json is not found and addons.sqlite is. The code is basically all moved without changes from the previous code, but now is in a separate module. The code that is not related to reading (e.g. INSERT queries) are thrown away.

I removed the tryAgain part of DB opening because in my understanding that was done not to try to read again, but to try to delete and unlock the DB to get access to a blank one again.

The old schema upgrade is kept to simplify the code to avoid the SELECT queries to fail, but the createIndices and createTriggers were removed.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 18 • 11 years ago

Now what's left is some test adjustments. There's a test which is currently failing and giving me nightmares, as I don't know yet what's going on, but I believe it's something that needs change in the test, and not in the code.. Or if in the code, I don't anticipate it to be a big thing

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 19 • 11 years ago

Comment on attachment 751291 [details] [diff] [review]
AddonDatabase main operations

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

(In reply to :Felipe Gomes from comment #15)
> i.e., addons became an obj (vs an array), with the keys being their ids. I
> went back and forth on this but in the end this was chosen because it's
> closer to what AddonRepository expects to be returned at getCachedAddonByID
> so we can pass it more directly.

Yea, I think this makes sense - most access is via requesting specific IDs.

Though, I'd much prefer to use a Map - makes using the object *so* much nicer (and it is specifically designed for this use-case). The only downside is Map doesn't convert to/from JSON without a helper. See serializerHelper here:
https://hg.mozilla.org/projects/ux/file/tip/browser/components/customizableui/src/CustomizableUI.jsm#l1017

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 20 • 11 years ago

So when AddonDatabase returns its list of addons, AddonsRepository stores it for its own caching in self._addons. Do you mean for us to change that one too to a Map?

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 21 • 11 years ago

(In reply to :Felipe Gomes from comment #20)
> So when AddonDatabase returns its list of addons, AddonsRepository stores it
> for its own caching in self._addons. Do you mean for us to change that one
> too to a Map?

Up to you, depending how how much you want to keep the scope contained here. I think eventually, it'd be nice to have that refactored so AddonRepository doesn't have it's own cache, and just always goes through AddonDatabase.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 22 • 11 years ago

Comment on attachment 751288 [details] [diff] [review]
Reuse AddonSearchResult as main obj representation

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

(In reply to :Felipe Gomes from comment #14)
> While in-memory, the expected fields (through the self-documenting
> prototype) are keep as regular fields, and everything else in the JSON (that
> might be part of a later schema) is put in the _remainingProperties obj, to
> avoid accidental clashes. They are merged again when stringifying through
> toJSON
> [this is not strictly necessary but seemed nice to have]

Indeed, that is nice. I guess when we want to remove/deprecate a field, it'll get cleaned up the next time the data is re-fetched from AMO. Should double check that works ok when downgrading after a field has been removed.


> The _make* functions are kept around in a dual mode (accepting rows as well)
> because we can reuse the same function during normal operation and on the
> code that will do the migration from SQL to JSON.

TBH, I'd like to avoid this reuse, and relegate all SQLite related code into the migration module. In addition to contaminating AddonRepository.jsm, it's going to cause issues when the fields change - requiring migration code in the _make* functions instead of the separate module.


> The only thing that this patch doesn't do is actually *renaming*
> AddonSearchResult to something else. It doesn't felt really necessary and I
> couldn't think of a better name. It should be an easy grep later to replace
> if wanted.

Good-first-bug :)

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +288,5 @@
>  
>  function AddonSearchResult(aId) {
>    this.id = aId;
>    this.icons = {};
> +  this._remainingProperties = {};

"remaining" is a bit ambiguous. _unsupportedProperties, or something similar?

@@ +348,5 @@
>    /**
>     * The url of the add-on's icon
>     */
>    get iconURL() {
> +    return this.icons && this.icons[32];

Is this change needed? this.icons is guaranteed by the constructor to be an object.

@@ +567,5 @@
> +            break;
> +
> +          case "creator":
> +            json.creator = value ? value.name : "";
> +            json.creatorURL = value ? value.url : "";

This will get verbose/awkward if we start adding more metadata to AddonAuthor - better to serialize it as an object (ie, {name, url}). And, for that matter, add a toJSON method to AddonAuthor in AddonManager.jsm. Ditto for AddonScreenshot and AddonCompatibilityOverride.

@@ +2262,5 @@
> +      return aObj;
> +
> +    let id = aObj.id;
> +    if (!aObj.id)
> +      return;

Be intentional - return null.

@@ +2320,5 @@
> +              addon.icons[size] = url;
> +            }
> +            break;
> +
> +          case "iconURL":

Since this is just an alias for backwards compat, it shouldn't be serialized/deserialized.

@@ +2607,5 @@
>     * @param  aRow
>     *         The asynchronous row to use
>     * @return An object containing the size and URL of the icon
>     */
> +  _makeIcon: function AD_makeIconFromAsyncRow(aRow) {

Not used?

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 23 • 11 years ago

Comment on attachment 751292 [details] [diff] [review]
AddonDatabase file access

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1766,5 @@
> +
> +  /**
> +   * Synchronously opens a new connection to the database file.
> +   */
> +  openConnection: function() {

I assume you plan on making this async in the future? Cos, uh, this should totally be async.

@@ +1793,5 @@
> +
> +     inputAddons = JSON.parse(data);
> +
> +     if (!inputAddons.hasOwnProperty("addons") ||
> +         typeof(inputAddons.addons) !== "object") {

I wonder if the in-memory representation should be a Map, while the on-disk representation is an array - since it'll save on bytes that need to be read/written, and we need to iterate over the data anyway when reading/writing.

@@ +1801,5 @@
> +     if (!inputAddons.hasOwnProperty("schema")) {
> +       throw new Error("No schema specified.");
> +     }
> +
> +     schema = parseInt(inputAddons.schema, 10);

Shouldn't this already be a number (rather than a string that needs parsed)?

@@ +1810,5 @@
> +     }
> +
> +    } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
> +     LOG("No " + FILE_DATABASE + " found. A blank one will be created.");
> +     this.Writer.saveChanges();

It's a normal condition that there may not be a database file - AddonRepository will call AddonDatabase.delete() if it doesn't want any add-ons stored (saves I/O - was more important with SQLite, but IMO is still useful).

@@ +2034,5 @@
> +    get _deferredWriter() {
> +      delete this._deferredWriter;
> +      let DeferredTask = Cu.import("resource://gre/modules/DeferredTask.jsm", {})
> +                           .DeferredTask;
> +      return this._deferredWriter = new DeferredTask(this._writeToDisk.bind(this), 50);

Nit: No magic numbers; the 50 should be a const.

@@ +2059,5 @@
> +          } catch (e) {}
> +        }
> +
> +        Services.obs.notifyObservers(null,
> +                                     "addon-repository-data-written",

Nit: const.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 24 • 11 years ago

Comment on attachment 751293 [details] [diff] [review]
Lazy-loaded migrator module

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

(In reply to :Felipe Gomes from comment #17)
> I removed the tryAgain part of DB opening because in my understanding that
> was done not to try to read again, but to try to delete and unlock the DB to
> get access to a blank one again.

Yep.
 
> The old schema upgrade is kept to simplify the code to avoid the SELECT
> queries to fail, but the createIndices and createTriggers were removed.

Makes sense.

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +634,5 @@
> +  // Whether a migration in currently in progress
> +  _pendingMigration: false,
> +
> +  // A callback to be called when migration finishes
> +  _postMigrationCallback: null,

Feel like this should be more generic, associated with opening the DB async (which migration is just one part of). And allowing multiple callbacks to be handled once the DB is open, too (which is a general problem with AddonRepository - don't want to make it any worse).

@@ +1718,5 @@
>       }
>  
>      } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
> +      LOG("No " + FILE_DATABASE + " found. A blank one will be created.");
> +      this.Writer.saveChanges();

Ditto with a previous patch - an empty database shouldn't exist on disk.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 25 • 11 years ago

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #22)
> > The _make* functions are kept around in a dual mode (accepting rows as well)
> > because we can reuse the same function during normal operation and on the
> > code that will do the migration from SQL to JSON.
> 
> TBH, I'd like to avoid this reuse, and relegate all SQLite related code into
> the migration module. In addition to contaminating AddonRepository.jsm, it's
> going to cause issues when the fields change - requiring migration code in
> the _make* functions instead of the separate module.

Ok, I removed the "double" mode in those functions and will put the getResultByName version only in the migration code. I still kept the functions around instead of inlining them in the deserializer code because I tried and the code there looked too busy.

> >    this.icons = {};
> > +  this._remainingProperties = {};
> 
> "remaining" is a bit ambiguous. _unsupportedProperties, or something similar?

changed to _unsupportedProperties

> >     */
> >    get iconURL() {
> > +    return this.icons && this.icons[32];
> 
> Is this change needed? this.icons is guaranteed by the constructor to be an
> object.

no, removed. this was leftover from some debugging

> 
> @@ +567,5 @@
> > +            break;
> > +
> > +          case "creator":
> > +            json.creator = value ? value.name : "";
> > +            json.creatorURL = value ? value.url : "";
> 
> This will get verbose/awkward if we start adding more metadata to
> AddonAuthor - better to serialize it as an object (ie, {name, url}). And,
> for that matter, add a toJSON method to AddonAuthor in AddonManager.jsm.
> Ditto for AddonScreenshot and AddonCompatibilityOverride.

creatorURL seems to be a backwards-compatibility field that had no representation in the created object anyways, so it can actually go away. Now I serialize creator directly as the AddonAuthor obj.

I don't really need to add a toJSON method to any of those at the moment because they already serialize as expected! (since they're only composed of primitive types)

> > +    if (!aObj.id)
> > +      return;
> 
> Be intentional - return null.

done

> > +
> > +          case "iconURL":
> 
> Since this is just an alias for backwards compat, it shouldn't be
> serialized/deserialized.

Ok, done. I keep a blank case for it to avoid data from json to accidentally overwrite the getter, but let me know if you think it's unecessary


> 
> @@ +2607,5 @@
> >     * @param  aRow
> >     *         The asynchronous row to use
> >     * @return An object containing the size and URL of the icon
> >     */
> > +  _makeIcon: function AD_makeIconFromAsyncRow(aRow) {
> 
> Not used?

Not in this patch, but it was used by the migration code. But with the newer changes now it can be removed

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 26 • 11 years ago

Changed the implementation to use a Map (and will make it an array on disk)

I didn't change the usage of AddonRepository to a map too because:
- to keep the scope reduced here
- as you mentioned, I also think it'd be nice to remove the _addons cache entirely in the future, so i'll avoid changing something to be removed


With that we need to convert from map to array when returning the result, but that's simple

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 27 • 11 years ago

Comment on attachment 755189 [details] [diff] [review]
Reuse AddonSearchResult as main obj representation

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +2265,5 @@
> +    let id = aObj.id;
> +    if (!aObj.id)
> +      return null;
> +
> +    let addon = new AddonSearchResult(id);

Nit: Using |id| here but |aObj.id| directly above. May as well just use |aObj.id|.

@@ +2321,5 @@
> +            }
> +            break;
> +
> +          case "iconURL":
> +            break;

Nit: Better add a comment explaining this.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 28 • 11 years ago

Comment on attachment 755200 [details] [diff] [review]
AddonDatabase main operations

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1955,5 @@
>     * @param  aCallback
>     *         An optional callback to call once complete
>     */
>    repopulate: function AD_repopulate(aAddons, aCallback) {
> +    this.addonDB.addons.clear();

This may be crazy, even for me, but.... the alliteration of addonsDB.addons makes me want to rename 'addonsDB' to 'db'.

@@ +1987,5 @@
>     * @param  aCallback
>     *         The callback to call once complete
>     */
> +  _insertAddon: function AD__insertAddon(aAddon) {
> +    let newAddon = this._parseAddon(aAddon);

Under what situations is aAddon not already guaranteed to be a AddonSearchresult object?

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 29 • 11 years ago

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #23)
> > +
> > +  /**
> > +   * Synchronously opens a new connection to the database file.
> > +   */
> > +  openConnection: function() {
> 
> I assume you plan on making this async in the future? Cos, uh, this should
> totally be async.

Oh yep, I will do this next, but I want to keep this change in a separate bug to not mix this change here (going for as little as possible of a behavior change in this one)

> 
> @@ +1793,5 @@
> > +
> > +     inputAddons = JSON.parse(data);
> > +
> > +     if (!inputAddons.hasOwnProperty("addons") ||
> > +         typeof(inputAddons.addons) !== "object") {
> 
> I wonder if the in-memory representation should be a Map, while the on-disk
> representation is an array - since it'll save on bytes that need to be
> read/written, and we need to iterate over the data anyway when
> reading/writing.

Done. Now the AddonDatabase object itself is what's "written" to disk and it has a toJSON function that converts the on-disk Map to an array.

> 
> @@ +1801,5 @@
> > +     if (!inputAddons.hasOwnProperty("schema")) {
> > +       throw new Error("No schema specified.");
> > +     }
> > +
> > +     schema = parseInt(inputAddons.schema, 10);
> 
> Shouldn't this already be a number (rather than a string that needs parsed)?

Yes, it should. But, you know, just in case.. It doesn't hurt trying to parseInt an int, so it's just an extra safety.

> 
> @@ +1810,5 @@
> > +     }
> > +
> > +    } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
> > +     LOG("No " + FILE_DATABASE + " found. A blank one will be created.");
> > +     this.Writer.saveChanges();
> 
> It's a normal condition that there may not be a database file -
> AddonRepository will call AddonDatabase.delete() if it doesn't want any
> add-ons stored (saves I/O - was more important with SQLite, but IMO is still
> useful).

Sure, now saveChanges is only called from insertAddons, so it means that if the file doesn't exist, or .delete() was called, the DB will only be created when non-empty

> Nit: No magic numbers; the 50 should be a const.
> Nit: const.

Done

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 30 • 11 years ago

Moved _make*FromAsyncRow back to this module (and not trying to reuse it from AddonRepository).

I also re-added the _createIndices() and _createTriggers().  It's probably a very edge case where someone could have schema = 2, then moves to this version that would create the tables but not the indices/triggers, and then moves back to a version expecting them (e.g. ESR)..  The indices don't even matter, but the triggers do. Since the cost of keeping this existing code is close to 0 in favor of correctness, I re-added it.

This patch also includes the pref with the schema version to avoid multiple migrations as we discussed.

> ::: toolkit/mozapps/extensions/AddonRepository.jsm
> @@ +634,5 @@
> > +  // Whether a migration in currently in progress
> > +  _pendingMigration: false,
> > +
> > +  // A callback to be called when migration finishes
> > +  _postMigrationCallback: null,
> 
> Feel like this should be more generic, associated with opening the DB async
> (which migration is just one part of). And allowing multiple callbacks to be
> handled once the DB is open, too (which is a general problem with
> AddonRepository - don't want to make it any worse).

Yes, I will do this. But how do you feel if we do this in the follow-up bug to make openConnection fully async? I say because it will be a natural fit there, and it would actually make the code here unnecessarily contrived to handle the sync/async cases in a generic way, as opposed to this special handling for the migration case.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 31 • 11 years ago

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #28)
> ::: toolkit/mozapps/extensions/AddonRepository.jsm
> @@ +1955,5 @@
> >     * @param  aCallback
> >     *         An optional callback to call once complete
> >     */
> >    repopulate: function AD_repopulate(aAddons, aCallback) {
> > +    this.addonDB.addons.clear();
> 
> This may be crazy, even for me, but.... the alliteration of addonsDB.addons
> makes me want to rename 'addonsDB' to 'db'.

Will do. The patches I attached here don't include it yet, but I'll do a global replace when I put them together

> 
> @@ +1987,5 @@
> >     * @param  aCallback
> >     *         The callback to call once complete
> >     */
> > +  _insertAddon: function AD__insertAddon(aAddon) {
> > +    let newAddon = this._parseAddon(aAddon);
> 
> Under what situations is aAddon not already guaranteed to be a
> AddonSearchresult object?

When it is read from disk as a vanilla JS object from JSON or from the object created by the SQL migrator code. _parseAddon is then the function responsible from converting this serialized version to the real representation (e.g. converting the date string to a Date object).

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 32 • 11 years ago

(In reply to :Felipe Gomes from comment #30)
> Yes, I will do this. But how do you feel if we do this in the follow-up bug
> to make openConnection fully async?

WFM.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 33 • 11 years ago

Comment on attachment 756362 [details] [diff] [review]
AddonDatabase file access

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1959,5 @@
>        return this.openConnection();
>      });
>  
> +    if (aSkipFlush) {
> +      aCallback();

There's a possible race-condition here, if a flush-to-disk is already pending. The following API calls (if called either synchronously one after the other, or in quick succession) will result in unexpected/undefined behaviour:
1. AddonRepository.insertAddons(...)
2. AddonRepository.delete()

The flush could occur after the file has been deleted, resulting in the original full DB being re-written to disk.

Additionally, the following APIs calls will act unexpectedly:
1. AddonRepository.delete()
2. AddonRepository.insertAddons(...)

Because previously, _insertAddon would have forced the DB connection to re-open. That's no longer the case, so it will now insert into the stale in-memory copy of the DB and re-write that to disk.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 34 • 11 years ago

Comment on attachment 756365 [details] [diff] [review]
Lazy-loaded migrator module

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

::: toolkit/mozapps/extensions/AddonRepository_SQLiteMigrator.jsm
@@ +6,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");

Nit: May as well define Cu and use that in this file.

@@ +12,5 @@
> +Components.utils.import("resource://gre/modules/FileUtils.jsm");
> +
> +const KEY_PROFILEDIR  = "ProfD";
> +const FILE_DATABASE   = "addons.sqlite";
> +const LAST_DB_SCHEMA   = 4;

Nit: The last = doesn't line up.

@@ +51,5 @@
> +   *                        finishes, with the results in an array
> +   * @returns bool          True if a migration will happen (DB was
> +   *                        found and succesfully opened)
> +   */
> +  migrate: function(structFunctions, aCallback) {

Er, didn't you remove the structFunctions param?

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 35 • 11 years ago

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #33)
> There's a possible race-condition here, if a flush-to-disk is already
> pending. The following API calls (if called either synchronously one after
> the other, or in quick succession) will result in unexpected/undefined
> behaviour:
> 1. AddonRepository.insertAddons(...)
> 2. AddonRepository.delete()
> 
> The flush could occur after the file has been deleted, resulting in the
> original full DB being re-written to disk.
> 
> Additionally, the following APIs calls will act unexpectedly:
> 1. AddonRepository.delete()
> 2. AddonRepository.insertAddons(...)
> 
> Because previously, _insertAddon would have forced the DB connection to
> re-open. That's no longer the case, so it will now insert into the stale
> in-memory copy of the DB and re-write that to disk.


Thanks for catching those. I did the following:
 - Added a this.db = BLANK_DB() in delete.
 This clearly solves the second case of insertAddons writing to a stale DB in memory that has been deleted to disk.

 It also *almost* solves the first condition.. If the race happened it would at least correctly write a blank DB instead of a stale one. But we want to keep the condition of removing the file from disk, so I also added a flush that would force the batch timer to fire and start the pending write. If there was already a write in progress, it is ok because OS.File guarantees that everything that was queued through it completes in order (so the OS.File.delete will complete after the write does)

One extra thing is that I added the reopening of the DB in insertAddons in case it has been closed, to preserve previous behavior.

And I removed the Writer code from here and I'm now using the DeferredSave.jsm module from Irving's patch in bug 853388

	:Gijs (he/him)
	Comment 36 • 11 years ago

Does anything still need to happen here? Check in? :-)

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 37 • 11 years ago

Its dependant on bug 853388, and will need updated for recent changes there. And even if bug 853388 were done, neither are safe to land before the merge.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 38 • 11 years ago

these are the fixes that the tests needed to work with the new async behavior.  

On this patch you can also see the changes from the Writer callbacks to promises.

Two small things to mention:
 - I had to go back to the `this.icons && this.icons[32]` getter because on a normal object that works properly (as we initialize icons in the constructor), however the json parsing code iterates through the prototypes' properties, so it would throw an exception there

 - the changes from octal to decimal numbers was to shut up dozens of warnings printed during the tests saying that octals are deprecated

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 39 • 11 years ago

Two try runs, one with the tree that I was working with (a few days old), and one with the patch rebased to m-c tip:

https://tbpl.mozilla.org/?tree=Try&rev=5d300f389371
https://tbpl.mozilla.org/?tree=Try&rev=362edb0fee86

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 40 • 11 years ago

FWIW the patch posted is just the diff on top of the 4 main patches. If you want to see everything together take a look at the try push where I qfolded everything together: https://hg.mozilla.org/try/rev/ba22050ccf16

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 41 • 11 years ago

Comment on attachment 784175 [details] [diff] [review]
Fixes tests

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1581,5 @@
>      } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
>        LOG("No " + FILE_DATABASE + " found.");
> +      
> +      // Create a blank addons.json file
> +      this.Writer.saveChanges().then(this._reportDBWritten, Cu.reportError);

Ensuring this._reportDBWritten is always called when saveCbhannges() resolved seems error prone - should make a helper function to do this.

Also, should use ERROR instead of Cu.reportError.

Extra nit: trailing whitespace before this.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js
@@ +611,5 @@
>   */
>  function writeInstallRDFToDir(aData, aDir, aExtraFile) {
>    var rdf = createInstallRDF(aData);
>    if (!aDir.exists())
> +    aDir.create(AM_Ci.nsIFile.DIRECTORY_TYPE, 493); // 0755

Using decimals for this is rather opaque - use FileUtils.PERMS_DIRECTORY / FileUtils.PERMS_FILE

::: toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js
@@ +523,5 @@
>  
> +// Waits for the data to be written from the in-memory DB to the addons.json
> +// file that is done asynchronously through OS.File
> +function waitForFlushedData(aCallback) {
> +  do_test_pending();

Nit: If you're nesting do_test_pending() calls, pass in a name (should pass in the same name to the corresponding do_test_finished call too).

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 43 • 11 years ago

Comment on attachment 784198 [details] [diff] [review]
Fixes tests

(In reply to Blair McBride [:Unfocused] from comment #41)
> Comment on attachment 784175 [details] [diff] [review]
> Fixes tests
> 
> Review of attachment 784175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/AddonRepository.jsm
> @@ +1581,5 @@
> >      } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
> >        LOG("No " + FILE_DATABASE + " found.");
> > +      
> > +      // Create a blank addons.json file
> > +      this.Writer.saveChanges().then(this._reportDBWritten, Cu.reportError);
> 
> Ensuring this._reportDBWritten is always called when saveCbhannges()
> resolved seems error prone - should make a helper function to do this.
> 
> Also, should use ERROR instead of Cu.reportError.
> 
> Extra nit: trailing whitespace before this.

Done. I realized that I wouldn't need the _reportDBWritten on flush(), because it always needs to be preceded by a saveChanges(), so this was simple.

> 
> ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js
> @@ +611,5 @@
> >   */
> >  function writeInstallRDFToDir(aData, aDir, aExtraFile) {
> >    var rdf = createInstallRDF(aData);
> >    if (!aDir.exists())
> > +    aDir.create(AM_Ci.nsIFile.DIRECTORY_TYPE, 493); // 0755
> 
> Using decimals for this is rather opaque - use FileUtils.PERMS_DIRECTORY /
> FileUtils.PERMS_FILE

done

> 
> ::: toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js
> @@ +523,5 @@
> >  
> > +// Waits for the data to be written from the in-memory DB to the addons.json
> > +// file that is done asynchronously through OS.File
> > +function waitForFlushedData(aCallback) {
> > +  do_test_pending();
> 
> Nit: If you're nesting do_test_pending() calls, pass in a name (should pass
> in the same name to the corresponding do_test_finished call too).

I don't really need this nesting, I think. I added this while debugging the test to see if it helped, but I'm pretty sure it makes no difference, as there's nothing else calling do_test_finished until the last test. Removed

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 44 • 11 years ago

Since this last remaining patch:
 - is simple and only fixes small things that are necessary for tests only,
 - addresses all the comments on the previous review pass for it,
 - is green on try

I took the liberty of landing it with pending review. Apologies if it shouldn't have landed. If there are extra things that come up on review I'll promptly address them or backout the patch if it's not possible to do it quickly.

https://hg.mozilla.org/integration/fx-team/rev/57ee510e991e

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/57ee510e991e

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 46 • 11 years ago

Comment on attachment 784198 [details] [diff] [review]
Fixes tests

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

Yep, n/p. Meant to get to it right away last night, but was too tired.