555349 - Finalize the restartless add-on format

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Dave Townsend [:mossop]]

A couple of things are striking me as incorrect about the current form of restartless add-ons. These should be fixed before we land on trunk.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This addresses the basics of the new spec. It is slightly wrong in that the install method will get called even when the add-on is installed disabled, but I think that this is good enough for trunk and I've filed bug 560608 to fix that afterward.

Comment 2

[Dave Townsend [:mossop]]

Fixed in http://hg.mozilla.org/projects/addonsmgr/rev/ff12ff293569

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Had to make a couple of small changes to this as I was testing locally with the crash reporter disabled. The only changes are:

http://hg.mozilla.org/projects/addonsmgr/rev/78f54b9bf8ca
http://hg.mozilla.org/projects/addonsmgr/rev/439285fc775d

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 440601 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -96,28 +96,36 @@ const PREFIX_NS_EM                    = 
> 
> const TOOLKIT_ID                      = "[e-mail]";
> 
> const BRANCH_REGEXP                   = /^([^\.]+\.[0-9]+[a-z]*).*/gi;
> 
> const DB_SCHEMA                       = 1;
> const REQ_VERSION                     = 2;
> 
>+const APP_STARTUP                     = 1;
>+const APP_SHUTDOWN                    = 2;
>+const ADDON_ENABLE                    = 3;
>+const ADDON_DISABLE                   = 4;
>+const ADDON_INSTALL                   = 5;
>+const ADDON_UNINSTALL                 = 6;
>+const ADDON_UPGRADE                   = 7;
>+const ADDON_DOWNGRADE                 = 8;
>+

