Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Check that bootstrapping of newly detected/missing add-ons behaves right for non-visible locations

VERIFIED FIXED in mozilla2.0b8

Status

()

Toolkit
Add-ons Manager
P2
normal
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

(Depends on: 1 bug)

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

7 years ago
Depends on: 557710
(Assignee)

Updated

7 years ago
blocking2.0: --- → final+
(Assignee)

Updated

7 years ago
Assignee: dtownsend → nobody
(Assignee)

Updated

7 years ago
Assignee: nobody → dtownsend
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?
(Assignee)

Comment 2

7 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.
(Assignee)

Comment 3

7 years ago
Created attachment 486500 [details] [diff] [review]
patch rev 1

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.
Attachment #486500 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Whiteboard: [rewrite] → [has patch][needs review rs]
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
Attachment #486500 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][needs review rs] → [has patch]
(Assignee)

Comment 7

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/dc97ef1346f5
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
(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?
(Assignee)

Comment 9

7 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.