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?

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.

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.

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;

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); >+ } >+ }

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

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

(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?

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

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