330895 - Remove the use of the app.extensions.version pref

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

I'd like to remove all usage of the app.extensions.version preference. Most users don't use this pref to override the app version that is used to check extensions for compatibility and the ones that do end up making extensions incompatible when they have a minVersion and maxVersion defined that will only work with the current application. DOM Inspector and Talkback and the obvious examples but there have been bug reports as well as forum posts from people that state the Extension Manager won't allow them to install an extension they know is compatible when they have this pref set.

bsmedberg and beng, do either of you have a problem with my doing this?

Comment 1

[Benjamin Smedberg]

I don't think this is a good idea. The version pref is useful in various testing circumstances. I would support perhaps pushing a JSconsole warning message if the user has the pref set.

Comment 2

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

Can you provide details of the testing scenarios where this comes in handy? I ask because it may be possible to still provide the same ability while alleviating the issue I am trying to solve.

Comment 3

[Jesse Ruderman]

Robert, are you planning to provide a new way for users to override extension compatibility checks when you remove this pref?

Comment 4

[Hixie (not reading bugmail)]

I need a way to install extensions built for older versions onto trunk builds of arbitrary branches with versions sent to any random thing. Currently, a.e.v provides that.

Comment 5

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

(In reply to comment #3)
> Robert, are you planning to provide a new way for users to override extension
> compatibility checks when you remove this pref?
I hadn't planned on it but if I were to choose between having this or having an override which disables compatibility checking entirely I would prefer having the override. What we have now is normally used for overriding compatibility checking but does so only in part and in a manner that causes other problems.

As for testing, I test extensions as much if not more so than most since I focus on the Extension Manager code and I never use this pref. Instead, I either manually modify my extensions.rdf or use the Nightly Tester Tools extension. As is this pref is not something I think should be used by the Extension Manager though if there was consensus that it is ok for people to override compatibility checking entirely I would be more than happy to implement that when removing this pref check.

Comment 6

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

From what has been stated so far this pref is used for testing purposes. If that is the case then we should make it so it doesn't cause problems for users even though it is the user causing their own problems by using it. There has been discussion of having some functionality only work in the nightly builds which may be an option for this pref.

If it isn't just for testing purposes then changing it to override compatibility checking entirely would allow testing while changing the behavior to be what people are expecting. What happens today for people that aren't testing is along the lines of
* they change the pref to get an extension to install / work
* at some point in the future they upgrade
* extensions that are compatible won't install / work

Though the pref can be valuable for testing I would be very surprised if there weren't more people this has caused problems for vs. people that use it for testing and there are other methods to test such as
* use Nightly Tester Tools or one of the other extensions that have taken the functionality from it (e.g. Local Install, etc.).
* modifying an extension's install.rdf (e.g. open the xpi with a zip reader that supports modifying files with an editor and saving them back to the xpi as well as several other methods).
* extract the xpi into the extensions directory, rename the directory with the extension's id, and modify the minVersion or maxVersion in the install.rdf as appropriate.

Comment 7

[Hixie (not reading bugmail)]

Yeah I would recommend changing the pref to simply a boolean pref "app.extensions.checkVersion" or some such and then when that pref is set to false, make the extension UI have a clear warning that says "Extension Compatibility Checking has been disabled. (( Reenable ))".

Comment 8

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

reassigning to default owner for now in case anyone else might want to take this on *hint* :)

Comment 9

[Dave Townsend [:mossop]]

I wouldn't mind taking a crack at this this weekend unless there are any objections.

Comment 10

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

Dave, you are the person I had in mind! :)

Comment 11

[Dave Townsend [:mossop]]

Attachment: Add ignoreVersions pref

This removes all use of the app.extensions.version pref from the EM and adds the use of extensions.ignoreVersions. When set to true this new pref makes the app allow extensions that normally would be disabled for compatibility reasons. It also allows you to install incompatible extensions and shows a warning notification in the Add-ons window.

A few notes on the behaviour with this patch included:

The pref is currently by default unset which is the same as being set to false.

