When switching versions of Firefox no compatibility update gets downloaded

VERIFIED FIXED in mozilla2.0b5

Status

()

Toolkit
Add-ons Manager
P2
normal
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: whimboo, Assigned: mossop)

Tracking

({regression})

Trunk
mozilla2.0b5
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus ?

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

(Whiteboard: [rewrite])

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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.
Flags: in-testsuite?
(Reporter)

Updated

8 years ago
Priority: -- → P1
(Reporter)

Comment 1

8 years ago
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.
(Reporter)

Updated

8 years ago
Assignee: nobody → dtownsend
(Assignee)

Comment 2

7 years ago
(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.
Severity: blocker → normal
Priority: P1 → P2
(Reporter)

Updated

7 years ago
Depends on: 558823
(Assignee)

Updated

7 years ago
No longer depends on: 558823
(Assignee)

Updated

7 years ago
blocking2.0: --- → beta1+
(Assignee)

Updated

7 years ago
Assignee: dtownsend → nobody
(Assignee)

Updated

7 years ago
blocking2.0: beta1+ → final+
(Assignee)

Updated

7 years ago
blocking2.0: final+ → beta2+
(Assignee)

Updated

7 years ago
Assignee: nobody → dtownsend

Comment 3

7 years ago
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?)
Keywords: regression
(Assignee)

Updated

7 years ago
blocking2.0: beta2+ → betaN+
(Assignee)

Updated

7 years ago
Duplicate of this bug: 582473
(Assignee)

Comment 5

7 years ago
Needed by string freeze
blocking2.0: betaN+ → beta5+
(Assignee)

Comment 6

7 years ago
Created attachment 464990 [details] [diff] [review]
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.
Attachment #464990 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 7

7 years ago
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.
Status: NEW → ASSIGNED
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 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 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
Attachment #464990 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 11

7 years ago
(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.
(Assignee)

Comment 12

7 years ago
(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.
(Assignee)

Comment 13

7 years ago
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).
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
(Assignee)

Comment 14

7 years ago
Disabled the test due to timeouts on windows 7
(Assignee)

Comment 15

7 years ago
Filed bug 588440 to track an issue where this UI is showing up for new profiles in some cases.
Depends on: 588440
(Reporter)

Comment 16

7 years ago
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?
(Assignee)

Comment 17

7 years ago
(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"
(Reporter)

Comment 18

7 years ago
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.
Status: RESOLVED → VERIFIED
They are not run very often at all. They can probably be simplified to some degree.
(Reporter)

Updated

7 years ago
Depends on: 590998

Updated

7 years ago
Depends on: 597868
You need to log in before you can comment on or make changes to this bug.