557956 - When switching versions of Firefox no compatibility update gets downloaded

Status: VERIFIED FIXED

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

Description

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

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a4pre) Gecko/20100407 Minefield/3.7a4pre (.NET CLR 3.5.30729)

I have seen this problem today after I have installed the DOM inspector into one of my normal profiles while using Minefield. After the installation I switched to the above branch version to check if the new Add-ons Manager is working correctly. But all the add-ons have been marked as incompatible.

Given by Dave on IRC we do not check for compatibility updates. Once the version info of Firefox get bumped up we will end in a complete list of disabled add-ons which cannot be reactivated manually. You will have to wait for the next update check.

I will raise the severity for this bug because I think that should be a blocker and be fixed before we merge to trunk. Otherwise we will leave nightly testers with all their add-ons disabled.

Comment 1

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

Dave, can you please specify in detail on which items we have to block the trunk landing and which ones we can fix later? I would like to file a follow-up bug.

Comment 2

[Dave Townsend [:mossop]]

(In reply to comment #1)
> Dave, can you please specify in detail on which items we have to block the
> trunk landing and which ones we can fix later? I would like to file a follow-up
> bug.

I filed bug 558823 to cover the part that I think needs to block the trunk landing.

Comment 3

[Dave Garrett]

It doesn't check for new versions either. Have an addon incompatible with 4.0b1 but with a new version that is and it doesn't update to it. I've seen a few users complaining of broken extensions under the beta because they have compatibility checking off and didn't think to check for updates themselves when it didn't do it automatically. (file new bug or use this for both update checks?)

Comment 5

[Dave Townsend [:mossop]]

Needed by string freeze

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 1

I don't like this UI but it has served us up till now and we don't really have the time to do something better in time for Firefox 4.

This switches it to the new APIs and implements more tests than we had in the past so I think in many ways we are better off than we were. I have however removed the errors.xul UI which I never even knew about. I don't believe it really serves a useful purpose, the UI already says an error has occurred and if users are really interested they can go to the add-ons manager and check for updates.

I'll be honest the code isn't as good as it could be and hope for some leniency around the reviews with the view of having it working with no expected problems but not being overly concerned with correcting code quality issues that were there before I touched it. I've filed bug 586492 to look at replacing this after we're done with Firefox 4.

Comment 7

[Dave Townsend [:mossop]]

Forgot to add. As I mentioned in person one niggle with this is that now the EM restart is gone we have no restart to finalize updates caused by this UI. So I've made the backend understand that during startup extensions cannot yet have been started by the platform and so any installs can happen without restarts.

Comment 8

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

Comment on attachment 464990 [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
>...
>@@ -975,18 +979,33 @@ var XPIProvider = {
>   // The value of the checkCompatibility preference
>   checkCompatibility: true,
>   // The value of the checkUpdateSecurity preference
>   checkUpdateSecurity: true,
>   // A dictionary of the file descriptors for bootstrappable add-ons by ID
>   bootstrappedAddons: {},
>   // A dictionary of JS scopes of loaded bootstrappable add-ons by ID
>   bootstrapScopes: {},
>+  // True if the platform can have allowed extensions to startup
nit: I think the following is easier to grok though I do prefer startup to activated
True if the platform could have activated extensions

>+  extensionsActive: false,
>+
>+  // True if all of the add-ons found during startup were installed in the
>+  // application install location
>+  allAppGlobal: true,
>   // A string listing the enabled add-ons for annotating crash reports
>   enabledAddons: null,
>+  // An array of the IDs of add-ons that were inactive during startup
nit: An array of add-on IDs that were inactive during startup

>+  inactiveAddonIDs: [],
>+  // A cache of the IDs of add-ons that had changes performed to them during
nit: s/the IDs of add-ons/add-on IDs/

>+  // this session's startup. This is preliminary work, hopefully it will be
>+  // expanded on in the future and an API made to get at it from the application.
>+  startupChanges: {
>+    // Add-ons that became disabled for compatibility reasons
>+    appDisabled: []
>+  },

>@@ -1087,16 +1106,35 @@ var XPIProvider = {
>     Services.prefs.addObserver(this.checkCompatibilityPref, this, false);
>     Services.prefs.addObserver(PREF_EM_CHECK_UPDATE_SECURITY, this, false);
> 
>     this.checkForChanges(aAppChanged);
> 
>     // Changes to installed extensions may have changed which theme is selected
>     this.applyThemeChange();
> 
>+    if (Services.prefs.prefHasUserValue(PREF_EM_DISABLED_ADDONS_LIST)) {
>+      // Clear the disabled addons list
nitty nit: don't think this comment is useful so might as well remove it... the call following this comment says as much.

>+      Services.prefs.clearUserPref(PREF_EM_DISABLED_ADDONS_LIST);
>+    }
>+
>+    // Determine if we should check for compatibility updates when upgrading if
>+    // we have add-ons that aren't managed by the application.
This comment isn't all that accurate. The code doesn't always check for compatibility updates even when the mismatch window is shown since it may just sync compatibility if all are compatible.

>+    if (aAppChanged && !this.allAppGlobal) {
>+      // Should we show a UI or just pass the list via a pref?
>+      if (Prefs.getBoolPref(PREF_EM_SHOW_MISMATCH_UI, true)) {
>+        this.showMismatchWindow();
>+      }
>+      else if (this.startupChanges.appDisabled.length > 0) {
>+        // Remember the list of add-ons that were disabled this startup
>+        Services.prefs.setCharPref(PREF_EM_DISABLED_ADDONS_LIST,
>+                                   this.startupChanges.appDisabled.join(","));
>+      }
>+    }
>+

>@@ -1124,30 +1162,38 @@ var XPIProvider = {
>           dir.persistentDescriptor = XPIProvider.bootstrappedAddons[id].descriptor;
>           XPIProvider.callBootstrapMethod(id, XPIProvider.bootstrappedAddons[id].version,
>                                           dir, "shutdown",
>                                           BOOTSTRAP_REASONS.APP_SHUTDOWN);
>         }
>         Services.obs.removeObserver(this, "quit-application-granted");
>       }
>     }, "quit-application-granted", false);
>+
>+    this.extensionsActive = true;
nit: since this is only for xpcshell tests please add a comment to this affect. I'm also a little concerned about shutdown being called by an extension instead of xpcom-shutdown but I don't think the effort to guard against that is worth the reward.

Comment 9

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

Comment on attachment 464990 [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

>@@ -1182,16 +1230,34 @@ var XPIProvider = {
>     }
>     catch (e) {
>       ERROR(e);
>     }
>     Services.prefs.clearUserPref(PREF_DSS_SWITCHPENDING);
>   },
> 
>   /**
>+   * Shows the "Compatibility Updates" UI
>+   * @param   items
>+   *          an array of item IDs that were not enabled in the previous version
>+   *          of the application.
There is no param ;)

>+   */
>+  showMismatchWindow: function XPI_showMismatchWindow() {

>@@ -2161,16 +2241,21 @@ var XPIProvider = {
>   /**
>    * Tests whether enabling an add-on will require a restart.
>    *
>    * @param  aAddon
>    *         The add-on to test
>    * @return true if the operation requires a restart
>    */
>   enableRequiresRestart: function XPI_enableRequiresRestart(aAddon) {
>+    // If the platform can't have started up extensions then we can make changes
>+    // without any restart.
nit: same as earlier nit
if the platform couldn't have activated extensions


>+    if (!this.extensionsActive)
>+      return false;
>+

>@@ -2178,16 +2263,21 @@ var XPIProvider = {
>   /**
>    * Tests whether disabling an add-on will require a restart.
>    *
>    * @param  aAddon
>    *         The add-on to test
>    * @return true if the operation requires a restart
>    */
>   disableRequiresRestart: function XPI_disableRequiresRestart(aAddon) {
>+    // If the platform can't have started up extensions then we can make changes
>+    // without any restart.
nit: same as previous nit

>+    if (!this.extensionsActive)
>+      return false;
>+

>@@ -2197,32 +2287,42 @@ var XPIProvider = {
>   /**
>    * Tests whether installing an add-on will require a restart.
>    *
>    * @param  aAddon
>    *         The add-on to test
>    * @return true if the operation requires a restart
>    */
>   installRequiresRestart: function XPI_installRequiresRestart(aAddon) {
>+    // If the platform can't have started up extensions then we can make changes
>+    // without any restart.
nit: same as previous nit

>+    if (!this.extensionsActive)
>+      return false;
>+

>   uninstallRequiresRestart: function XPI_uninstallRequiresRestart(aAddon) {
>+    // If the platform can't have started up extensions then we can make changes
>+    // without any restart.
nit: same as previous nit

>+    if (!this.extensionsActive)
>+      return false;
>+

>@@ -4566,17 +4666,16 @@ AddonInstall.prototype = {
>     let requiresRestart = XPIProvider.installRequiresRestart(this.addon);
>     // Restarts is required if the existing add-on is active and disabling it
>     // requires a restart
>     if (!requiresRestart && this.existingAddon) {
>       requiresRestart = this.existingAddon.active &&
>                         XPIProvider.disableRequiresRestart(this.existingAddon);
>     }
> 
>-    LOG("Starting install of " + this.sourceURI.spec);
curious why you are removing this? Is it already logged elsewhere?

Comment 10

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

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

>diff --git a/toolkit/mozapps/extensions/content/update.js b/toolkit/mozapps/extensions/content/update.js
>--- a/toolkit/mozapps/extensions/content/update.js
>+++ b/toolkit/mozapps/extensions/content/update.js
>...
>+    // Retrieve all add-ons in order to sync their app compatibility information
>+    AddonManager.getAllAddons(function(aAddons) {
>+      gUpdateWizard.addons = aAddons;
I don't recall the conditions that poke bug 566787... will it mess up tests?

now on to tests

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #9)
> >     let requiresRestart = XPIProvider.installRequiresRestart(this.addon);
> >     // Restarts is required if the existing add-on is active and disabling it
> >     // requires a restart
> >     if (!requiresRestart && this.existingAddon) {
> >       requiresRestart = this.existingAddon.active &&
> >                         XPIProvider.disableRequiresRestart(this.existingAddon);
> >     }
> > 
> >-    LOG("Starting install of " + this.sourceURI.spec);
> curious why you are removing this? Is it already logged elsewhere?

I'm curious too, I don't think I meant to.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #10)
> Comment on attachment 464990 [details] [diff] [review]
> patch rev 1
> 
> >diff --git a/toolkit/mozapps/extensions/content/update.js b/toolkit/mozapps/extensions/content/update.js
> >--- a/toolkit/mozapps/extensions/content/update.js
> >+++ b/toolkit/mozapps/extensions/content/update.js
> >...
> >+    // Retrieve all add-ons in order to sync their app compatibility information
> >+    AddonManager.getAllAddons(function(aAddons) {
> >+      gUpdateWizard.addons = aAddons;
> I don't recall the conditions that poke bug 566787... will it mess up tests?

Possibly, it didn't on tinderbox or on my machine so I don't think I'm going to hold up landing for it (I want to get this in before the nightly with the 4.0b5pre version bump). I have also put up a patch in bug 566787 which I'll try to get landed a.s.a.p. too.

Comment 13

[Dave Townsend [:mossop]]

landed: http://hg.mozilla.org/mozilla-central/rev/ab6e93fb7fec

The tests here test the backend in xpcshell and the UI in browser-chrome, but we can't do real testing here, some manual tests would be very beneficial (I believe we had some for old versions).

Comment 14

[Dave Townsend [:mossop]]

Disabled the test due to timeouts on windows 7

Comment 15

[Dave Townsend [:mossop]]

Filed bug 588440 to track an issue where this UI is showing up for new profiles in some cases.

Comment 16

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

Dave, is there a pref to disable such a compatibility check? It will cause problems for us when running Mozmill tests across different branches. Was 'extensions.update.notifyUser' the formerly pref for older branches?

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> Dave, is there a pref to disable such a compatibility check? It will cause
> problems for us when running Mozmill tests across different branches. Was
> 'extensions.update.notifyUser' the formerly pref for older branches?

The preference used in 3.6 and trunk is "extensions.showMismatchUI"

Comment 18

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre

(In reply to comment #13)
> The tests here test the backend in xpcshell and the UI in browser-chrome, but
> we can't do real testing here, some manual tests would be very beneficial (I
> believe we had some for old versions).

We haven't had such a manual test in the add-ons manager subgroup but for major updates:

https://litmus.mozilla.org/show_test.cgi?id=8752 (compatible extension)
https://litmus.mozilla.org/show_test.cgi?id=8753 (compatible theme)
https://litmus.mozilla.org/show_test.cgi?id=8756 (incompatible extension)
https://litmus.mozilla.org/show_test.cgi?id=8757 (incompatible themes)

Al, how often do you use that 3.5->3.6 MU subgroup for your tests? I wonder if we can simplify some of those tests.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

They are not run very often at all. They can probably be simplified to some degree.