Dave Townsend [:mossop] Assignee
	Description • 15 years ago

      No description provided.

	Johnathan Nightingale [:johnath]
	Comment 1 • 14 years ago

Dave - this bug was marked blocking back in May, but it's pretty hard to reason about it as written. Any chance for some elaboration?

	Dave Townsend [:mossop] Assignee
	Comment 2 • 14 years ago

If a new bootstrapped add-on is detected in say the installation directory but there is already an add-on with the same ID in the profile directory then we shouldn't be loading the bootstrap script of the new add-on. Mostly just needs tests writing to ensure it, but since it hasn't been tested it might actually be broken right now.

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

I should have remembered the TODOS around here. There were indeed a couple of broken cases, this fixes everything I found except for a couple of slightly incorrect reason codes being sent in some cases that are quite difficult to fix. Since the reasons sent are mostly sane I don't think fixing that needs to block release so I've filed it as a follow-up bug 607818.

	Robert Strong (they/them - no direct email)
	Comment 4 • 14 years ago

Comment on attachment 486500 [details] [diff] [review]
patch rev 1

>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
>@@ -1848,23 +1848,32 @@ var XPIProvider = {
>       newAddon._installLocation = aInstallLocation;
>       newAddon.updateDate = aAddonState.mtime;
>       newAddon.visible = !(newAddon.id in visibleAddons);
> 
>       // Update the database
>       XPIDatabase.updateAddonMetadata(aOldAddon, newAddon, aAddonState.descriptor);
>       if (newAddon.visible) {
>         visibleAddons[newAddon.id] = newAddon;
>-        // If the old version was active and wasn't bootstrapped or the new
>-        // version will be active and isn't bootstrapped then we must force a
>-        // restart
>-        if ((aOldAddon.active && !aOldAddon.bootstrap) ||
>-            (newAddon.active && !newAddon.bootstrap)) {
>-          return true;
>+
>+        // If the new add-on is bootstrapped then call its install method
active and bootstrapped

>@@ -1887,26 +1896,33 @@ var XPIProvider = {
> 
>       // This add-ons metadata has not changed but it may have become visible
>       if (!(aOldAddon.id in visibleAddons)) {
>         visibleAddons[aOldAddon.id] = aOldAddon;
> 
>         if (!aOldAddon.visible) {
>           XPIDatabase.makeAddonVisible(aOldAddon);
> 
>-          // If the add-on is bootstrappable and it should be active then
>-          // mark it as active and add it to the list to be activated.
>-          if (aOldAddon.bootstrap && !aOldAddon.appDisabled &&
>-              !aOldAddon.userDisabled) {
>-            aOldAddon.active = true;
>-            XPIDatabase.updateAddonActive(aOldAddon);
>-            XPIProvider.bootstrappedAddons[aOldAddon.id] = {
>-              version: aOldAddon.version,
>-              descriptor: aAddonState.descriptor
>-            };
>+          if (aOldAddon.bootstrap) {
>+            // If the add-on is bootstrappable then call its install script
nit: After the if so, // The add-on is bootstrappable so call its install script

>@@ -2078,25 +2098,42 @@ var XPIProvider = {
>       }
> 
>       if (newAddon.visible) {
>         // Note if any visible add-on is not in the application install location
>         if (newAddon._installLocation.name != KEY_APP_GLOBAL)
>           XPIProvider.allAppGlobal = false;
> 
>         visibleAddons[newAddon.id] = newAddon;
>+
>+        let installReason = BOOTSTRAP_REASONS.ADDON_INSTALL;
>+
>+        // If we're hiding a bootstrapped add-on then call it's uninstall
>+        if (newAddon.id in XPIProvider.bootstrappedAddons) {
>+          let bootstrapState = XPIProvider.bootstrappedAddons[newAddon.id];
bootstrapState seems badly named to me.

>+
>+          installReason = Services.vc.compare(bootstrapState.version, newAddon.version) < 0 ?
>+                          BOOTSTRAP_REASONS.ADDON_UPGRADE :
>+                          BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
>+
>+          let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
It would make things a little clearer if this were named oldAddonFile especially since the following uses newAddon.id which made me initially think it was calling uninstall on the new add-on. Would help to add a comment about this calling uninstall on the old / existing add-on before callBootstrapMethod.

>+          file.persistentDescriptor = bootstrapState.descriptor;
>+          XPIProvider.callBootstrapMethod(newAddon.id, bootstrapState.version,
>+                                          file, "uninstall", installReason);
>+          XPIProvider.unloadBootstrapScope(newAddon.id);
>+        }
>+
>         if (!newAddon.bootstrap)
>           return true;

	Robert Strong (they/them - no direct email)
	Comment 5 • 14 years ago

Comment on attachment 486500 [details] [diff] [review]
patch rev 1

>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
...
>@@ -3032,17 +3069,64 @@ var XPIProvider = {
>         }
>         this.callBootstrapMethod(aAddon.id, aAddon.version, file, "uninstall",
>                                  BOOTSTRAP_REASONS.ADDON_UNINSTALL);
>         this.unloadBootstrapScope(aAddon.id);
>       }
>       aAddon._installLocation.uninstallAddon(aAddon.id);
>       XPIDatabase.removeAddonMetadata(aAddon);
>       AddonManagerPrivate.callAddonListeners("onUninstalled", wrapper);
>-      // TODO reveal hidden add-ons (bug 557710)
>+
>+      // Reveal the highest priority add-on with the same ID
>+      function revealAddon(aAddon) {
>+        XPIDatabase.makeAddonVisible(aAddon);
>+
>+        let wrapper = createWrapper(aAddon);
note: the Mozilla world is so full of wrappers nowadays I kind of regret this new one when searching. Might be a good thing to start calling them wrappedAddon.

>+        AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false);
>+
>+        if (!aAddon.userDisabled && !aAddon.appDisabled) {
>+          if (!XPIProvider.enableRequiresRestart(aAddon)) {
should just be one if statement

>+            aAddon.active = true;
>+            XPIDatabase.updateAddonActive(aAddon);
>+          }
>+        }

	Robert Strong (they/them - no direct email)
	Comment 6 • 14 years ago

Comment on attachment 486500 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js b/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js
>@@ -9,20 +9,37 @@ const ADDON_DISABLE                   = 
> const ADDON_INSTALL                   = 5;
> const ADDON_UNINSTALL                 = 6;
> const ADDON_UPGRADE                   = 7;
> const ADDON_DOWNGRADE                 = 8;
> 
> // This verifies that bootstrappable add-ons can be used without restarts.
> Components.utils.import("resource://gre/modules/Services.jsm");
> 
>-Services.prefs.setIntPref("bootstraptest.version", 0);
>+// Enable loading extensions from the user scopes
>+Services.prefs.setIntPref("extensions.enabledScopes",
>+                          AddonManager.SCOPE_PROFILE + AddonManager.SCOPE_USER);
>+
>+createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
> 
> const profileDir = gProfD.clone();
> profileDir.append("extensions");
>+const userDir = gProfD.clone();
>+userDir.append("extensions2");
>+userDir.append(gAppInfo.ID);
>+registerDirectory("XREUSysExt", userDir.parent);
Could you come up with a name that better describes this than userDir? userExtDir perhaps?

>+
>+function clearPrefs() {
Badly named since you are setting prefs

>+  Services.prefs.setIntPref("bootstraptest.active_version", -1);
>+  Services.prefs.setIntPref("bootstraptest.installed_version", -1);
>+  Services.prefs.setIntPref("bootstraptest.startup_reason", -1);
>+  Services.prefs.setIntPref("bootstraptest.shutdown_reason", -1);
>+  Services.prefs.setIntPref("bootstraptest.install_reason", -1);
>+  Services.prefs.setIntPref("bootstraptest.uninstall_reason", -1);
>+}

These are mainly nits so no need for me to see it again unless you want me to

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

Landed: http://hg.mozilla.org/mozilla-central/rev/dc97ef1346f5

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

(In reply to comment #2)
> If a new bootstrapped add-on is detected in say the installation directory but
> there is already an add-on with the same ID in the profile directory then we
> shouldn't be loading the bootstrap script of the new add-on. Mostly just needs

Dave, does that apply to any version of a Jetpack in the application folder or only to older ones? Aren't we now also installing newer versions of Jetpacks to the profile directory?

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

This applies to any restartless extension when there are multiple versions installed in the various global and profile install locations.

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

Thanks. Works as expected. Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209030359