340055 - enable compatible extensions on downgrade

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

There have been a couple of bug reports and forum posts from people that have tried out bon echo and then downgraded back to a 1.5.0.x build which then leaves their extensions app disabled. This is due to the 1.5.0.x EM not re-enabling compatible extensions on downgrade and this is fixed for 2.0 by part of the patch in bug 271166. I think it would be a good thing to backport that portion of the patch before Firefox 2.0 is released to prevent an potentially large number of people from experiencing this bug.

Comment 1

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

The workaround when this happens is to open the profile directory and delete the extensions.rdf file. The profile directory can be found using the instructions available at
http://www.mozilla.org/support/firefox/profile

Comment 2

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

requesting blocking 1.8.0.5... I'll put together a patch for this sometime in the next few days.

The current user experience on downgrade after running a newer build is bad... we disable their incompatible extensions on upgrade and then don't re-enable them on downgrade on the 1.8.0 branch. This has already been fixed on the 1.8.1 branch.

Comment 3

[Daniel Veditz [:dveditz]]

Not blocking 1.8.0.5, but if you get a reviewed patch in before tomorrow (our deadline for non-blocker approvals) we'll consider it. There may be a day or two leeway if the patch is small.

Comment 4

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

Attachment: patch

This is for the MOZILLA_1_8_0_BRANCH only.

Comment 5

[Benjamin Smedberg]

Comment on attachment 225512 [details] [diff] [review]
patch

rob, I think this is too risky for 1.8.0.5 at this point. I'd need to review this really carefully to make sure there are no common situations that might throw. I'll take a look for 1.8.0.6.

Comment 6

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

Sounds fine. My main concern is that when 2.0 is released we will end up with a subset of people that upgrade and then downgrade to the previous release which will then leave their extensions disabled which this patch fixes. Thanks

Comment 7

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

renominating after discussion with dveditz

Comment 8

[Daniel Veditz [:dveditz]]

Need this reviewed ASAP

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Would be great if we could get comments from whoever reviews this if this is safe for the 1.8.0.5 release or not.

Comment 10

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

Comment on attachment 225512 [details] [diff] [review]
patch

Some notes to assist with the review:

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
...
>-    // Disable all incompatible items and let update enable them if appropriate.
>     var ds = this.datasource;
>-    var currAppID = gApp.ID;
>-    var items = ds.getIncompatibleItemList(currAppID, currAppVersion,
>-                                           nsIUpdateItem.TYPE_ADDON, true);
>-    for (var i = 0; i < items.length; ++i)
>-      ds.setItemProperty(items[i].id, EM_R("appDisabled"), EM_L("true"));
Since 1.0 the EM has only disabled extensions on upgrade (e.g. getIncompatibleItemList only returns incompatible items and on downgrade there may be items that were incompatible and are now compatible) and never took into account the fact that users may downgrade.

>+    var allAppManaged = true;
>+    var ctr = getContainer(ds, ds._itemRoot);
>+    var elements = ctr.GetElements();
>+    while (elements.hasMoreElements()) {
>+      var itemResource = elements.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
>+      var id = stripPrefix(itemResource.Value, PREFIX_ITEM_URI);
>+      if (allAppManaged && ds.getItemProperty(id, "appManaged") != "true")
>+        allAppManaged = false;
>+      if (ds.getItemProperty(id, "compatible") == "true") {
>+        if (ds.getItemProperty(id, "appDisabled"))
>+          ds.setItemProperty(id, EM_R("appDisabled"), null);
>+      }
>+      else if (!ds.getItemProperty(id, "appDisabled"))
>+        ds.setItemProperty(id, EM_R("appDisabled"), EM_L("true"));
>+    }
Here we enumerate all items in the extensions datasource, set allAppManaged to false if we have items that are not managed by the app, and set appEnable / appDisable for the items in the extensions datasource based on whether the item is compatible or not and doesn't already have the property set. This is essentially the same code that has been on both trunk and 1.8.1 branch for quite some time now with the difference being that on the trunk and 1.8.1 branch we use a helper function since whether an item is app disabled or not is determined by whether it is compatible, satisfies its dependencies, and is not blocklisted.

>-    // Always check for compatibility updates when upgrading
>-    this._showMismatchWindow();
>+    // Always check for compatibility updates when upgrading if we have add-ons
>+    // that aren't managed by the application.
>+    if (!allAppManaged)
>+      this._showMismatchWindow();
App managed items are never updated by the extension manager so we don't display the compatibility wizard when there are no items in the extensions datasource if all items are app managed.

Comment 11

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

(In reply to comment #10)
> App managed items are never updated by the extension manager so we don't
> display the compatibility wizard when there are no items in the extensions
> datasource if all items are app managed.
Should read as:
App managed items are never updated by the extension manager so we don't display the compatibility wizard when there are only app managed items in the extensions datasource.

Comment 12

[Benjamin Smedberg]

Why are we pushing this so hard now?

Comment 13

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

Comment on attachment 225512 [details] [diff] [review]
patch

Requesting approval for 1.8.0.5 per discussion with dveditz.

Comment 14

[Jay Patel [:jay]]

Comment on attachment 225512 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=jay for drivers.

Comment 15

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

Checked in on MOZILLA_1_8_0_BRANCH for Firefox 1.5.0.5

Comment 16

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

Checked in on MOZILLA_1_8_0_BRANCH for Firefox 1.5.0.5(In reply to comment #12)
> Why are we pushing this so hard now?
This will affect anyone that has extensions that aren't compatible with 2.0b1, tries out Firefox 2.0 beta 1, and then goes back to using an official release... so, with Firefox 2.0 beta 1 being released shortly and with more people expected to try out beta 1 it is believed this would affect more people.

Comment 17

[juan becerra [:juanb]]

Verified using: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.5) Gecko/20060626 Firefox/1.5.0.5

I used a 1.5.0.4 build and installed several extensions. Then I launched Bon Echo using the same profile. It disabled some of the extensions that were not compatible. Then I went back to using 1.5.0.4 and the extensions that were not compatible with Bon Echo were disabled by default. With the new build(1.5.0.4) those extensions are enabled on launch.

Comment 18

[Mike Cowperthwaite]

Thanks for getting this onto the branch for TB 1.5!  I noticed the change of behavior right away, as I'm constantly switching between versions.