Last Comment Bug 412819 - Mechanism for changing an extension GUID via updates
: Mechanism for changing an extension GUID via updates
Status: VERIFIED FIXED
[amo:want P2]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: mozilla2.0b8
Assigned To: Dave Townsend [:mossop]
:
Mentors:
Depends on: 617291 617293
Blocks: 765717
  Show dependency treegraph
 
Reported: 2008-01-17 11:40 PST by Justin Scott [:fligtar]
Modified: 2012-06-18 06:52 PDT (History)
16 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
patch rev 1 (34.37 KB, patch)
2010-11-19 14:35 PST, Dave Townsend [:mossop]
robert.strong.bugs: review-
Details | Diff | Review
patch rev 2 (35.35 KB, patch)
2010-11-24 14:05 PST, Dave Townsend [:mossop]
robert.strong.bugs: review+
Details | Diff | Review

Description Justin Scott [:fligtar] 2008-01-17 11:40:21 PST
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.
Comment 1 Mike Connor [:mconnor] 2008-07-29 13:26:52 PDT
Dave, this seems like something we should look into.
Comment 2 Dave Townsend [:mossop] 2008-07-29 17:04:54 PDT
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
Comment 3 Dave Townsend [:mossop] 2009-04-23 12:45:25 PDT
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.
Comment 4 Rolf Bode-Meyer 2009-04-24 05:14:22 PDT
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.
Comment 5 Dave Townsend [:mossop] 2009-04-24 10:00:28 PDT
(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?
Comment 6 Rolf Bode-Meyer 2009-04-25 08:45:10 PDT
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.
Comment 7 Dave Townsend [:mossop] 2010-11-18 12:51:05 PST
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.
Comment 8 Dave Townsend [:mossop] 2010-11-19 14:35:25 PST
Created attachment 491956 [details] [diff] [review]
patch rev 1

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.
Comment 9 Robert Strong [:rstrong] (use needinfo to contact me) 2010-11-23 15:12:12 PST
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.
Comment 10 Dave Townsend [:mossop] 2010-11-24 13:25:24 PST
(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?
Comment 11 Dave Townsend [:mossop] 2010-11-24 14:05:29 PST
Created attachment 493114 [details] [diff] [review]
patch rev 2

Updated from review, waiting on response to comment 10
Comment 12 Robert Strong [:rstrong] (use needinfo to contact me) 2010-11-24 14:07:32 PST
(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.
Comment 13 Dave Townsend [:mossop] 2010-11-24 14:45:20 PST
Landed: http://hg.mozilla.org/mozilla-central/rev/15f8aa161de5
Comment 14 Eric Shepherd [:sheppy] 2010-12-20 13:30:32 PST
Updated docs:

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

And mentioned on Fx4 for developers
Comment 15 Henrik Skupin (:whimboo) 2011-02-11 18:52:58 PST
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

Note You need to log in before you can comment on or make changes to this bug.