extensions.lastIgnoreVersions is used to track the state of the pref at the last startup in a similar vein to extensions.lastAppVersion. When a change is noted the EM updates appDisabled for all extensions.

While incompatible add-ons can be installed by the user, the behaviour here will not automatically update to newer versions of an extension that is still normally incompatible with the app.

This setting will allow a user to install an add-on that normally wouldn't ever work with the current app, e.g. installing a Thunderbird extension into Firefox.

The setting will not allow you to enable extensions that either have unsatisfied dependencies or are blacklisted.

The UI continues to show a red warning mark on incompatible extensions, but they can be freely enabled and disabled by the user.

The notification currently shows a question mark, but probably should be a warning exclamation mark, but I am no graphical artist, sorry.

Comment 12

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

Comment on attachment 220341 [details] [diff] [review]
Add ignoreVersions pref

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v
>retrieving revision 1.23
>diff -u -8 -p -r1.23 extensions.properties
>--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties	28 Apr 2006 19:36:11 -0000	1.23
>+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties	30 Apr 2006 21:53:06 -0000
>@@ -116,16 +116,17 @@ updatesAvailableTitle=Updates Found
...
>+ignoreVersionsMsg=Possibly incompatible add-ons are currently enabled.
ignoreVersions isn't a good name through the patch. I'll think of a better name later unless you do. Something along the lines of disableCompatibilityCheck would be better though that is a bit long. I'll think on the msg as well.

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.90
>diff -u -8 -p -r1.90 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	27 Apr 2006 03:04:32 -0000	1.90
>+++ toolkit/mozapps/extensions/content/extensions.js	30 Apr 2006 21:53:09 -0000
>@@ -48,19 +48,21 @@ var gView             = null;
...
>+var gIgnoreVersions   = false;
ditto. I'm not going to point out the other ones.

>@@ -511,16 +517,22 @@ function Startup() 
>     gExtensionsView.scrollBoxObject.scrollToElement(gExtensionsView.selectedItem);
> 
>   if (gInSafeMode) {
>     var addonsMsg = document.getElementById("addonsMsg");
>     addonsMsg.showMessage("chrome://mozapps/skin/extensions/question.png",
>                           getExtensionString("safeModeMsg"),
>                           null, null, true, null);
>   }
>+  else if (gIgnoreVersions) {
>+    var addonsMsg = document.getElementById("addonsMsg");
>+    addonsMsg.showMessage("chrome://mozapps/skin/extensions/question.png",
>+                          getExtensionString("ignoreVersionsMsg"),
>+                          null, null, true, null);
>+  }
> }
These will queue so if there is more than one the last one to be called will be displayed. The following will display the safe mode msg and when that is closed it will display the other msg.
if (gIgnoreVersions) {
  var addonsMsg = document.getElementById("addonsMsg");
  addonsMsg.showMessage("chrome://mozapps/skin/extensions/question.png",
                        getExtensionString("ignoreVersionsMsg"),
                        null, null, true, null);
}
if (gInSafeMode) {
  addonsMsg = document.getElementById("addonsMsg");
  addonsMsg.showMessage("chrome://mozapps/skin/extensions/question.png",
                        getExtensionString("safeModeMsg"),
                        null, null, true, null);
}

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.188
I think it would be appropriate to use the existing pref observer which would allow you to enable / disable all add-ons appropriately when the pref is changed. It should be fine to only have one pref then though it wouldn't handle the case where it is changed while the app isn't running but I'm not concerned about that case.

If you go this route the majority of this patch will change from this point on so I'm stopping the review here until you reply with what you think of this approach.

Comment 13

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

I should mention regarding the addonsmessage that it queues existing messages that haven't been dismissed in order to allow new messages to display. Then when the message is dismissed it will display the queued message.

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #12)
> ignoreVersions isn't a good name through the patch. I'll think of a better name
> later unless you do. Something along the lines of disableCompatibilityCheck
> would be better though that is a bit long. I'll think on the msg as well.

Spent a while thinking up names and actually changed away from compatibility because it was so long, so if you can come up with a good idea that would be great.

> These will queue so if there is more than one the last one to be called will be
> displayed. The following will display the safe mode msg and when that is closed
> it will display the other msg.

