Closed Bug 613294 Opened 12 years ago Closed 11 years ago

Upgrading a restartless add-on to a non-restartless add-on doesn't call the uninstall method

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla6
Tracking Status
blocking2.0 --- .x+

People

(Reporter: mossop, Assigned: mossop)

Details

Attachments

(1 file, 1 obsolete file)

The code recognises that the updating requires a restart so we don't immediately shutdown and uninstall the old add-on and instead do the upgrade at the restart, but the code doesn't currently handle calling the uninstall method on the old add-on at this point.
This should probably block as it is likely that add-on developers will switch between restartless and non-restartless depending on their needs.
blocking2.0: --- → betaN+
Notes from the Grand Retriage: worth fixing on branch. Not worth holding FF4 for the hypothetical addon authors porting to restartless *and then back again*.
blocking2.0: betaN+ → .x
Whiteboard: [wanted fx5]
Assignee: nobody → dtownsend
Attached patch patch rev 1 (obsolete) — Splinter Review
Relatively straightforward. Check the list of active bootstrapped add-ons to see if we're about to upgrade one and if so call its uninstall method. We have to be sure to load the manifest from the new add-on earlier than previously to be sure we get the reason code right but this is only moving work not doing anything more.
Attachment #527548 - Flags: review?(robert.bugzilla)
Comment on attachment 527548 [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
>@@ -1817,68 +1817,114 @@ var XPIProvider = {
>               ERROR("Failed to uninstall add-on " + id + " in " + aLocation.name, e);
>             }
>             // The file check later will spot the removal and cleanup the database
>             continue;
>           }
>         }
> 
>         aManifests[aLocation.name][id] = null;
>-        let existingAddonID = null;
>+        let existingAddonID = id;
> 
>         // Check for a cached AddonInternal for this add-on, it may contain
>         // updated compatibility information
>         let jsonfile = stagingDir.clone();
>         jsonfile.append(id + ".json");
>         if (jsonfile.exists()) {
>           LOG("Found updated manifest for " + id + " in " + aLocation.name);
>           let fis = Cc["@mozilla.org/network/file-input-stream;1"].
>                        createInstance(Ci.nsIFileInputStream);
>           let json = Cc["@mozilla.org/dom/json;1"].
>                      createInstance(Ci.nsIJSON);
> 
>           try {
>             fis.init(jsonfile, -1, 0, 0);
>             aManifests[aLocation.name][id] = json.decodeFromStream(fis,
>                                                                    jsonfile.fileSize);
>-            existingAddonID = aManifests[aLocation.name][id].existingAddonID;
>+            existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
nit: this is a tad weird since existingAddonID is already id but I think I'm ok with it

>           }
>           catch (e) {
>             ERROR("Unable to read add-on manifest for " + id + " in " +
>                   aLocation.name, e);
>           }
>           finally {
>             fis.close();
>           }
>         }
> 
>+        // If there was no cached AddonInternal then load it directly
>+        if (!aManifests[aLocation.name][id]) {
>+          try {
>+            aManifests[aLocation.name][id] = loadManifestFromFile(stageDirEntry);
>+            existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
>+          }
>+          catch (e) {
>+            // This add-on can't be installed so just remove it now
>+            stageDirEntry.remove(true);
>+            ERROR("Unable to read add-on manifest for " + id + " in " +
>+                  aLocation.name, e);
This is even weirder in that the previous error has the exact same text.
Comment on attachment 527548 [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
>@@ -1817,68 +1817,114 @@ var XPIProvider = {
>...
>           try {
>             fis.init(jsonfile, -1, 0, 0);
>             aManifests[aLocation.name][id] = json.decodeFromStream(fis,
>                                                                    jsonfile.fileSize);
>-            existingAddonID = aManifests[aLocation.name][id].existingAddonID;
>+            existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
>           }
>           catch (e) {
>             ERROR("Unable to read add-on manifest for " + id + " in " +
>                   aLocation.name, e);
should this still be an error now that you have a fallback below?

>           }
>           finally {
>             fis.close();
>           }
>         }
> 
>+        // If there was no cached AddonInternal then load it directly
>+        if (!aManifests[aLocation.name][id]) {
>+          try {
>+            aManifests[aLocation.name][id] = loadManifestFromFile(stageDirEntry);
>+            existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
>+          }
>+          catch (e) {
>+            // This add-on can't be installed so just remove it now
>+            stageDirEntry.remove(true);
>+            ERROR("Unable to read add-on manifest for " + id + " in " +
>+                  aLocation.name, e);
>+          }
>+        }
>+
>         LOG("Processing install of " + id + " in " + aLocation.name);
>+        if (existingAddonID in this.bootstrappedAddons) {
>+          var oldBootstrap = this.bootstrappedAddons[existingAddonID];
>+          try {
>+            var existingAddon = aLocation.getLocationForID(existingAddonID);
>+            if (oldBootstrap.descriptor == existingAddon.persistentDescriptor) {
>+              // We'll be replacing a currently active bootstrapped add-on so
>+              // call its uninstall method
>+              let oldVersion = aManifests[aLocation.name][id].version;
>+              let newVersion = oldBootstrap.version;
>+              let uninstallReason = Services.vc.compare(newVersion, oldVersion) < 0 ?
>+                                    BOOTSTRAP_REASONS.ADDON_UPGRADE :
>+                                    BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
>+
>+              this.callBootstrapMethod(existingAddonID, oldBootstrap.version,
>+                                       existingAddon, "uninstall", uninstallReason);
>+              this.unloadBootstrapScope(existingAddonID);
>+            }
>+            else {
>+              oldBootstrap = null;
>+            }
>+          }
>+          catch (e) {
>+          }
>+        }
>+
>         try {
>           var addonInstallLocation = aLocation.installAddon(id, stageDirEntry,
>                                                             existingAddonID);
>           if (aManifests[aLocation.name][id])
>             aManifests[aLocation.name][id]._sourceBundle = addonInstallLocation;
>         }
>         catch (e) {
>           ERROR("Failed to install staged add-on " + id + " in " + aLocation.name,
>                 e);
>           delete aManifests[aLocation.name][id];
>+
>+          if (oldBootstrap) {
>+            // Re-install the old add-on
>+            this.callBootstrapMethod(existingAddonID, oldBootstrap.version,
>+                                     existingAddon, "install",
>+                                     BOOTSTRAP_REASONS.ADDON_INSTALL);
There is a case where you set oldBootstrap to null above... is that somehow ok?
Attachment #527548 - Flags: review?(robert.bugzilla) → review-
(In reply to comment #5)
>...
> There is a case where you set oldBootstrap to null above... is that somehow ok?
Specifically, if the add-on should be reinstalled?
(In reply to comment #4)
> >+        // If there was no cached AddonInternal then load it directly
> >+        if (!aManifests[aLocation.name][id]) {
> >+          try {
> >+            aManifests[aLocation.name][id] = loadManifestFromFile(stageDirEntry);
> >+            existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
> >+          }
> >+          catch (e) {
> >+            // This add-on can't be installed so just remove it now
> >+            stageDirEntry.remove(true);
> >+            ERROR("Unable to read add-on manifest for " + id + " in " +
> >+                  aLocation.name, e);
> This is even weirder in that the previous error has the exact same text.

Changed the messages to specify what file they were trying to load the manifest from.

(In reply to comment #5)
> Comment on attachment 527548 [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
> >@@ -1817,68 +1817,114 @@ var XPIProvider = {
> >...
> >           try {
> >             fis.init(jsonfile, -1, 0, 0);
> >             aManifests[aLocation.name][id] = json.decodeFromStream(fis,
> >                                                                    jsonfile.fileSize);
> >-            existingAddonID = aManifests[aLocation.name][id].existingAddonID;
> >+            existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
> >           }
> >           catch (e) {
> >             ERROR("Unable to read add-on manifest for " + id + " in " +
> >                   aLocation.name, e);
> should this still be an error now that you have a fallback below?

I think so, it suggests that the json file is corrupt somehow, we'd need to know that we failed to load it if we experience issues with compat updates for new installs seeming to not apply.

> >+
> >+          if (oldBootstrap) {
> >+            // Re-install the old add-on
> >+            this.callBootstrapMethod(existingAddonID, oldBootstrap.version,
> >+                                     existingAddon, "install",
> >+                                     BOOTSTRAP_REASONS.ADDON_INSTALL);
> There is a case where you set oldBootstrap to null above... is that somehow ok?

Changed this a little but basically when oldBootstrap is null it means the new add-on isn't replacing an existing bootstrapped add-on, so there will be nothing to call the install method on again.
Attached patch patch rev 2Splinter Review
Updated patch
Attachment #527548 - Attachment is obsolete: true
Attachment #528148 - Flags: review?(robert.bugzilla)
Whiteboard: [wanted fx5] → [wanted fx5][has patch][needs review rs]
Comment on attachment 528148 [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
>...
>@@ -1817,68 +1816,112 @@ var XPIProvider = {
>...
>+        var oldBootstrap = null;
>         LOG("Processing install of " + id + " in " + aLocation.name);
>+        if (existingAddonID in this.bootstrappedAddons) {
>+          try {
>+            var existingAddon = aLocation.getLocationForID(existingAddonID);
>+            if (this.bootstrappedAddons[existingAddonID].descriptor ==
>+                existingAddon.persistentDescriptor) {
>+              oldBootstrap = this.bootstrappedAddons[existingAddonID];
>+
>+              // We'll be replacing a currently active bootstrapped add-on so
>+              // call its uninstall method
>+              let oldVersion = aManifests[aLocation.name][id].version;
>+              let newVersion = oldBootstrap.version;
>+              let uninstallReason = Services.vc.compare(newVersion, oldVersion) < 0 ?
>+                                    BOOTSTRAP_REASONS.ADDON_UPGRADE :
>+                                    BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
What should happen if it is the same version... followup bug?
Attachment #528148 - Flags: review?(robert.bugzilla) → review+
(In reply to comment #9)
> Comment on attachment 528148 [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
> >...
> >@@ -1817,68 +1816,112 @@ var XPIProvider = {
> >...
> >+        var oldBootstrap = null;
> >         LOG("Processing install of " + id + " in " + aLocation.name);
> >+        if (existingAddonID in this.bootstrappedAddons) {
> >+          try {
> >+            var existingAddon = aLocation.getLocationForID(existingAddonID);
> >+            if (this.bootstrappedAddons[existingAddonID].descriptor ==
> >+                existingAddon.persistentDescriptor) {
> >+              oldBootstrap = this.bootstrappedAddons[existingAddonID];
> >+
> >+              // We'll be replacing a currently active bootstrapped add-on so
> >+              // call its uninstall method
> >+              let oldVersion = aManifests[aLocation.name][id].version;
> >+              let newVersion = oldBootstrap.version;
> >+              let uninstallReason = Services.vc.compare(newVersion, oldVersion) < 0 ?
> >+                                    BOOTSTRAP_REASONS.ADDON_UPGRADE :
> >+                                    BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
> What should happen if it is the same version... followup bug?

Turns out everywhere has the same problem so I've filed bug 652645 to come to a solution. For now this matches what happens elsewhere so I'll land this.
Landed: http://hg.mozilla.org/mozilla-central/rev/e67a34896a22
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [wanted fx5][has patch][needs review rs] → [wanted fx5]
Target Milestone: --- → mozilla6
Dave, when has the uninstall method being called? Is it after the restart? Because that's what I'm seeing with my example extension update.
(In reply to comment #12)
> Dave, when has the uninstall method being called? Is it after the restart?
> Because that's what I'm seeing with my example extension update.

Correct, because the non-restartless add-on doesn't get installed until after the restart we don't call uninstall on the old one until then.
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110529 Firefox/6.0a2

Dave, can we remove the [wanted-fx5] whiteboard entry? I don't think we want to get this patch onto the 5 branch.
Status: RESOLVED → VERIFIED
Whiteboard: [wanted fx5]
You need to log in before you can comment on or make changes to this bug.