568251 - Pending installs/upgrades are lost when upgrading to the new add-ons manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Dave Townsend [:mossop]]

When installing a newer build of Firefox we currently ignore any pending upgrades or installs.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Fairly straightforward. On startup we check the staged-xpis directory for any XPI's put there by older Firefoxes and if so extract them to the new staging location where they will be picked up shortly afterwards and installed.

I've also added some code to read the updated compatibility information out of extensions.rdf but this latter part will only happen when extensions.sqlite doesn't already exist. I don't think this is a big problem, by now a lot of extensions people install are marked compatible with Firefox 3.6 in their install.rdf and so Firefox 3.6 won't bother to fetch updated information.

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 465894 [details] [diff] [review]
patch rev 1

Actually this is spewing errors into the console.

Comment 3

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100918 Firefox/4.0b7pre SeaMonkey/2.1b1pre - Build ID: 20100918011221

On upgrade (from Sm 2.1a3pre 20100801) with installs pending, I see the following in <profile>/extensions.log:

2010-09-19 04:52:43 ERROR addons.xpi: Failed to add add-on {dc5d9a10-2736-11da-8cd6-0800200c9a66} in app-profile to database: [Exception... "Unexpected error"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: anonymous :: line 807"  data: no]
repeated for each pending install with only the GUID changing. These extensions are not listed in about:addons and their functionality is not there; OTOH a newly installed and enabled theme (also waiting on restart) does not trigger this error, appears in about:addons, and is displayed.

After doing the following:
1. Close the new version with File=>Quit (on other apps and/or platforms it might be File=>Exit -- or, of course, whatever translations are used in non-English versions)
2. Remove the following files from the current profile: addons.sqlite, addons.sqlite-journal, extensions.ini, extensions.sqlite, extensions.sqlite-journal
3. Start the new version again
...the "missing" extensions reappear, both in terms of functionality and of about:addons visibility. (The files removed at step 2 are recreated upon restart.)

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Slightly complicated by supporting not unpacking add-ons (though I guess arguably we could unpack these without worrying too much), but this is relatively straightforward.

We check for directories in the staged-xpis directory of the install location. In those we iterate over all XPIs choosing the last one as the one to install (this matches branch http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#1379 though we don't bother to remove the unused xpi files and just remove the whole staged-xpis directory at the end).

For the found file we open it to get its ID (we could just get this from the parent directory except we need to open it anyway) and whether to unpack it. Then we either extract or move it to the normal staging directory where the next round of checks will install it proper.

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 478449 [details] [diff] [review]
patch rev 1

Still reviewing but I wanted to mention this before I am done

>+          if (addon.unpack || Prefs.getBoolPref(PREF_XPI_UNPACK, false)) {
>+            let targetDir = stagingDir.clone();
>+            targetDir.append(addon.id);
>+            try {
>+              targetDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>+            }
>+            catch (e) {
>+              ERROR("Failed to create staging directory for add-on " + id + ": " + e);
>+              continue;
>+            }
>+
>+            try {
>+              extractFiles(stagedXPI, targetDir);
>+            }
>+            catch (e) {
>+              ERROR("Failed to extract staged XPI for add-on " + id + " in " +
>+                    aLocation.name + ": " + e);
>+            }
>+          }
>+          else {
>+            try {
>+              stagedXPI.moveTo(stagingDir, addon.id + ".xpi");
>+            }
>+            catch (e) {
>+              ERROR("Failed to move staged XPI for add-on " + id + " in " +
>+                    aLocation.name + ": " + e);
>+            }
>+          }
>+        }
>+        entries.close();
>+>+        try {
>+          stagedXPIDir.remove(true);
I'm a tad concerned about relying on remove though I seem to recall something being done to mitigate that though I might just be dreaming. See bug 321333 and
http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#490

Might want to do this for all directory removals. iirc when restoring from a backup device (e.g. cd, some backup programs, etc.) on Win2K the read only attribute is set on the restored files and it isn't that unusual for people to backup / restore their profiles. I also recall there being a backup program that did this on WinXP.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 478449 [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
>...
>@@ -1346,29 +1347,118 @@ var XPIProvider = {
>    * installed or add-ons to be uninstalled.
>    *
>    * @param  aManifests
>    *         A dictionary to add detected install manifests to for the purpose
>    *         of passing through updated compatibility information
>    * @return true if an add-on was installed or uninstalled
>    */
>   processPendingFileChanges: function XPI_processPendingFileChanges(aManifests) {
>-    // TODO maybe this should be passed off to the install locations to handle?
>     let changed = false;
>     this.installLocations.forEach(function(aLocation) {
>       aManifests[aLocation.name] = {};
>       // We can't install or uninstall anything in locked locations
>       if (aLocation.locked)
>         return;
> 
>+      let stagedXPIDir = aLocation.getXPIStagingDir();
>       let stagingDir = aLocation.getStagingDir();
>-      if (!stagingDir || !stagingDir.exists())
>+
>+      if (stagedXPIDir.exists() && stagedXPIDir.isDirectory()) {
would it make sense to remove stagedXPIDir if it is a file? As is it is only removed if it is a directory.

>...
>@@ -6035,16 +6134,22 @@ DirectoryInstallLocation.prototype = {
>    * @return an nsIFile
>    */
>   getStagingDir: function DirInstallLocation_getStagingDir() {
>     let dir = this._directory.clone();
>     dir.append(DIR_STAGE);
>     return dir;
>   },
> 

Comment for the function
>+  getXPIStagingDir: function DirInstallLocation_getXPIStagingDir() {
>+    let dir = this._directory.clone();
>+    dir.append(DIR_XPI_STAGE);
>+    return dir;
>+  },
>+
>   /**
>    * Installs an add-on into the install location.
>    *
>    * @param  aId
>    *         The ID of the add-on to install
>    * @param  aSource
>    *         The source nsIFile to install from
>    * @return an nsIFile indicating where the add-on was installed to

Could you also test that the staged xpi directory is removed after they have been processed?

Overall looks good but I'd like to see the patch again.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

(In reply to comment #5)
> I'm a tad concerned about relying on remove though I seem to recall something
> being done to mitigate that though I might just be dreaming. See bug 321333 and
> http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#490
> 
> Might want to do this for all directory removals. iirc when restoring from a
> backup device (e.g. cd, some backup programs, etc.) on Win2K the read only
> attribute is set on the restored files and it isn't that unusual for people to
> backup / restore their profiles. I also recall there being a backup program
> that did this on WinXP.

Done, I made it able to handle files or directories though since we frequently want to remove an extension's files without caring if it is packed or unpacked.

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 482923 [details] [diff] [review]
patch rev 2

>+ * Recursively removes a directory or file fixing permissions when necessary.
>+ *
>+ * @param  aDir
>+ *         The nsIFile to remove
>+ */
>+function recursiveRemove(aFile) {
function takes aFile and the comment param is aDir

>+  aFile.permissions = aFile.isDirectory() ? FileUtils.PERMS_DIRECTORY
>+                                          : FileUtils.PERMS_FILE;
I wonder what the cost setting the permis is especially since this shouldn't normally be needed? Wouldn't hurt to find out when time permits but I don't think it should hold up this patch.

Comment 9

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/4f6c27fc9977

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with an add-on update from 3.6.12 to Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101204 Firefox/4.0b8pre ID:20101204030328

Dave, how well is it tested by the automated test? Do we need a manual Litmus test here?

Comment 11

[Dave Townsend [:mossop]]

There is basic automated coverage that creates a simulated profile from an older Firefox and then starts up with it and verifies the add-ons get installed. It's a useful test but having a manual test to verify this in the real update process would be quite useful I think.