>@@ -1886,165 +1901,158 @@ var XPIProvider = {
>...
>+  /**
>+   * Loads a bootstrapped add-on's bootstrap.js into a sandbox and adds the
>+   * constants to it. This will also add information about the add-on to the
>+   * bootstrappedAddons dictionary and notify the crash reporter that new
>+   * add-ons have been loaded.
nit: "adds the constants to it" leaves me wanting to know what constants these are. Could you please make this more specific?

>+   *
>+   * @param  aId
>+   *         The add-on's ID
>+   * @param  aDir
>+   *         The add-on's install directory
nit: "The nsILocalFile for the directory containing the add-on" or something similar.

>    * @param  aVersion
>-   *         The version of the add-on being activated
>-   * @param  aDir
>-   *         The directory containing the add-on
>-   * @param  aStartup
>-   *         true if the add-on is being activated during startup
>-   * @param  aInstall
>-   *         true if the add-on is being activated during installation
>-   */
>-  activateAddon: function XPI_activateAddon(aId, aVersion, aDir, aStartup, aInstall) {
>-    let methods = ["enable"];
>-    if (aStartup)
>-      methods.unshift("startup");
>-    if (aInstall)
>-      methods.unshift("install");
>+   *         The add-on's version
>+   * @return a JavaScript scope
>+   */
>+  loadBootstrapScope: function XPI_loadBootstrapScope(aId, aDir, aVersion) {
>+    LOG("Loading bootstrap scope from " + aDir.path);
>+    // Mark the add-on as active for the crash reporter before loading
>+    this.bootstrappedAddons[aId] = {
>+      version: aVersion,
>+      descriptor: aDir.persistentDescriptor
>+    };
>+    this.addAddonsToCrashReporter();
>+
>+    let principal = Cc["@mozilla.org/systemprincipal;1"].
>+                    createInstance(Ci.nsIPrincipal);
>+    this.bootstrapScopes[aId] = new Components.utils.Sandbox(principal);
>+
>     let bootstrap = aDir.clone();
>     bootstrap.append("bootstrap.js");
>     if (bootstrap.exists()) {
>       let uri = Services.io.newFileURI(bootstrap);
>-      let principal = Cc["@mozilla.org/systemprincipal;1"].
>-                      createInstance(Ci.nsIPrincipal);
>-      let scope = new Components.utils.Sandbox(principal);
>       let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
>                    createInstance(Ci.mozIJSSubScriptLoader);
> 
>-      // Mark the add-on as active for the crash reporter before loading
>-      this.bootstrappedAddons[aId] = {
>-        version: aVersion,
>-        descriptor: aDir.persistentDescriptor
>-      };
>-      this.bootstrapScopes[aId] = scope;
>-      this.addAddonsToCrashReporter();
>-
>-      try {
>-        loader.loadSubScript(uri.spec, scope);
>+      try {
>+        loader.loadSubScript(uri.spec, this.bootstrapScopes[aId]);
>       }
>       catch (e) {
>         WARN("Error loading bootstrap.js for " + aId + ": " + e);
>       }
> 
>-      this.callBootstrapMethod(aId, scope, methods);
>+      let consts = ["APP_STARTUP", "APP_SHUTDOWN", "ADDON_ENABLE",
>+                    "ADDON_DISABLE","ADDON_INSTALLED", "ADDON_UNINSTALLED",
>+                    "ADDON_UPGRADED", "ADDON_DOWNGRADED"];
>+      for (let i = 0; i < consts.length; i++)
>+        this.bootstrapScopes[aId][consts[i]] = i;
This is ugly. Not sure of a better solution but I suspect there is one.

>     }
>     else {
>       WARN("Bootstrap missing for " + aId);
>     }
>   },
> 
>   /**
>-   * Dectivates a bootstrapped add-on by by calling the appropriate method on
>-   * the cached JS scope. Removes the add-on and its scope from the
>-   * bootstrapScopes and bootstrappedAddons dictionaries.
>-   *
>-   * @param  aId
>-   *         The ID of the add-on being deactivated
>-   * @param  aShutdown
>-   *         true if the add-on is being deactivated during shutdown
>-   * @param  aUninstall
>-   *         true if the add-on is being deactivated during uninstallation
>-   */
>-  deactivateAddon: function XPI_deactivateAddon(aId, aShutdown, aUninstall) {
>-    if (!(aId in this.bootstrappedAddons)) {
>-      ERROR("Attempted to deactivate an add-on that was never activated");
>-      return;
>-    }
>-    let scope = this.bootstrapScopes[aId];
>+   * Unloads a bootstrap scope by dropping all references to it and then
>+   * updating the list of active add-ons with the crash reporter.
>+   *
>+   * @param  aId
>+   *         The add-on's ID
>+   */
>+  unloadBootstrapScope: function XPI_unloadBootstrapScope(aId) {
>+    delete this.bootstrapScopes[aId];
>     delete this.bootstrappedAddons[aId];
>-    delete this.bootstrapScopes[aId];
>-
>-    let methods = ["disable"];
>-    if (aShutdown)
>-      methods.unshift("shutdown");
>-    if (aUninstall)
>-      methods.unshift("uninstall");
>-    this.callBootstrapMethod(aId, scope, methods);
>-
>     this.addAddonsToCrashReporter();
>   },
> 
>   /**
>+   * Calls a bootstrap method for an add-on.
>+   *
>+   * @param  aId
>+   *         The ID of the add-on
>+   * @param  aVersion
>+   *         The version of the add-on
>+   * @param  aDir
>+   *         The directory containing the add-on
nit: "The nsILocalFile for the directory containing the add-on" or something similar. This should be consistent with the previous aDir comment.

>+   * @param  aMethod
>+   *         The name of the bootstrap method to call
>+   * @param  aReason
>+   *         The reason flag to pass to the bootstrap's startup method
>+   */
>+  callBootstrapMethod: function XPI_callBootstrapMethod(aId, aVersion, aDir,
>+                                                        aMethod, aReason) {
>+    // Load the scope if it hasn't already been loaded
>+    if (!(aId in this.bootstrapScopes))
>+      this.loadBootstrapScope(aId, aDir, aVersion);
>+
>+    let params = {
>+      id: aId,
>+      version: aVersion,
>+      installPath: aDir.clone()
>+    };
nit: put this after the early return below

>+
>+    if (!(aMethod in this.bootstrapScopes[aId])) {
>+      WARN("Add-on " + aId + " is missing bootstrap method " + aMethod);
>+      return;
>+    }
>+
>+    LOG("Calling bootstrap method " + aMethod + " on " + aId + " version " + aVersion);
>+    try {
>+      this.bootstrapScopes[aId][aMethod](params, aReason);
>+    }
>+    catch (e) {
>+      WARN("Exception running bootstrap method " + aMethods + " on " +
>+           aId + ": " + e);
>+    }
>+  },
>+
>+  /**
>    * Updates the appDisabled property for all add-ons.
>    */
>   updateAllAddonDisabledStates: function XPI_updateAllAddonDisabledStates() {
>     let addons = XPIDatabase.getAddons();
>     addons.forEach(function(aAddon) {
>       this.updateAddonDisabledState(aAddon);
>     }, this);
>   },

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 440601 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -2613,17 +2634,17 @@ var XPIDatabase = {
>     addon._internal_id = aRow.internal_id;
>     addon._installLocation = XPIProvider.installLocationsByName[aRow.location];
>     addon._descriptor = aRow.descriptor;
>     copyProperties(aRow, PROP_METADATA, addon);
>     addon._defaultLocale = aRow.defaultLocale;
>     addon.installDate = aRow.installDate;
>     addon.updateDate = aRow.updateDate;
>     ["visible", "active", "userDisabled", "appDisabled",
>-     "pendingUninstall", "applyBackgroundUpdates"].forEach(function(aProp) {
>+     "pendingUninstall", "applyBackgroundUpdates", "bootstrap"].forEach(function(aProp) {
nit: move "bootstrap" to the next line

>       addon[aProp] = aRow[aProp] != 0;
>     });
>     this.addonCache[aRow.internal_id] = Components.utils.getWeakReference(addon);
>     return addon;
>   },
> 
>   /**
>    * Asynchronously fetches additional metadata for a DBAddonInternal.
>@@ -2763,17 +2784,17 @@ var XPIDatabase = {
>     let location = aRow.getResultByName("location");
>     addon._installLocation = XPIProvider.installLocationsByName[location];
>     addon._descriptor = aRow.getResultByName("descriptor");
>     copyRowProperties(aRow, PROP_METADATA, addon);
>     addon._defaultLocale = aRow.getResultByName("defaultLocale");
>     addon.installDate = aRow.getResultByName("installDate");
>     addon.updateDate = aRow.getResultByName("updateDate");
>     ["visible", "active", "userDisabled", "appDisabled",
>-     "pendingUninstall", "applyBackgroundUpdates"].forEach(function(aProp) {
>+     "pendingUninstall", "applyBackgroundUpdates", "bootstrap"].forEach(function(aProp) {
nit: move "bootstrap" to the next line

>       addon[aProp] = aRow.getResultByName(aProp) != 0;
>     });
>     this.addonCache[internal_id] = Components.utils.getWeakReference(addon);
>     return addon;
>   },
> 
>   /**
>    * Synchronously reads all install locations known about by the database. This
>@@ -3038,17 +3059,17 @@ var XPIDatabase = {
> 
>     stmt.params.locale = insertLocale(aAddon.defaultLocale);
>     stmt.params.location = aAddon._installLocation.name;
>     stmt.params.descriptor = aDescriptor;
>     stmt.params.installDate = aAddon.installDate;
>     stmt.params.updateDate = aAddon.updateDate;
>     copyProperties(aAddon, PROP_METADATA, stmt.params);
>     ["visible", "userDisabled", "appDisabled",
>-     "applyBackgroundUpdates"].forEach(function(aProp) {
>+     "applyBackgroundUpdates", "bootstrap"].forEach(function(aProp) {
nit: put applyBackgroundUpdates on the previous line

>       stmt.params[aProp] = aAddon[aProp] ? 1 : 0;
>     });
>     stmt.params.active = (aAddon.visible && !aAddon.userDisabled &&
>                           !aAddon.appDisabled) ? 1 : 0;
>     stmt.execute();
>     let internal_id = this.connection.lastInsertRowID;
> 
>     stmt = this.getStatement("addAddonMetadata_addon_locale");

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 440601 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/bootstrap.js b/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/bootstrap.js
>--- a/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/bootstrap.js
>+++ b/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/bootstrap.js
>@@ -1,9 +1,19 @@
> Components.utils.import("resource://gre/modules/Services.jsm");
> 
>-function enable() {
>-  Services.prefs.setIntPref("bootstraptest.version", 1);
>+function install(data, reason) {
>+  Services.prefs.setIntPref("bootstraptest.install", 1);
> }
Per in person discussion please use bootstraptest.version here and elsewhere

> 
>-function disable() {
>+function startup(data, reason) {
>+  Services.prefs.setIntPref("bootstraptest.version", 1);
>+  Services.prefs.setIntPref("bootstraptest.startup", reason);
>+}
>+
>+function shutdown(data, reason) {
>   Services.prefs.setIntPref("bootstraptest.version", 0);
>+  Services.prefs.setIntPref("bootstraptest.shutdown", reason);
> }
>+
>+function uninstall(data, reason) {
>+  Services.prefs.setIntPref("bootstraptest.install", 0);
>+}
Per in person discussion please use bootstraptest.version... I think?

>diff --git a/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/install.rdf b/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/install.rdf
>--- a/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/install.rdf
>+++ b/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/install.rdf
>@@ -1,17 +1,17 @@
> <?xml version="1.0"?>
> 
> <RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>      xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> 
>   <Description about="urn:mozilla:install-manifest">
>     <em:id>[e-mail]</em:id>
>     <em:version>1.0</em:version>
>-    <em:type>64</em:type>
>+    <em:bootstrap>true</em:bootstrap>
nit: add the type for extension here and elsewhere.

> 
>     <!-- Front End MetaData -->
>     <em:name>Test Bootstrap 1</em:name>
>     <em:description>Test Description</em:description>
> 
>     <em:targetApplication>
>       <Description>
>         <em:id>[e-mail]</em:id>

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 440601 [details] [diff] [review]
patch rev 2

r=me with nits fixed. If you come up with a better way to deal with adding the constants either request review in this bug or a followup bug... I don't think making that better should hold this up.

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This addresses the nits but I have done a slight better way (I think) of passing hte constants through to the bootstrap scope and also changes the preferences used to check the bootstrap script was called correctly.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 4

From the discussion IRL, put the codes into a single global object.

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Attached the right patch this time

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 441904 [details] [diff] [review]
patch rev 4

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -1886,165 +1905,156 @@ var XPIProvider = {
>...
>+  loadBootstrapScope: function XPI_loadBootstrapScope(aId, aDir, aVersion) {
>+    LOG("Loading bootstrap scope from " + aDir.path);
>+    // Mark the add-on as active for the crash reporter before loading
>+    this.bootstrappedAddons[aId] = {
>+      version: aVersion,
>+      descriptor: aDir.persistentDescriptor
>+    };
>+    this.addAddonsToCrashReporter();
>+
>+    let principal = Cc["@mozilla.org/systemprincipal;1"].
>+                    createInstance(Ci.nsIPrincipal);
>+    this.bootstrapScopes[aId] = new Components.utils.Sandbox(principal);
>+
>     let bootstrap = aDir.clone();
>     bootstrap.append("bootstrap.js");
>     if (bootstrap.exists()) {
>       let uri = Services.io.newFileURI(bootstrap);
>-      let principal = Cc["@mozilla.org/systemprincipal;1"].
>-                      createInstance(Ci.nsIPrincipal);
>-      let scope = new Components.utils.Sandbox(principal);
>       let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
>                    createInstance(Ci.mozIJSSubScriptLoader);
> 
>-      // Mark the add-on as active for the crash reporter before loading
>-      this.bootstrappedAddons[aId] = {
>-        version: aVersion,
>-        descriptor: aDir.persistentDescriptor
>-      };
>-      this.bootstrapScopes[aId] = scope;
>-      this.addAddonsToCrashReporter();
>-
>-      try {
>-        loader.loadSubScript(uri.spec, scope);
>+      try {
>+        loader.loadSubScript(uri.spec, this.bootstrapScopes[aId]);
>       }
>       catch (e) {
>         WARN("Error loading bootstrap.js for " + aId + ": " + e);
>       }
Seems wrong to continue when there is an error loading the bootstrap? I suspect that if loadSubScript fails none of the script will be loaded but it wouldn't hurt to verify that.

> 
>-      this.callBootstrapMethod(aId, scope, methods);
>+      // Copy the reason values from the global object into the bootstrap scope.
>+      for (let name in BOOTSTRAP_REASONS)
>+        this.bootstrapScopes[aId][name] = BOOTSTRAP_REASONS[name];
>     }
>     else {
>       WARN("Bootstrap missing for " + aId);
>     }
>   },

>...
>   /**
>+   * Calls a bootstrap method for an add-on.
>+   *
>+   * @param  aId
>+   *         The ID of the add-on
>+   * @param  aVersion
>+   *         The version of the add-on
>+   * @param  aDir
>+   *         The nsILocalFile for the directory containing the add-on
>+   * @param  aMethod
>+   *         The name of the bootstrap method to call
>+   * @param  aReason
>+   *         The reason flag to pass to the bootstrap's startup method
>+   */
>+  callBootstrapMethod: function XPI_callBootstrapMethod(aId, aVersion, aDir,
>+                                                        aMethod, aReason) {
>+    // Load the scope if it hasn't already been loaded
>+    if (!(aId in this.bootstrapScopes))
>+      this.loadBootstrapScope(aId, aDir, aVersion);
>+
>+    if (!(aMethod in this.bootstrapScopes[aId])) {
>+      WARN("Add-on " + aId + " is missing bootstrap method " + aMethod);
>+      return;
>+    }
>+
>+    let params = {
>+      id: aId,
>+      version: aVersion,
>+      installPath: aDir.clone()
>+    };
>+
>+    LOG("Calling bootstrap method " + aMethod + " on " + aId + " version " + aVersion);
nit: over 80 and easy to fix

>+    try {
>+      this.bootstrapScopes[aId][aMethod](params, aReason);
>+    }
>+    catch (e) {
>+      WARN("Exception running bootstrap method " + aMethods + " on " +
>+           aId + ": " + e);
>+    }
>+  },

>@@ -2476,17 +2499,17 @@ var XPIDatabase = {
>                                 "id TEXT, location TEXT, version TEXT, " +
>                                 "type TEXT, internalName TEXT, updateURL TEXT, " +
>                                 "updateKey TEXT, optionsURL TEXT, aboutURL TEXT, " +
>                                 "iconURL TEXT, defaultLocale INTEGER, " +
>                                 "visible INTEGER, active INTEGER, " +
>                                 "userDisabled INTEGER, appDisabled INTEGER, " +
>                                 "pendingUninstall INTEGER, descriptor TEXT, " +
>                                 "installDate INTEGER, updateDate INTEGER, " +
>-                                "applyBackgroundUpdates INTEGER, " +
>+                                "applyBackgroundUpdates INTEGER, bootstrap INTEGER, " +
>                                 "UNIQUE (id, location)");
>     this.connection.createTable("targetApplication",
>                                 "addon_internal_id INTEGER, " +
>                                 "id TEXT, minVersion TEXT, maxVersion TEXT, " +
>                                 "UNIQUE (addon_internal_id, id)");
nit: a few of these creep over 80 and are easy to fix

>@@ -3837,29 +3862,43 @@ AddonInstall.prototype = {
>                                                  createWrapper(this.addon));
>       }
>       else {
>         // TODO We can probably reduce the number of DB operations going on here
>         // We probably also want to support rolling back failed upgrades etc.
>         // See bug 553015.
> 
>         // Deactivate and remove the old add-on as necessary
>+        let reason = BOOTSTRAP_REASONS.ADDON_INSTALL;
>         if (this.existingAddon) {
>-          if (this.existingAddon.active) {
>-            if (this.existingAddon.type == "bootstrapped")
>-              XPIProvider.deactivateAddon(this.existingAddon.id, false,
>-                                          isUpgrade);
>-            // If this is an upgrade its metadata will be removed below
>-            if (!isUpgrade) {
>-              this.existingAddon.active = false;
>-              XPIDatabase.updateAddonActive(this.existingAddon);
>-            }
>-          }
>-          if (isUpgrade)
>+          if (Services.vc.compare(this.existingAddon.version, this.addon.version) < 0)
>+            reason = BOOTSTRAP_REASONS.ADDON_UPGRADE;
>+          else
>+            reason = BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
>+
>+          if (this.existingAddon.bootstrap) {
>+            let dir = this.existingAddon._installLocation.getLocationForID(this.existingAddon.id);
nit: well over 80

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 441904 [details] [diff] [review]
patch rev 4

r=me with nits fixed

Comment 13

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/0d05b191ef5d

Comment 14

[Biju]

do we a sample extension XPI file for Bootstrapped Extensions ?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking as verified fixed. If further changes are needed, a new bug should handle that work. Jetpacks work just fine with the new add-ons manager. Tested with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100517 Minefield/3.7a5pre

(In reply to comment #14)
> do we a sample extension XPI file for Bootstrapped Extensions ?

You can check https://secure.toolness.com/xpi/restartless/ which contains some examples of Jetpacks.

Comment 16

[Dave Townsend [:mossop]]

We have the docs at https://wiki.mozilla.org/Extension_Manager:Bootstrapped_Extensions but they really need to be nicified and put on MDC

Comment 17

[Eric Shepherd [:sheppy]]

https://developer.mozilla.org/en/Extensions/Bootstrapped_extensions