Ben Parr [:bparr] Assignee
	Description • 14 years ago

Right now only remote add-ons have the new properties available from AMO (e.g. suggested contribution data). Installed add-ons also need to access to this data.

	Ben Parr [:bparr] Assignee
	Comment 1 • 14 years ago

Creates an SQLite database (addons.sqlite) inside the Firefox profile to store the AMO data. Update the database when background updates occur (once a day by default). Also updates providers, including changes to XPIProvider so that it actually exposes the AMO data.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 2 • 14 years ago

Just to be curious. Why do we need another sqlite file? Can't we handle those information in extensions.sqlite?

	Dave Townsend [:mossop]
	Comment 3 • 14 years ago

(In reply to comment #2)
> Just to be curious. Why do we need another sqlite file? Can't we handle those
> information in extensions.sqlite?

We'd need to make the extensions database shared to do that which I don't want to do as well as have a much harder time handling schema migrations between the two different users of the database.

	Dave Townsend [:mossop]
	Comment 4 • 14 years ago

Comment on attachment 465158 [details] [diff] [review]
Patch and Test

This is some great work, really close to being ready to land, just a few points that could do with being corrected.

>   backgroundUpdateCheck: function AMI_backgroundUpdateCheck() {
>     if (!Services.prefs.getBoolPref(PREF_EM_UPDATE_ENABLED))
>       return;
> 
>+    Services.obs.notifyObservers(null, "addons-background-update-start", null);
>+    let pendingUpdates = 1;
>+
>+    function notifyComplete() {
>+      if (--pendingUpdates == 0)
>+        Services.obs.notifyObservers(null, "addons-background-update-complete", null);
>+    }
>+
>     let scope = {};
>+    Components.utils.import("resource://gre/modules/AddonRepository.jsm", scope);
>     Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", scope);
>     scope.LightweightThemeManager.updateCurrentTheme();
> 
>     this.getAllAddons(function getAddonsCallback(aAddons) {
>+      if (scope.AddonRepository.HAS_CACHING) {
>+        pendingUpdates++;
>+        var ids = [a.id for each (a in aAddons)];
>+        scope.AddonRepository.repopulateCache(ids, notifyComplete);
>+      }
>+
>+      pendingUpdates += aAddons.length;
>       aAddons.forEach(function BUC_forEachCallback(aAddon) {
>         // Check all add-ons for updates so that any compatibility updates will
>         // be applied
>         aAddon.findUpdates({
>           onUpdateAvailable: function BUC_onUpdateAvailable(aAddon, aInstall) {
>             // Start installing updates when the add-on can be updated and
>             // background updates should be applied.
>             if (aAddon.permissions & AddonManager.PERM_CAN_UPGRADE &&
>                 aAddon.applyBackgroundUpdates) {
>               aInstall.install();
>             }
>-          }
>+          },
>+
>+          onUpdateFinished: notifyComplete
>         }, AddonManager.UPDATE_WHEN_PERIODIC_UPDATE);

You need to increment pendingUpdates for each add-on you start an update check for.

>diff --git a/toolkit/mozapps/extensions/AddonRepository.jsm b/toolkit/mozapps/extensions/AddonRepository.jsm

> var AddonRepository = {
>+  /**
>+   * Whether the Addon Repository supports caching repository results
>+   */
>+  HAS_CACHING: true,

This isn't really needed. From the other side just test |if ("repopulateCache" in AddonRepository)|

>+  shutdown: function() {
>+    this.cancelSearch();
>+
>+    this._addons = null;
>+    this._pendingCallbacks = null;
>+    AddonDatabase.shutdown(function() {
>+      Services.obs.notifyObservers(null, "addon-repository-shutdown", null);
>+    });
>+  },

Please just do this from the observer service xpcom-shutdown topic rather than from AddonManager.jsm or we could introduce a shutdown time regression. You'll need to make the tests call it all the time probably though.

>+  /**
>+   * Asynchronously get a cached add-on by id. The add-on (or null if the
>+   * add-on is not found) is passed to the specified callback. If caching is
>+   * disabled, null is passed to the specified callback.
>+   *
>+   * @param  aId
>+   *         The id of the add-on to get
>+   * @param  aCallback
>+   *         The callback to pass the result back to
>+   */
>+  getCachedAddonByID: function(aId, aCallback) {
>+    if (!aId || !this.cacheEnabled) {
>+      aCallback(null);
>+      return;
>+    }
>+
>+    let self = this;
>+    function getAddon(aAddons) {
>+      aCallback((aId in aAddons) ? aAddons[aId] : null);
>+    }
>+
>+    if (this._addons == null) {
>+      if (this._pendingCallbacks == null) {
>+        // Data has not been retrieved from the database, so retrieve it
>+        this._pendingCallbacks = [];
>+        this._pendingCallbacks.push(getAddon);
>+        AddonDatabase.retrieveStoredData(function(aAddons) {
>+          let pendingCallbacks = self._pendingCallbacks;
>+
>+          // Check if cache was shutdown or deleted before callback was called
>+          if (pendingCallbacks == null)
>+            return;
>+
>+          // Callbacks may want to trigger a other caching operations that may
>+          // affect _addons and _pendingCallbacks, so set to final values early
>+          self._pendingCallbacks = null;
>+          self._addons = aAddons;
>+
>+          pendingCallbacks.forEach(function(aCallback) aCallback(aAddons));
>+        });

This wasn't quite what I was expecting. I think I'd rather we only pull the data for the add-on's we're requesting and hold them weakly. However I think this is good enough for now and we'll probably consider whether it is worth fixing that in a follow-up bug.

>+  repopulateCache: function(aIds, aCallback) {

>+  cacheAddons: function(aIds, aCallback) {

I'm not sure why we have both of these functions. I hope the latter doesn't allow you to add multiple copies of the same add-on to the database, in which case it is really only doing the same as the former, I guess without wiping out the rest of the data first. When will we need the cacheAddons function?


>             case 2:
>               addon.type = "theme";
>               break;
>             default:
>-              Cu.reportError("Unknown type id when parsing addon: " + id);
>+              ERROR("Unknown type id when parsing addon: " + id);

This should just use WARN. ERROR is really only for serious problems that need to be logged to disk and so should be used sparingly.

>   // Create url from preference, returning null if preference does not exist
>   _formatURLPref: function(aPreference, aSubstitutions) {
>     let url = null;
>     try {
>       url = Services.prefs.getCharPref(aPreference);
>     } catch(e) {
>-      Cu.reportError("_formatURLPref: Couldn't get pref: " + aPreference);
>+      ERROR("_formatURLPref: Couldn't get pref: " + aPreference);

WARN again I think.

>+  openConnection: function AD_openConnection(aSecondAttempt) {
>+    this.initialized = true;
>+    delete this.connection;
>+
>+    let dbfile = FileUtils.getFile(KEY_PROFILEDIR, [FILE_DATABASE], true);
>+    let dbMissing = !dbfile.exists();
>+
>+    try {
>+      this.connection = Services.storage.openUnsharedDatabase(dbfile);
>+    } catch (e) {
>+      ERROR("Failed to open database: " + e);
>+      if (aSecondAttempt || dbMissing)
>+        throw e;

Might want to set initialized to false here, maybe also make it so cacheEnabled is false afterwards.

>+
>+      LOG("Deleting database, and attempting openConnection again");
>+      dbfile.remove(false);
>+      return this.openConnection(true);
>+    }
>+
>+    this.connection.executeSimpleSQL("PRAGMA locking_mode = EXCLUSIVE");
>+    if (dbMissing || this.connection.schemaVersion == 0)
>+      this._createSchema();

schemaVersion should be 0 if the db didn't exist so that check is unnecessary. You should also add a little code that detects if the schemaVersion is not DB_SCHEMA and deletes and re-creates it. I don't really care about migrating the data right now, it will get rebuilt within a day.

>+  repopulate: function AD_repopulate(aAddons, aCallback) {
>+    let self = this;
>+
>+    // Completely empty the database
>+    let stmts = [this.getStatement("emptyAddon"),
>+                 this.getStatement("emptyDeveloper"),
>+                 this.getStatement("emptyScreenshot")];

You only need to clear the addon table, the other tables will get automatically cleared by the trigger you have set up.

>+  _insertAddon: function AD__insertAddon(aAddon, aCallback) {
>+    let self = this;
>+    let internal_id = null;

Start a transaction here and then roll it back if any of the insertions for this add-on fail and commit it if they all succeed. That way we won't end up with any partial data for an add-on in the db.

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm

> function AddonWrapper(aAddon) {
>+  function chooseValue(aObj, aProp) {
>+    let undefined;

undefined is not a great name for a variable. Not really sure what this is here for either.

>+    let repositoryAddon = aAddon._repositoryAddon;
>+    let objValue = aObj[aProp];
>+
>+    if (repositoryAddon && (aProp in repositoryAddon))
>+      if (objValue === undefined || objValue === null)
>+        return [repositoryAddon[aProp], true];
>+
>+    return [objValue, false];
>+  }

I kind of don't like this function, but can't think of a nicer alternative right now.

>   PROP_LOCALE_SINGLE.forEach(function(aProp) {
>     this.__defineGetter__(aProp, function() {
>+      // Override XPI creator if repository creator is defined
>+      if (aProp == "creator")
>+        if (aAddon._repositoryAddon && aAddon._repositoryAddon.creator)
>+          return aAddon._repositoryAddon.creator;
>+
>+      let result = null;
>+      let usedRepository = false;
>+
>       if (aAddon.active) {
>         try {
>           let pref = PREF_EM_EXTENSION_FORMAT + aAddon.id + "." + aProp;
>           let value = Services.prefs.getComplexValue(pref,
>                                                      Ci.nsIPrefLocalizedString);
>           if (value.data)
>-            return value.data;
>+            result = value.data;
>         }
>         catch (e) {
>         }
>       }
>-      return aAddon.selectedLocale[aProp];
>+
>+      if (result == null)
>+        [result, usedRepository] = chooseValue(aAddon.selectedLocale, aProp);

You can just do |[result, ] = | and not declare usedRepository at all.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js

>+function do_check_addon(aActualAddon, aExpectedAddon, aProperties) {
>+  let undefined;

What is it with defining undefined as a variable? I'm kind of surprised that it doesn't fail tbh.

	Ben Parr [:bparr] Assignee
	Comment 5 • 14 years ago

(In reply to comment #4)
> >+
> >+      pendingUpdates += aAddons.length;
> >       aAddons.forEach(function BUC_forEachCallback(aAddon) {
> 
> You need to increment pendingUpdates for each add-on you start an update check
> for.
> 

I assume this is what you are talking about and just missed that line(?).

> >+  shutdown: function() {
> >+    this.cancelSearch();
> >+
> >+    this._addons = null;
> >+    this._pendingCallbacks = null;
> >+    AddonDatabase.shutdown(function() {
> >+      Services.obs.notifyObservers(null, "addon-repository-shutdown", null);
> >+    });
> >+  },
> 
> Please just do this from the observer service xpcom-shutdown topic rather than
> from AddonManager.jsm or we could introduce a shutdown time regression. You'll
> need to make the tests call it all the time probably though.
> 

This request is unclear (at least to me).

> >+  getCachedAddonByID: function(aId, aCallback) {
> 
> This wasn't quite what I was expecting. I think I'd rather we only pull the
> data for the add-on's we're requesting and hold them weakly. However I think
> this is good enough for now and we'll probably consider whether it is worth
> fixing that in a follow-up bug.
> 

Hmm. I must have misunderstood from one of our meetings.

> >+  repopulateCache: function(aIds, aCallback) {
> 
> >+  cacheAddons: function(aIds, aCallback) {
> 
> I'm not sure why we have both of these functions. I hope the latter doesn't
> allow you to add multiple copies of the same add-on to the database, in which
> case it is really only doing the same as the former, I guess without wiping out
> the rest of the data first. When will we need the cacheAddons function?
> 

repopulateCache is meant to be used within the background update. cacheAddons is for adding a list of add-ons to the cache after the fact. If the add-on is already in the cache, there will be an ERROR(). The idea of cacheAddons is to use it when an add-on is installed and being added to the repository cache.

> schemaVersion should be 0 if the db didn't exist so that check is unnecessary.

If _createSchema fails, wouldn't the schemaVersion be 0 next time with the db existing? Might want to consider a better handling of this case though.

> undefined is not a great name for a variable. Not really sure what this is here
> for either.
 
> What is it with defining undefined as a variable? I'm kind of surprised that it
> doesn't fail tbh.

Hmm. I guess it's unneccessary code. The problem it tries to fix is when `undefined` is defined to be something somewhere else in the code (`let undefined = 1`). Then it's possible for `if (value === undefined)` to not work as expected because it will be checking if value is equal to 1 instead of checking if value is in fact not defined. Given the location of the code though, this shouldn't be a problem.



I agree with everything else in comment #4.

	Dave Townsend [:mossop]
	Comment 6 • 14 years ago

(In reply to comment #5)
> (In reply to comment #4)
> > >+
> > >+      pendingUpdates += aAddons.length;
> > >       aAddons.forEach(function BUC_forEachCallback(aAddon) {
> > 
> > You need to increment pendingUpdates for each add-on you start an update check
> > for.
> > 
> 
> I assume this is what you are talking about and just missed that line(?).

Yeah I must have missed that.

> > >+  shutdown: function() {
> > >+    this.cancelSearch();
> > >+
> > >+    this._addons = null;
> > >+    this._pendingCallbacks = null;
> > >+    AddonDatabase.shutdown(function() {
> > >+      Services.obs.notifyObservers(null, "addon-repository-shutdown", null);
> > >+    });
> > >+  },
> > 
> > Please just do this from the observer service xpcom-shutdown topic rather than
> > from AddonManager.jsm or we could introduce a shutdown time regression. You'll
> > need to make the tests call it all the time probably though.
> > 
> 
> This request is unclear (at least to me).

Rather than having AddonManager call shutdown on AddonRepository instead the AddonRepository.jsm should just register an observer for xpcom-shutdown and shutdown itself then. It keeps it more self-contained.

> > >+  repopulateCache: function(aIds, aCallback) {
> > 
> > >+  cacheAddons: function(aIds, aCallback) {
> > 
> > I'm not sure why we have both of these functions. I hope the latter doesn't
> > allow you to add multiple copies of the same add-on to the database, in which
> > case it is really only doing the same as the former, I guess without wiping out
> > the rest of the data first. When will we need the cacheAddons function?
> > 
> 
> repopulateCache is meant to be used within the background update. cacheAddons
> is for adding a list of add-ons to the cache after the fact. If the add-on is
> already in the cache, there will be an ERROR(). The idea of cacheAddons is to
> use it when an add-on is installed and being added to the repository cache.

Fair enough, makes sense.

> > schemaVersion should be 0 if the db didn't exist so that check is unnecessary.
> 
> If _createSchema fails, wouldn't the schemaVersion be 0 next time with the db
> existing? Might want to consider a better handling of this case though.

This is true, I'm ok with that as it is then.

> > undefined is not a great name for a variable. Not really sure what this is here
> > for either.
> 
> > What is it with defining undefined as a variable? I'm kind of surprised that it
> > doesn't fail tbh.
> 
> Hmm. I guess it's unneccessary code. The problem it tries to fix is when
> `undefined` is defined to be something somewhere else in the code (`let
> undefined = 1`). Then it's possible for `if (value === undefined)` to not work
> as expected because it will be checking if value is equal to 1 instead of
> checking if value is in fact not defined. Given the location of the code
> though, this shouldn't be a problem.

I believe that the new JS changes make it impossible to redefine undefined now, but either way we should never be redefining undefined in Firefox JS code, if there is a case where we are then it should be filed as a bug and fixed.

	Dave Townsend [:mossop]
	Comment 7 • 14 years ago

Blair, any chance you could update this patch from the review comments so it can be ready to land when the AMO side is done?

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

Fixes review comments. Also corrects the test data files that had some incorrect values.


(In reply to comment #4)
> >+  repopulate: function AD_repopulate(aAddons, aCallback) {
> >+    let self = this;
> >+
> >+    // Completely empty the database
> >+    let stmts = [this.getStatement("emptyAddon"),
> >+                 this.getStatement("emptyDeveloper"),
> >+                 this.getStatement("emptyScreenshot")];
> 
> You only need to clear the addon table, the other tables will get automatically
> cleared by the trigger you have set up.

Somewhat ironically, this is actually the downside of triggers. If there were no trigger, we could just truncate the tables - which is hugely faster than deleting row-by-row and running a trigger on each of those rows. But that doesn't work if there's a trigger on the table.


> This wasn't quite what I was expecting. I think I'd rather we only pull the
> data for the add-on's we're requesting and hold them weakly. However I think
> this is good enough for now and we'll probably consider whether it is worth
> fixing that in a follow-up bug.

Filed bug 588249 for this. I have a patch in progress for that, but its taking longer than expected (test failures), so I'll try to finish it up sometime after beta 5 (if I get time).

	Dave Townsend [:mossop]
	Comment 9 • 14 years ago

Comment on attachment 466882 [details] [diff] [review]
Patch v2

Looks good. I'll land this along with the dependent bug on Thursday so that things should be working with that nightly.

	Dave Townsend [:mossop]
	Comment 10 • 14 years ago

Landed, should be working when AMO updates later this afternoon.

http://hg.mozilla.org/mozilla-central/rev/937a9da47469

	Dave Townsend [:mossop]
	Comment 11 • 14 years ago

Will probably need to think about some automated tests here, we can test individual elements automatically but the whole lot depends on AMO etc.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 12 • 14 years ago

Verified fixed with builds on all platforms like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100922 Firefox/4.0b7pre

I will have to wait with the creation of any litmus test until bug 595915 has been fixed. In the current state we can't enable those tests.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 13 • 14 years ago

Litmus test has been added:
https://litmus.mozilla.org/show_test.cgi?id=15150

	Ben Bucksch (:BenB)
	Comment 14 • 12 years ago

This bug should have never have been done and is broken by design.

The information in install.rdf matches exactly the addon that the user installed and confirmed the install for. Thus, this is what must be listed in the "installed extensions" dialog. Information on AMO may refer to other version, other extensions or other authors altogether. You MUST NOT replace information about local addons with info from AMO.

We had exactly that case now: Distributing an addon, in several variants, with official Mozilla approval, but on channels other than AMO. Now, we're in the process of adding it on AMO, but the process hasn't finished yet, and the information on AMO is still stubs. Nevertheless, Firefox already replaces the info on live Firefox installations (we have a few million installations) with the stub info. This was overwriting even the author and thus the copyright information. This means Mozilla removes the copyright from already installed addons and replaces it with another one. This must NEVER EVER happen. It's a copyright violation.

You must not touch other people's products, be it name, description, author or anything. The fact that you *think* the AMO ext author is the author of the installed extension does not matter.