Justin Scott [:fligtar] Reporter
	Description • 17 years ago

Add-on developers change their extension GUID fairly often without realizing what it will do to their listing on AMO and automatic updates.

Most times this is "fixed" by either abandoning the old GUID (and those people will never get updated) or reverting back to the old GUID and anyone who's since downloaded the new version will never get an update.

It would be helpful if there was a way to indicate that an add-on update is meant to replace or merge with an existing GUID.

So for example, an author has many users with extension@fligtar.com. Author doesn't have fligtar.com anymore so changes GUID to extension@mfinkle.com and ends up with 2 entries on AMO. Author doesn't want to go back to the old GUID since he doesn't have that domain anymore, so when people using extension@fligtar.com check for updates, instead they get an update file indicating that the extension has been moved to extension@mfinkle.com and to check for updates there.

	Mike Connor [:mconnor]
	Comment 1 • 16 years ago

Dave, this seems like something we should look into.

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

Yeah it came up in the session just now. We'll be able to deal with it fairly easily with the changes I have planned for sqlite support

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

Putting this on the radar. I'm a little concerned that this is open to abuse though, could I for instance mark my extension as being the new version of the google toolbar? I guess they can do that already anyway though.

	Rolf Bode-Meyer
	Comment 4 • 15 years ago

If the addon contains an update key and the update.rdf is signed and contains an updateHash that should be no issue.

But addon update via an SSL site, even if it's AMO, doesn't provide this security as far as I can see.

	Dave Townsend [:mossop] Assignee
	Comment 5 • 15 years ago