I didn't realise that, but I think it might still be correct to only display the warning in normal mode. In safe mode the incompatible extensions aren't enabled so the warning is not necessarily valid. Unless it's useful to have it there to warn those that can't open the add-ons manager in normal mode because of really bad problems?

> I think it would be appropriate to use the existing pref observer which would
> allow you to enable / disable all add-ons appropriately when the pref is
> changed. It should be fine to only have one pref then though it wouldn't handle
> the case where it is changed while the app isn't running but I'm not concerned
> about that case.
> 
> If you go this route the majority of this patch will change from this point on
> so I'm stopping the review here until you reply with what you think of this
> approach.
> 

Thats sounds like a better idea than what I've used here so I'll get on it.

Comment 15

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

Comment on attachment 220341 [details] [diff] [review]
Add ignoreVersions pref

minusing since Dave is going to re-write the patch to use a pref observer. Thanks Dave!

Comment 16

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

beltzner, can Dave and I have your sage advice as to what the string should be for the disabled app compatibility check?

Dave, I ran this by a couple of people and think that the addonsmessage should include the option to enable the compatibility check. You can use the same method as how isXPInstallEnabled does essentially the same thing.

Thanks from me to both of you.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 220341 [details] [diff] [review]
Add ignoreVersions pref

>+ignoreVersionsMsg=Possibly incompatible add-ons are currently enabled.

Suggest:

ignoreExtVersionMsg=Extension compatibility checking is disabled. You may have incompatible extensions.

Comment 18

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

Thanks Mike! May I suggest it Add-on instead of Extension since this applies across the board to all EM managed Add-ons?  Perhaps disabledCompatMsg instead of ignoreExtVersionMsg as well?

Comment 19

[Dave Townsend [:mossop]]

Attachment: Use pref observer to update extensions

As per review comments this uses the pref observer already in the extension manager and appenables/disables the extensions accordingly.

Also adds the enable button to the notification in the add-ons manager.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 221511 [details] [diff] [review]
Use pref observer to update extensions

strings look good to me

Comment 21

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

Comment on attachment 221511 [details] [diff] [review]
Use pref observer to update extensions

Thanks David and this is much closer.

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>--- toolkit/mozapps/extensions/content/extensions.js.orig
>+++ toolkit/mozapps/extensions/content/extensions.js
>@@ -53,9 +53,11 @@ var gDefaultTheme     = "classic/1.0";
> var gDownloadManager  = null;
> var gObserverIndex    = -1;
> var gInSafeMode       = false;
>+var gDisabledCompat   = false;
> var gAppID            = "";
> var gPref             = null;
> 
>+const PREF_EM_DISABLED_COMPATIBILITY        = "extensions.disabledCompatibility";
perhaps PREF_EM_CHECK_COMPATIBILITY and extensions.checkCompatibility with it defaulting to true.

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in.orig
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>@@ -54,7 +54,7 @@ const nsIURL                          = 
> // it has been removed will cause a crash on Mac OS X - bug 292823
> const nsIDirectoryEnumerator          = Components.interfaces.nsIDirectoryEnumerator;
> 
>-const PREF_EM_APP_EXTENSIONS_VERSION  = "app.extensions.version";
>+const PREF_EM_DISABLED_COMPATIBILITY  = "extensions.disabledCompatibility";
Same as previous.