(In reply to comment #4)
> If the addon contains an update key and the update.rdf is signed and contains
> an updateHash that should be no issue.
> 
> But addon update via an SSL site, even if it's AMO, doesn't provide this
> security as far as I can see.

The problem is that this bug isn't necessarily fixed by only looking at the auto-update path. What if a user goes to the author's website and installs the new version which has a different GUID. Should there still be something that makes sure the old version is removed or do we just let things break in that case?

	Rolf Bode-Meyer
	Comment 6 • 15 years ago

From my point of view automatic GUID migration should only happen when using the update path, not when installing another version manually.

If on manuall installing, an addonn with GUI B identifies itself as replacement to an addon with GUI A and that addon is installed, we might inform the user about that including the consequences this might have (yes, I know another annoying warning) but leave up to him what to do.

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

We want to encourage existing add-on developers to move to using the Jetpack SDK where possible but this blocks that since the Jetpack SDK currently doesn't allow you to use a custom ID (bug 604499) so I think we really want to block on getting a fix here.

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

This implements the change in the most straightforward manner possible. If the update.rdf for an extension offers an XPI then we download it and install it, removing the extension that was being updated in the process regardless of whether the ID changed. No additional properties needed or anything, but it does only work through the update path.

Two cases that behave a little differently. If an add-on with the new XPI's ID already exists then we just upgrade that and leave the old one alone. This could be changed but it seems like a bit of a weird case and I'm wary of having a single update remove two installed extensions. Also if the old extension exists in a different install location then it won't get removed so the behaviour would just be as it is now in that case, both old and new extensions would exist after the update.

Obviously this will only work in Firefox 4, if developers still target older versions then they won't be able to use this mechanism.

The patch includes tests verifying updating non-restartless -> non-restartless, non-restartless -> restartless, restartless -> restartless, restartless -> non-restartless as well as checking things don't break if the filesystem is locked in some way. IN writing these tests I discovered bug 613294 but that was pre-existing so I plan to fix it separately.

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

Comment on attachment 491956 [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
>@@ -1714,27 +1714,18 @@ var XPIProvider = {
>             catch (e) {
>               ERROR("Failed to uninstall add-on " + id + " in " + aLocation.name, e);
>             }
>             // The file check later will spot the removal and cleanup the database
>             continue;
>           }
>         }
> 
>-        LOG("Processing install of " + id + " in " + aLocation.name);
>-        try {
>-          var addonInstallLocation = aLocation.installAddon(id, stageDirEntry);
>-        }
>-        catch (e) {
>-          ERROR("Failed to install staged add-on " + id + " in " + aLocation.name,
>-                e);
>-          continue;
>-        }
>-
>         aManifests[aLocation.name][id] = 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"].
>@@ -1742,25 +1733,37 @@ var XPIProvider = {
>           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);
>             aManifests[aLocation.name][id]._sourceBundle = addonInstallLocation;
>+            existingAddonID = aManifests[aLocation.name][id].existingAddonID;
>           }
>           catch (e) {
>             ERROR("Unable to read add-on manifest for " + id + " in " +
>                   aLocation.name, e);
>           }
>           finally {
>             fis.close();
>           }
>         }
>+
>+        LOG("Processing install of " + id + " in " + aLocation.name);
>+        try {
>+          var addonInstallLocation = aLocation.installAddon(id, stageDirEntry, existingAddonID);
addonInstallLocation is declared here and used above when setting _sourceBundle

>@@ -5459,22 +5462,31 @@ AddonInstall.prototype = {
>   },
> 
>   /**
>    * Notify listeners that the download completed.
>    */
>   downloadCompleted: function() {
>     let self = this;
>     XPIDatabase.getVisibleAddonForID(this.addon.id, function(aAddon) {
>-      self.existingAddon = aAddon;
>       if (aAddon)
>-        self.addon.userDisabled = aAddon.userDisabled;
>+        self.existingAddon = aAddon;
>+
>+      self.state = AddonManager.STATE_DOWNLOADED;
>       self.addon.updateDate = Date.now();
>-      self.addon.installDate = aAddon ? aAddon.installDate : self.addon.updateDate;
>-      self.state = AddonManager.STATE_DOWNLOADED;
>+
>+      if (self.existingAddon) {
>+        self.addon.existingAddonID = self.existingAddon.id;
>+        self.addon.userDisabled = self.existingAddon.userDisabled;
>+        self.addon.installDate = self.existingAddon.installDate;
>+      }
>+      else {
>+        self.addon.installDate = self.addon.updateDate;
>+      }
>...
>@@ -5613,17 +5625,19 @@ AddonInstall.prototype = {
> 
>           if (!isUpgrade && this.existingAddon.active) {
>             this.existingAddon.active = false;
>             XPIDatabase.updateAddonActive(this.existingAddon);
>           }
>         }
> 
>         // Install the new add-on into its final location
>-        let file = this.installLocation.installAddon(this.addon.id, stagedAddon);
>+        let existingAddonID = this.existingAddon ? this.existingAddon.id : null;
>+        let file = this.installLocation.installAddon(this.addon.id, stagedAddon,
>+                                                     existingAddonID);
>         cleanStagingDir(stagedAddon.parent, []);
> 
>         // Update the metadata in the database
>         this.addon._installLocation = this.installLocation;
>         this.addon.updateDate = recursiveLastModifiedTime(file);
>         this.addon.visible = true;
>         if (isUpgrade) {
>           XPIDatabase.updateAddonMetadata(this.existingAddon, this.addon,
I find the usage of self.existingAddon.id and self.addon.existingAddonID a tad confusing. Could this be made simpler?

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_updateid.js b/toolkit/mozapps/extensions/test/browser/browser_updateid.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/browser/browser_updateid.js
>@@ -0,0 +1,78 @@
>...
>+add_test(function() {
>+  var item = get_addon_element(gManagerWindow, "addon1@tests.mozilla.org");
>+  var update = gManagerWindow.document.getAnonymousElementByAttribute(item, "anonid", "update-btn");
>+  EventUtils.synthesizeMouseAtCenter(update, { }, gManagerWindow);
>+
>+  var pending = gManagerWindow.document.getAnonymousElementByAttribute(item, "anonid", "pending");
>+  is_element_visible(pending, "Pending message should be visible");
>+  is(pending.textContent, "manually updating addon will be updated after you restart " + gApp + ".", "Pending message should be correct");
Please use the localized string vs. hardcoding it.

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

(In reply to comment #9)
> Comment on attachment 491956 [details] [diff] [review]
> patch rev 1
> 
> >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps> >         // Install the new add-on into its final location
> >-        let file = this.installLocation.installAddon(this.addon.id, stagedAddon);
> >+        let existingAddonID = this.existingAddon ? this.existingAddon.id : null;
> >+        let file = this.installLocation.installAddon(this.addon.id, stagedAddon,
> >+                                                     existingAddonID);
> >         cleanStagingDir(stagedAddon.parent, []);
> I find the usage of self.existingAddon.id and self.addon.existingAddonID a tad
> confusing. Could this be made simpler?

I'm not sure it can really. I have to pass either null or the existing add-on's ID to installAddon. Since this.existingAddon might be null I need to check it an return either its id or null. I could do that directly in the method call but it seems more readable to me to store it in a variable first. Do you disagree?

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

Updated from review, waiting on response to comment 10

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

(In reply to comment #11)
> Created attachment 493114 [details] [diff] [review]
> patch rev 2
> 
> Updated from review, waiting on response to comment 10
Sorry, I didn't cc myself. I'm ok with that.

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

Landed: http://hg.mozilla.org/mozilla-central/rev/15f8aa161de5

	Eric Shepherd [:sheppy]
	Comment 14 • 14 years ago

Updated docs:

https://developer.mozilla.org/index.php?title=en/Extension_Versioning%2C_Update_and_Compatibility

And mentioned on Fx4 for developers

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 15 • 13 years ago

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

I have created a live test which changes the id here:
https://www.hskupin.info/mozilla/addons/update-id/empty_v1.xpi