>@@ -2719,6 +2719,8 @@ ExtensionManager.prototype = {
>     case "nsPref:changed":
>       if (data == PREF_EM_LOGGING_ENABLED)
>         this._loggingToggled();
>+      if (data == PREF_EM_DISABLED_COMPATIBILITY)
>+        this._compatDisableToggled();
nit: else if so it doesn't evaluate when PREF_EM_LOGGING_ENABLED changes.

>@@ -2732,6 +2734,33 @@ ExtensionManager.prototype = {
>   },
> 
>   /**
>+   * Update normally incompatible extensions.
>+   */
>+  _compatDisableToggled: function() {
nit: how about _checkCompatToggled and the comment should be along the lines of
Enables or disables extensions that are incompatible depending upon the pref setting for compatibility checking.

>+    var disabledCompatibility = getPref("getBoolPref", PREF_EM_DISABLED_COMPATIBILITY, false);
>+    var ds = this.datasource;
>+
>+    // Enumerate all items
>+    var ctr = getContainer(ds, ds._itemRoot);
>+    var elements = ctr.GetElements();
>+    while (elements.hasMoreElements()) {
>+      var itemResource = elements.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
>+
>+      // Change only those items that are not blocklisted and have no
>+      // unsatisfied dependencies.
>+      id = stripPrefix(itemResource.Value, PREFIX_ITEM_URI);
>+      if (ds.getItemProperty(id, "compatible") == "false" &&
>+          ds.getItemProperty(id, "blocklisted") == "false" &&
>+          ds.getItemProperty(id, "satisfiesDependencies") == "true") {
>+        if (disabledCompatibility)
>+          this._appEnableItem(id);
>+        else
>+          this._appDisableItem(id);
>+      }
>+    }
>+  },
There is similar code elsewhere that this patch doesn't take into account. Perhaps it can be cetralized? For one example _updateDependentItemsForID will app disable items that are not compatible when another item requires the item is enabled even though the pref to disable compatibility checking is set.

>@@ -3555,13 +3584,14 @@ ExtensionManager.prototype = {
>           // Enumerate all items
>           var ctr = getContainer(ds, ds._itemRoot);
>           var elements = ctr.GetElements();
>+          var disabledCompatibility = getPref("getBoolPref", PREF_EM_DISABLED_COMPATIBILITY, false);
nit: checkCompatibiity describes this better.

>@@ -3864,7 +3892,7 @@ ExtensionManager.prototype = {
>       // if the item is already compatible don't attempt to migrate the
>       // item's compatibility info
>       var newRes = getResourceForID(itemsToCheck[i]);
>-      if (ds.isCompatible(ds, newRes))
>+      if (getPref("getBoolPref", PREF_EM_DISABLED_COMPATIBILITY, false) || ds.isCompatible(ds, newRes))
>         continue;
Unless I am missing something this isn't needed since it doesn't disable the extension due to it being incompatible... all this does is migrate the targetApplication minVersion and maxVersion from the toolkit version 1.7 (e.g. Firefox 1.0.x) extensions datasource when the minVersion maxVersion range is not compatible with the current application.

Comment 22

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This addresses the comments given.

I've created the method _isUsableItem which tests whether or not an item can be app enabled and used it in the various places in the code where this check is necessary.

In doing this I've simplified somewhat the check in checkForBlocklistChanges. Firstly the check there had a problem where it would not update the item if appDisabled was OP_NEEDS_DISABLE, and secondly since _appEnableItem checks whether the item needs enabling in the first place I think it's better to just do that checking from there.

This patch also creates a global variable in the EM to keep track of the checkCompatibility pref rather than having to retrieve it everytime, same as the logging pref.

Just a minor thought, it might be worth making _isUsableItem a method on the EM interface. The addons UI could use it to know whether enabling an addon is possible rather than it having to know why and check all those.

Comment 23

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

After a brief run through all I can say is yah! Thanks again Dave for taking this on and I'll review it over the weekend.

Comment 24

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

Comment on attachment 223350 [details] [diff] [review]
patch rev 3

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>@@ -1119,16 +1135,20 @@ var gExtensionsDNDObserver =
> // Notification Messages
> const gAddonsMsgObserver = {
>   observe: function (aSubject, aTopic, aData)
>   {
>     switch (aData) {
>     case "addons-enable-xpinstall":
>       gPref.setBoolPref("xpinstall.enabled", true);
>       break;
>+    case "addons-enable-compatibility":
>+      gPref.setBoolPref(PREF_EM_CHECK_COMPATIBILITY, true);
>+      gCheckCompat = true;
>+      break;
nit: on the one hand if someone sets this pref manually it is nice to keep it around so it is displayed in about:config. On the other hand it is good practice to clear the user pref. I could go either way.


Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
===================================================================
...
>@@ -5182,30 +5205,44 @@ ExtensionManager.prototype = {
...
>+   * Determines whether an item should be appDisabled or not based on it's
>+   * current state.
Let's simplify that and just require reading the code to see what determines whether an item should be disabled by the application. How about:
Determines whether an item should be disabled by the application.

>+   * @param   id
>+   *          The ID of the item whose dependent items will be checked
should read "The ID of the item to check."

>+   */
>+  _isUsableItem: function(id) {
>+    var ds = this.datasource;
>+
>+    // appDisabled is determined by an item being compatible (or compatibility
>+    // checking disabled), satisfying its dependencies, and not being blocklisted
The space and the comment can be removed with the comment fixed above.

I think that the function installItem needs a little love as well for the INSTALLERROR_INCOMPATIBLE_VERSION case - I think the log text is fine but shouldn't the following be added?
if (!gCheckCompatibility)
  em._appDisableItem(id);

With the comments fixed and the above fixed or explained for installItem r=me

Thank you much Dave!

Comment 25

[Dave Townsend [:mossop]]

Attachment: patch rev 4

(In reply to comment #24)
> I think that the function installItem needs a little love as well for the
> INSTALLERROR_INCOMPATIBLE_VERSION case - I think the log text is fine but
> shouldn't the following be added?
> if (!gCheckCompatibility)
>   em._appDisableItem(id);

I've addressed the other comments, but I don't believe that this is the case, _getInstallData will only ever give a INSTALLERROR_INCOMPATIBLE_VERSION if gCheckCompatibility is true, in which case the item should certainly be disabled.

Rob can you just do a quick review of that to make sure I'm not missing something you spotted.

Comment 26

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

Comment on attachment 223681 [details] [diff] [review]
patch rev 4

Looks good and you are correct regarding not needing the extra check. Thanks again!

Comment 27

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

Checked in on trunk and this should bake for a few days before landing on the 1.8.1 branch.

Comment 28

[Atachi Hayashime]

this will break compatibility in 3rd-party browsers, that use Firefox as source-base

imagine:
- uses own version (browser/config/version.txt) which is not representing the Fx-source-version
- uses same APP-Guid as Fx, so that extensions work without rewrite
- uses a.e.v to force the extension-system to identify as the version it's build against

in this scenario all existing extensions created for Fx will work in that browser, but with the change there no longer would be a way to _fake_ the extension-system without completely turning the verify off (which noone really wants) or reimporting the old functionality

for example it would break Flock which fakes to 1.0+ while the real version is somewhere in the 0.5xx, but all versions below 0.8/0.9 will not work, because it's the minimum for the current system

Comment 29

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

That sounds like an unsupported hack at best. The app.extensions.version pref was used for testing purposes as is this new pref. It is definitely use at your own risk. Flock used to have their own app GUID though I haven't checked it for a while.

Comment 30

[Atachi Hayashime]

(In reply to comment #29)
> That sounds like an unsupported hack at best. The app.extensions.version pref
> was used for testing purposes as is this new pref. It is definitely use at your
> own risk. Flock used to have their own app GUID though I haven't checked it for
> a while.
> 

flock uses own GUID, but still needs to fake, cause their version-number is below the boundaries of Fx's extension-system (which says no 'extensions for below 0.9').

try it out and build with 0.6 in version.txt and any GUID (or just use the one of Fx) ...no extensions will work/install in that build even if correctly written.
the only way to fix that to date, was a.e.v

(in my case, the version-tag i currently use fo my browser internally is 0.2.3 and fakes to the latest 2.0ax)

Comment 31

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

Sunbird is using 0.3a2 without experiencing the problem you described and even if that is true that is a bug you should have reported instead of using a hack to workaround it.

Comment 32

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

Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0

Comment 33

[Grey Nicholson (they/them)]

So what name was chosen for the pref in the end?

Comment 34

[Grey Nicholson (they/them)]

Answering my own question: it's extensions.checkCompatibility, to be set to false. (Hooray for MozillaZine Forums http://forums.mozillazine.org/viewtopic.php?p=2303850#2303850 )