Closed Bug 1281884 Opened 8 years ago Closed 8 years ago

strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on"

Categories

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

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: wbamberg, Assigned: bsilverberg)

References

Details

(Whiteboard: version, triaged)

Attachments

(1 file)

Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",
58 bytes, text/x-review-board-request
aswan
: review+
Details 
In bug 1192437, add-ons can set "strict_min_version" and "strict_max_version" to control the Firefox versions in which the add-on can be installed.

This works fine for built XPIs, but does not work if you install the add-on using "Load Temporary Add-on" - Firefox seems to ignore these keys and just install the add-on anyway.

It seems like it would be a good thing for temporary installation to be as close as possible to proper installation.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/62146/#review58890

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3928
(Diff revision 1)
>     *         same ID is already temporarily installed
>     */
>    installTemporaryAddon: Task.async(function*(aFile) {
>      let addon = yield loadManifestFromFile(aFile, TemporaryInstallLocation);
>  
> +    if (!addon.isCompatible) {

This change creates a problem for other tests which do not specify an explicit minVersion. The default minVersion currently is `42.0a1`, which is much higher than the `1` currently used by the tests. I'm not sure if this is something we need to fix in the tests or in the code. Luca suggested we might want to only enforce this compatibility check if the manifest does in fact provide a value for min or max version, but at this point in the code we don't seem to have access to that information anymore.
https://reviewboard.mozilla.org/r/62146/#review59156

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:196
(Diff revision 1)
>  });
> +
> +
> +// Test that strict_min_version and strict_max_version are enforced for,
> +// loading temporary extension.
> +add_task(function* test_strict_min_max() {

I think it would make more sense for this to be in test_temporary.js

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:198
(Diff revision 1)
> +
> +// Test that strict_min_version and strict_max_version are enforced for,
> +// loading temporary extension.
> +add_task(function* test_strict_min_max() {
> +  if (!TEST_UNPACKED) {
> +    do_print("This test does not apply when using packed extensions");

why not?
https://reviewboard.mozilla.org/r/62146/#review58890

> This change creates a problem for other tests which do not specify an explicit minVersion. The default minVersion currently is `42.0a1`, which is much higher than the `1` currently used by the tests. I'm not sure if this is something we need to fix in the tests or in the code. Luca suggested we might want to only enforce this compatibility check if the manifest does in fact provide a value for min or max version, but at this point in the code we don't seem to have access to that information anymore.

Can you elaborate?  Do you mean that this change breaks other existing tests?
In any case, only doing the check if versions are specified makes sense, you could do the test only if minVersion/maxVersion != undefined
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62146/diff/1-2/
https://reviewboard.mozilla.org/r/62146/#review58890

> Can you elaborate?  Do you mean that this change breaks other existing tests?
> In any case, only doing the check if versions are specified makes sense, you could do the test only if minVersion/maxVersion != undefined

Yes, for example, `test_implicit_id_temp` fails because the error introduced by this change is thrown: https://gist.github.com/bobsilverberg/6b405a294c01ba09eff3230da2e0ec48

The problem with checking `minVersion` and/or `maxVersion` is that, by the time the checking code is reached, those properties have already been filled with their default values, so we don't know whether the manifest supplied a specific value or not. By the time the code runs at line 3928 of `installTemporaryAddon` we only have the `addon` object and no longer have access to the raw manifest data (unless it's hidden away somewhere inside the `addon` object that I cannot find). 

I think that if we want to be able to know whether or not specific versions were provided for an extension in the manifest file we’ll need to change `loadManifestFromWebManifest` to store that fact in the `addon` object that it returns, which doesn't feel ideal to me.

I’m not sure if that’s the best approach, or if there’s something else we can do. 

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#898
https://reviewboard.mozilla.org/r/62146/#review59156

> I think it would make more sense for this to be in test_temporary.js

It looks like everything else in `test_temporary.js` is testing a non-WebExtension add-on, whereas `test_webextension_install.js` seems to be tests for installing WebExtensions, which is why I put it there. Also, `test_webextension_install.js` does already include a number of tests that use `installTemporaryAddon()`. Are you sure it is better in `test_temporary.js`?

> why not?

I assumed that had to do with loading an extension from a folder, rather than an XPI, but I guess it's referring to something else. I have removed that code.
https://reviewboard.mozilla.org/r/62146/#review58890

> Yes, for example, `test_implicit_id_temp` fails because the error introduced by this change is thrown: https://gist.github.com/bobsilverberg/6b405a294c01ba09eff3230da2e0ec48
> 
> The problem with checking `minVersion` and/or `maxVersion` is that, by the time the checking code is reached, those properties have already been filled with their default values, so we don't know whether the manifest supplied a specific value or not. By the time the code runs at line 3928 of `installTemporaryAddon` we only have the `addon` object and no longer have access to the raw manifest data (unless it's hidden away somewhere inside the `addon` object that I cannot find). 
> 
> I think that if we want to be able to know whether or not specific versions were provided for an extension in the manifest file we’ll need to change `loadManifestFromWebManifest` to store that fact in the `addon` object that it returns, which doesn't feel ideal to me.
> 
> I’m not sure if that’s the best approach, or if there’s something else we can do. 
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#898

The code that adds the default values is specific to webextensions, I would suggest just removing it and letting those fields be undefined.  That's going to require some cascading changes to compatibility checks and perhaps to upgrade checks, but if those changes aren't too extensive, that seems like the right route to take.
https://reviewboard.mozilla.org/r/62146/#review59272

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:193
(Diff revision 2)
>    do_check_neq(addon, null);
>  
>    addon.uninstall();
>  });
> +
> +// Test that strict_min_version and strict_max_version are enforced for,

nitpick: no comma

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:213
(Diff revision 2)
> +  try {
> +    yield AddonManager.installTemporaryAddon(addonDir);
> +    do_throw("Installing a non-compatible add-on should return a rejected promise");
> +  } catch (err) {
> +    do_check_eq(err.message, "Add-on strict_min_max@tests.mozilla.org is not compatible with application version. " +
> +                "add-on minVersion: 0.1, add-on maxVersion: 1");
> +  }

You can use Assert.rejects for this
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Assert.jsm#rejects()
https://reviewboard.mozilla.org/r/62146/#review58890

> The code that adds the default values is specific to webextensions, I would suggest just removing it and letting those fields be undefined.  That's going to require some cascading changes to compatibility checks and perhaps to upgrade checks, but if those changes aren't too extensive, that seems like the right route to take.

Just to be sure I understand, are you suggesting getting rid of `webExtensionsMinPlatformVersion` [1] entirely from the codebase, so that any time WebExtensions are referenced the default minimum version will be -infinity? It looks like it's only used by `loadManifestFromWebManifest` [2] in `XPIProvider` and `parseJSONManifest` [3] in `AddonUpdateChecker`, so not a huge change, but I wonder why it was implemented in the first place and what we're losing by doing away with a minimum default value?

[1] https://dxr.mozilla.org/mozilla-central/search?q=webExtensionsMinPlatformVersion&redirect=false
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#979
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm#516
(In reply to Bob Silverberg [:bsilverberg] from comment #10)
> https://reviewboard.mozilla.org/r/62146/#review58890
> 
> > The code that adds the default values is specific to webextensions, I would suggest just removing it and letting those fields be undefined.  That's going to require some cascading changes to compatibility checks and perhaps to upgrade checks, but if those changes aren't too extensive, that seems like the right route to take.
> 
> Just to be sure I understand, are you suggesting getting rid of
> `webExtensionsMinPlatformVersion` [1] entirely from the codebase, so that
> any time WebExtensions are referenced the default minimum version will be
> -infinity? It looks like it's only used by `loadManifestFromWebManifest` [2]
> in `XPIProvider` and `parseJSONManifest` [3] in `AddonUpdateChecker`, so not
> a huge change, but I wonder why it was implemented in the first place and
> what we're losing by doing away with a minimum default value?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/
> search?q=webExtensionsMinPlatformVersion&redirect=false
> [2]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#979
> [3]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/AddonUpdateChecker.jsm#516

I wasn't suggesting making it -infinity, I was suggesting making it undefined and changing the compatibility check logic to only check min/max versions if they are specified.
I'm pretty sure that code is only there since it was the path of least resistance for adding web extensions to the exsiting add-ons manager code since those properties are mandatory in install.rdf.
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62146/diff/2-3/
https://reviewboard.mozilla.org/r/62146/#review58890

> Just to be sure I understand, are you suggesting getting rid of `webExtensionsMinPlatformVersion` [1] entirely from the codebase, so that any time WebExtensions are referenced the default minimum version will be -infinity? It looks like it's only used by `loadManifestFromWebManifest` [2] in `XPIProvider` and `parseJSONManifest` [3] in `AddonUpdateChecker`, so not a huge change, but I wonder why it was implemented in the first place and what we're losing by doing away with a minimum default value?
> 
> [1] https://dxr.mozilla.org/mozilla-central/search?q=webExtensionsMinPlatformVersion&redirect=false
> [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#979
> [3] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm#516

The one test that is still failing is `test_disabled_addon_can_be_enabled_after_reload` in `test_reload.js` [1]. The purpose of this test seems to be to prove that an add-on will change from disabled to enabled if you temporarily load it with an incompatible version (which is what causes it to be disabled) and then reload it with an updated manifest that makes it compatible. With the change introduced in this patch, it is no longer possible to temporarily load an incompatible add-on, which suggests that this test is no longer valid.

Do you agree, and should this test simply be removed, or should we try to find another way to temporarily load an add-on so that it will end up in a disabled state? Or perhaps we could set it to disabled after loading it, but I'm not sure if that would result in the same test.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_reload.js#93
Just in case this fell off your radar, aswan, there's still an outstanding question for you about tests in the review.
Flags: needinfo?(aswan)
Re comment 13, I do think that sounds reasonable but would like to get Kumar's take too.
Flags: needinfo?(aswan) → needinfo?(kumar.mcmillan)
Re comment 13, the version incompatibility part of that test is not important. The tricky part was that I didn't know how to set appDisabled=true. Is there a way to do that more directly with the AddonManager maybe? I think the test would be worth keeping *if* it's still technically possible to get a temporarily installed add-on into an appDisabled state. If it's not possible to enter that state then I'd say delete the test. One possibility is if the ID of the temporary add-on was blocklisted.
Flags: needinfo?(kumar.mcmillan)
Actually, that test was there originally to cover the logic of how reload() explicitly needs to set appDisabled = true but the implementation has since switched to using installTemporaryAddon() so maybe it's not important.
Blocks: 1286301
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62146/diff/3-4/
https://reviewboard.mozilla.org/r/62146/#review60778

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6886
(Diff revision 4)
>        return false;
>  
> +    if (app.minVersion == undefined && app.maxVersion == undefined) {
> +      return true;
> +    } else {
> +      app.minVersion = app.minVersion || "0";

I had to add this in to deal with situations in which only one of `minVersion` or `maxVersion` is specified. I'm not sure that "0" is the best value to use, as the code I am moving from actually uses `AddonManagerPrivate.webExtensionsMinPlatformVersion` which ends up being 42-ish. I think if our intent is that a missing value means that anything is allowed then "0" is probably okay.

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:93
(Diff revision 4)
>    yield Assert.rejects(addon.reload(), /Only temporary add-ons can be reloaded/);
>  
>    yield tearDownAddon(addon);
>  });
>  
> -add_task(function* test_disabled_addon_can_be_enabled_after_reload() {
> +add_task(function* test_invalid_version_cannot_be_reloaded() {

As per discussions on IRC with Kumar, this test is a replacement for the other one, which doesn't seem to be needed and in any case we cannot figure out how to reproduce. This test checks that reloading an add-on with invalid versions fails, and leaves the previous incarnation of the add-on in place. I am having an issue with this test though, in that it passes the first time but not the second, and I'm not sure why. Each of the tests in this file are actually executed twice during a test run. The first execution is fine, but the second one fails because it does not detect the rejection at line 127. I have no idea why this is happening. Can you shed any light?

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:266
(Diff revision 4)
> +  do_check_neq(addon, null);
> +  do_check_eq(addon.id, addonId);
> +  addon.uninstall();
> +  addonDir.remove(true);
> +
> +  // good only min

I added tests for valid manifests that only specify either a min or a max as I discovered there was a bug with this, and it wasn't covered by tests.
https://reviewboard.mozilla.org/r/62146/#review60856

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6883
(Diff revisions 1 - 4)
> +    if (app.minVersion == undefined && app.maxVersion == undefined) {
> +      return true;
> +    } else {
> +      app.minVersion = app.minVersion || "0";
> +      app.maxVersion = app.maxVersion || "*";
> +    }

instead of checking this up here, why don't you leave min/max alone and test for undefined right before actually testing them individually?  (ie lines 6932-3 in this version of the patch)
https://reviewboard.mozilla.org/r/62146/#review60856

> instead of checking this up here, why don't you leave min/max alone and test for undefined right before actually testing them individually?  (ie lines 6932-3 in this version of the patch)

I don't understand the suggestion. `app.minVersion` and `app.maxVersion` are each used more than once further down in the code. They're not only used at 6932-3, but also at 6926 and 6929, so they need to be dealt with for those lines as well. The latter are wrapped inside an `if` (line 6903) which does not include the former, so I either need to deal with each of them twice, or set a default for them above the `if`, which is essentially what I am doing now. I chose to put the code where I did because it felt to me like a good logical place to put it. We are already dealing with minVersion and maxVersion (line 6883), so it seemed logical to finish dealing with them then and there.
Priority: -- → P2
Whiteboard: version, triaged
https://reviewboard.mozilla.org/r/62146/#review60856

> I don't understand the suggestion. `app.minVersion` and `app.maxVersion` are each used more than once further down in the code. They're not only used at 6932-3, but also at 6926 and 6929, so they need to be dealt with for those lines as well. The latter are wrapped inside an `if` (line 6903) which does not include the former, so I either need to deal with each of them twice, or set a default for them above the `if`, which is essentially what I am doing now. I chose to put the code where I did because it felt to me like a good logical place to put it. We are already dealing with minVersion and maxVersion (line 6883), so it seemed logical to finish dealing with them then and there.

Right, I understand you would have to check for them in multiple places, but it still feels neatest to me.
If you really want to keep the current approach, how about using "*" for both min and max?
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62146/diff/4-5/
https://reviewboard.mozilla.org/r/62146/#review60856

> Right, I understand you would have to check for them in multiple places, but it still feels neatest to me.
> If you really want to keep the current approach, how about using "*" for both min and max?

I would prefer to keep the current approach. I prefer to have code whose execution costs next to nothing possibly run needlessly, rather than to add duplication later on in the method. Also, it feels safer to me to know that a valid default is set so that we don't need to worry about it anywhere else in the method. If the implementation changes further down in the code, we don't need to worry about checking for `undefined` because we'll have a valid value. Unfortunately `*` doesn't work for `minVersion` because it means infinity [1], so I've kept it exactly as is.

[1] http://searchfox.org/mozilla-central/source/xpcom/base/nsIVersionComparator.idl#15
https://reviewboard.mozilla.org/r/62146/#review60778

> As per discussions on IRC with Kumar, this test is a replacement for the other one, which doesn't seem to be needed and in any case we cannot figure out how to reproduce. This test checks that reloading an add-on with invalid versions fails, and leaves the previous incarnation of the add-on in place. I am having an issue with this test though, in that it passes the first time but not the second, and I'm not sure why. Each of the tests in this file are actually executed twice during a test run. The first execution is fine, but the second one fails because it does not detect the rejection at line 127. I have no idea why this is happening. Can you shed any light?

I fixed this problem by requesting that this test not be run with packed extensions. Kumar, do you mind taking a look at this test to see if it does what we discussed in IRC?
Attachment #8767739 - Flags: review?(kumar.mcmillan)
(In reply to Bob Silverberg [:bsilverberg] from comment #25)
> https://reviewboard.mozilla.org/r/62146/#review60778
> 
> > As per discussions on IRC with Kumar, this test is a replacement for the other one, which doesn't seem to be needed and in any case we cannot figure out how to reproduce. This test checks that reloading an add-on with invalid versions fails, and leaves the previous incarnation of the add-on in place. I am having an issue with this test though, in that it passes the first time but not the second, and I'm not sure why. Each of the tests in this file are actually executed twice during a test run. The first execution is fine, but the second one fails because it does not detect the rejection at line 127. I have no idea why this is happening. Can you shed any light?
> 
> I fixed this problem by requesting that this test not be run with packed
> extensions. Kumar, do you mind taking a look at this test to see if it does
> what we discussed in IRC?

Kumar, can you also comment on the fact that the test does not pass when run with a packed extension? The call to reload() does not generate a rejected promise when run in packed mode. Andrew and I discussed this, and came to the conclusion that it is because reload does not support xpi files, and only extensions loaded from a directory. Would you expect the call to reload() to simply be ignored, which is what seems to be happening, or would you expect some sort of exception when trying to do reload with a packed extension?
Flags: needinfo?(kumar.mcmillan)
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review61296

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:94
(Diff revision 5)
>  
>    yield tearDownAddon(addon);
>  });
>  
> -add_task(function* test_disabled_addon_can_be_enabled_after_reload() {
> +add_task(function* test_invalid_version_cannot_be_reloaded() {
> +  if (!TEST_UNPACKED) {

Huh. I'm not sure why this test won't work with packed add-ons. There was a recent bug fix related to reloading XPIs that would definitely affect this. Do you have this patch in your base? https://bugzilla.mozilla.org/show_bug.cgi?id=1283897

If you're still blocked on it, having test coverage for the unpacked case is better than nothing. You should maybe add a comment here though saying something more truthful than "does not apply when using packed extensions."

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:101
(Diff revision 5)
> +    return;
> +  }
>    yield promiseRestartManager();
>    let tempdir = gTmpD.clone();
>  
> -  // Create an add-on with strictCompatibility which should cause it
> +  const addonId = "invalid_version_cannot_be_reloaded@tests.mozilla.org";

nit: this setup code would be less confusing if you add a comment explaining how the *first* installation will have a *valid* app version.
Attachment #8767739 - Flags: review?(kumar.mcmillan) → review+
I added my comments on it
Flags: needinfo?(kumar.mcmillan)
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62146/diff/5-6/
Attachment #8767739 - Flags: review+
https://reviewboard.mozilla.org/r/62146/#review61296

> Huh. I'm not sure why this test won't work with packed add-ons. There was a recent bug fix related to reloading XPIs that would definitely affect this. Do you have this patch in your base? https://bugzilla.mozilla.org/show_bug.cgi?id=1283897
> 
> If you're still blocked on it, having test coverage for the unpacked case is better than nothing. You should maybe add a comment here though saying something more truthful than "does not apply when using packed extensions."

I was working off an old version of m-c. I updated and rebased and now the test runs fine in packed mode, so I removed the `if`.
https://reviewboard.mozilla.org/r/62146/#review62010

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3929
(Diff revision 6)
>    installTemporaryAddon: Task.async(function*(aFile) {
>      if (aFile.exists() && aFile.isFile()) {
>        flushJarCache(aFile);
>      }
>      let addon = yield loadManifestFromFile(aFile, TemporaryInstallLocation);
> +    let app = addon.matchingTargetApplication;

Move this inside the if, its not used anywhere else

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6888
(Diff revision 6)
>    isCompatibleWith: function(aAppVersion, aPlatformVersion) {
>      let app = this.matchingTargetApplication;
>      if (!app)
>        return false;
>  
> +    if (app.minVersion == undefined && app.maxVersion == undefined) {

By moving the test here, you skip the "compatibility overrides" section below.  I have no idea what that code is about but are you sure this doesn't change the overall behavior in an undesirable way?

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:93
(Diff revision 6)
>    yield Assert.rejects(addon.reload(), /Only temporary add-ons can be reloaded/);
>  
>    yield tearDownAddon(addon);
>  });
>  
> -add_task(function* test_disabled_addon_can_be_enabled_after_reload() {
> +add_task(function* test_invalid_version_cannot_be_reloaded() {

On first reading I was confused by this name.
How about something like test_reload_to_invalid_version_fails ?
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62146/diff/6-7/
https://reviewboard.mozilla.org/r/62146/#review62010

> By moving the test here, you skip the "compatibility overrides" section below.  I have no idea what that code is about but are you sure this doesn't change the overall behavior in an undesirable way?

I don't follow how I am skipping the "compatibility overrides" section by putting the test here. All this does is set some defaults, it does not prevent the check at line 6908 or 6914 from running (which is the code I assume you are talking about). That code does not reference `app.minVersion` or `app.maxVersion` so I'm not sure how setting a default value for those above could have an impact on that code.
https://reviewboard.mozilla.org/r/62146/#review62010

> I don't follow how I am skipping the "compatibility overrides" section by putting the test here. All this does is set some defaults, it does not prevent the check at line 6908 or 6914 from running (which is the code I assume you are talking about). That code does not reference `app.minVersion` or `app.maxVersion` so I'm not sure how setting a default value for those above could have an impact on that code.

On line 6889 you return true immediately.  Are you sure that if that happens, that for instance the return false on line 6920 could not have been hit?
https://reviewboard.mozilla.org/r/62146/#review62010

> On line 6889 you return true immediately.  Are you sure that if that happens, that for instance the return false on line 6920 could not have been hit?

Oh! I thought you were talking about the setting of the defaults. I didn't even look at that first `return`, which I admit, is the exact line that you commented on. Sorry about that. I think I made that assumption because this code with the `if` is much older and I thought you'd already reviewed it.

Now I see what you're talking about and yeah, I don't know what that code is for either, but it does look like it could be circumvented. That's a pain. I'll see what I can do about that.
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62146/diff/7-8/
https://reviewboard.mozilla.org/r/62146/#review62010

> Oh! I thought you were talking about the setting of the defaults. I didn't even look at that first `return`, which I admit, is the exact line that you commented on. Sorry about that. I think I made that assumption because this code with the `if` is much older and I thought you'd already reviewed it.
> 
> Now I see what you're talking about and yeah, I don't know what that code is for either, but it does look like it could be circumvented. That's a pain. I'll see what I can do about that.

That original condition which returned true was in place before I realized I needed to have default values for the variables, so I think the simple solution is just to remove the `if` and allow the rest of the pre-existing logic to do its thing. I have updated the patch accordingly.
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review64878

I think the tests in test_webextension_install.js would be easier to follow if, instead of mutating a single manifest from step to step, you make manifest a const and then use Object.assign() to create variants of it for each individual test case.  (in that way each test case would be self-contained and a reader doesn't have to read backward to track the evolving state of manifest to figure out what's being tested)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6889
(Diff revision 8)
>      let app = this.matchingTargetApplication;
>      if (!app)
>        return false;
>  
> +    // set reasonable defaults for app.minVersion and app.maxVersion
> +    app.minVersion = app.minVersion || "0";

Checking compatibility should not have side effects (i.e., changing the contents of this.matchingTargetApplication).  Please create local variables for minVersion and maxVersion and use those.

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:129
(Diff revision 8)
> +  manifest.applications.gecko.strict_max_version = "1";
> +  manifest.version = "2.0";
> +
> +  addonDir = writeWebManifestForExtension(manifest, tempdir, "invalid_version");
> +
> +  yield Assert.rejects(addon.reload(),

The documentation for Assert.rejects isn't very good but in this form, your second argument is just the message that gets displayed, it isn't actually tested against the contents of the thrown exception.  I think passing a regular expression here is probably the right thing.  (And adding a third argument with a message to explain what's being tested would be good too).
This applies to all the uses of Assert.rejects() below.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:220
(Diff revision 8)
> +  // bad min good max
> +  manifest.applications.gecko.strict_min_version = "1";
> +  manifest.applications.gecko.strict_max_version = "1";
> +  let addonDir = writeWebManifestForExtension(manifest, gTmpD,
> +                                              "the-addon-sub-dir");
> +
> +  yield Assert.rejects(AddonManager.installTemporaryAddon(addonDir),
> +                       "Add-on strict_min_max@tests.mozilla.org is not compatible with application version. " +
> +                       "add-on minVersion: 2, add-on maxVersion: 2");

The comment describing the test, the message in rejects() and what the test actually does are not consistent with each other here.
Attachment #8767739 - Flags: review?(aswan) → review-
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review64878

Okay, I have updated the code. I agree it's much nice now. :)
(In reply to Bob Silverberg [:bsilverberg] from comment #40)
> 
> Okay, I have updated the code. I agree it's much nice now. :)

Ugh, nicer!
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review68814

Looks good, thanks!
Attachment #8767739 - Flags: review?(aswan) → review+
Thanks Andrew!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a11a0035c85e
strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on". r=aswan
Keywords: checkin-needed
I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/794db78767b9 for xpcshell failures: 
https://treeherder.mozilla.org/logviewer.html#?job_id=11134341&repo=fx-team


It's entirely possible these are only happening on the Windows VM images, though the non-VM Windows XPCShell tests have not started running yet, so it could be all Windows XPCshell tests.
Flags: needinfo?(bob.silverberg)
Notes to myself:

Log extract for the failure of test_reload_to_invalid_version can be found at [1].
Log extract for the failure of test_strict_min_max can be found at [2].

It looks like there may be a problem rewriting the extension file via `writeWebManifestForExtension` in test_reload_to_invalid_version, which I am going to attempt to fix by using the profile directory instead of the temp directory (as is done at [3]).

It looks like the issue in test_strict_min_max is also related to file access, and I am also going to try to resolve it by switching to using the profile directory.

[1] https://gist.github.com/bobsilverberg/51454f8fc5215b72297df3a1db7e51a8#file-test_reload_to_invalid_version_fails-log
[2] https://gist.github.com/bobsilverberg/51454f8fc5215b72297df3a1db7e51a8#file-test_strict_min_max-txt
[3] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_webextension.js#11
Flags: needinfo?(bob.silverberg)
The change I made didn't make any difference to the failures in Windows 7. [1]

Andrew or Kris, do either of you have any ideas about what might be the problem and how I might fix it?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=74c140bc5730&selectedJob=25826386
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
You don't want to write to the profile directory, but you do need to clean up anything that you write to the temp directory.
Flags: needinfo?(aswan)
Thanks Andrew. I switched back to using the temp directory, and added a statement to remove what I created, e.g.,  `addonDir.remove(true);` on line 121 of test_reload.js, but now the exception is being thrown when trying to remove that folder (which is actually what was also happening before with test_webextension_install.js). Is there something different I need to do to clean up the temp folder?
Flags: needinfo?(kmaglione+bmo) → needinfo?(aswan)
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review69986

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:123
(Diff revisions 11 - 12)
>    equal(addon.version, "1.0");
>    equal(addon.appDisabled, false);
>    equal(addon.userDisabled, false);
> +
> +  // flush JAR cache and remove the file
> +  Services.obs.notifyObservers(addonDir, "flush-cache-entry", null);

I don't think this is right, this test simulates a sequence of actions a user might take and flushing a jar cache entry is not an action that a user can take, it needs to be done for them inside the AddonManager at the appropriate moment.
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review59156

> It looks like everything else in `test_temporary.js` is testing a non-WebExtension add-on, whereas `test_webextension_install.js` seems to be tests for installing WebExtensions, which is why I put it there. Also, `test_webextension_install.js` does already include a number of tests that use `installTemporaryAddon()`. Are you sure it is better in `test_temporary.js`?

That felt more logical to me but yeah, both files seem relevant here so I'm fine either way
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review69986

> I don't think this is right, this test simulates a sequence of actions a user might take and flushing a jar cache entry is not an action that a user can take, it needs to be done for them inside the AddonManager at the appropriate moment.

I'm not sure that I agree. This purpose of this test is to prove that a reload will fail if an extension's manifest has changed on disk to make it no longer compatible. It seems like, regardless of what we do to clear the cache, which is just to allow the file to be rewritten, this test still proves that. It's true that the entire flow of the test does not duplicate what a user might do, but I'm not sure that that renders it invalid.

Having said that, if you have a suggestion of another way around this issue, which would allow us to remove this cache-flushing code, I would be happy to give it a try. Based on my try runs this does seem to have fixed the problem with the tests on Windows 7.

I'm still not sure why this is needed just on that platform, nor why other tests in the same file which write add-ons to disk don't suffer from the same problem. For example, `test_manifest_changes_are_refreshed` does something similar, albeit with a non-webextension add-on, and doesn't seem to encounter the same problem. Do you know why that might be?

[1] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_reload.js#147
Just so I don't lose track of it, the try run that shows the flushing fixing the problem is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=999ea16d55eefcd9cbdfb060e4d4d6a317e1567f.
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review69986

> I'm not sure that I agree. This purpose of this test is to prove that a reload will fail if an extension's manifest has changed on disk to make it no longer compatible. It seems like, regardless of what we do to clear the cache, which is just to allow the file to be rewritten, this test still proves that. It's true that the entire flow of the test does not duplicate what a user might do, but I'm not sure that that renders it invalid.
> 
> Having said that, if you have a suggestion of another way around this issue, which would allow us to remove this cache-flushing code, I would be happy to give it a try. Based on my try runs this does seem to have fixed the problem with the tests on Windows 7.
> 
> I'm still not sure why this is needed just on that platform, nor why other tests in the same file which write add-ons to disk don't suffer from the same problem. For example, `test_manifest_changes_are_refreshed` does something similar, albeit with a non-webextension add-on, and doesn't seem to encounter the same problem. Do you know why that might be?
> 
> [1] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_reload.js#147

> This purpose of this test is to prove that a reload will fail if an extension's manifest has changed on disk to make it no longer compatible.

Right, so you want to ensure that if a developer is working on an extension and they change the manifest in such a way that it is no longer compatible, then press "reload", that they get an appropriate error.  If that doesn't work without this extra external "flush the cache" call, then developers will not get the desired experience.  I think kumar has touched the reload code most recently, you could check with him.

Without looking more closely, I'm also surprised about this failing only on Windows 7.  If you're still stuck I can try to take a look later today.
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review69986

> > This purpose of this test is to prove that a reload will fail if an extension's manifest has changed on disk to make it no longer compatible.
> 
> Right, so you want to ensure that if a developer is working on an extension and they change the manifest in such a way that it is no longer compatible, then press "reload", that they get an appropriate error.  If that doesn't work without this extra external "flush the cache" call, then developers will not get the desired experience.  I think kumar has touched the reload code most recently, you could check with him.
> 
> Without looking more closely, I'm also surprised about this failing only on Windows 7.  If you're still stuck I can try to take a look later today.

I'm sure you understand this better than I do, so I may be wrong, but I still don't agree. The flushing isn't being done in order to allow the reload to generate the message. Even without the flush the reload generates the message. It is only being done in order to prevent the error on Windows 7 that happens when we try to remove the file. Without that flush, the test passes everywhere else, proving that it works the way we expect. It's just an unfortunate situation that for some reason without that flush an error is thrown when trying to remove the file on Windows 7. I suppose that by adding the flush we have now created a test which will always call the flush, and therefore, if a regression is introduced that breaks this "unable to reload an invalid extension" only in the case where a flush is *not* called then we wouldn't catch that regression, but that seems like an extremely unlikely thing. Still, I do acknoweldge that it is possible. 

I'm not sure what to ask Kumar about. I don't think this issue has anything to do with the reload function at all. It has to do with the fact that we cannot remove a file from the temp folder without first flushing the JAR cache on Windows 7. I.e., it's entirely about the test.

I will try to spend some time figuring out why this particular test is an issue while the others in the file are not.
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review69986

> I'm sure you understand this better than I do, so I may be wrong, but I still don't agree. The flushing isn't being done in order to allow the reload to generate the message. Even without the flush the reload generates the message. It is only being done in order to prevent the error on Windows 7 that happens when we try to remove the file. Without that flush, the test passes everywhere else, proving that it works the way we expect. It's just an unfortunate situation that for some reason without that flush an error is thrown when trying to remove the file on Windows 7. I suppose that by adding the flush we have now created a test which will always call the flush, and therefore, if a regression is introduced that breaks this "unable to reload an invalid extension" only in the case where a flush is *not* called then we wouldn't catch that regression, but that seems like an extremely unlikely thing. Still, I do acknoweldge that it is possible. 
> 
> I'm not sure what to ask Kumar about. I don't think this issue has anything to do with the reload function at all. It has to do with the fact that we cannot remove a file from the temp folder without first flushing the JAR cache on Windows 7. I.e., it's entirely about the test.
> 
> I will try to spend some time figuring out why this particular test is an issue while the others in the file are not.

Ah okay, thanks for the clarification.  But this points to a flaw with the construction of the test.  When running in packed mode on Windows (or maybe just certain versions of windows?), a user (an extension developer to be precise) won't be able to remove a temporarily loaded xpi (because until the flush the jar cache can keep an open handle to it?).
I think that running that test only in the unpacked mode would be reasonable, or if we want to make sure we get coverage of this specific scenario on non-windows platforms, skip that test on windows.
Comment on attachment 8767739 [details]
Bug 1281884 - strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on",

https://reviewboard.mozilla.org/r/62146/#review69986

> Ah okay, thanks for the clarification.  But this points to a flaw with the construction of the test.  When running in packed mode on Windows (or maybe just certain versions of windows?), a user (an extension developer to be precise) won't be able to remove a temporarily loaded xpi (because until the flush the jar cache can keep an open handle to it?).
> I think that running that test only in the unpacked mode would be reasonable, or if we want to make sure we get coverage of this specific scenario on non-windows platforms, skip that test on windows.

That seems like a reasonable approach. I will have this test in particular not run on Windows, which should resolve the issue. I've submitted a new try for this.
After rebasing against m-c, picking up Kris' changes to AddonTestUtils, the test is now failing on both OS X and Windows, but in different places and for different reasons. I was able to get a Windows VM set up and a build working on that, so that makes debugging a bit easier, but I'm not sure what my next debugging steps should be. Here is what I am seeing now in terms of failures/exceptions:

On OS X, in packed mode, the re-write of the xpi file is generating an exception [1], but, based on the test's assertions seems to be successful. That is, the test asserts that the reload will reject after we change the manifest, and the reload does in fact reject, even though an exception is thrown when trying to rewrite the xpi file. I'm not sure what's going on here. Note that in this version of the code [2] we are removing the file before attempting to rewrite it, and the removal is not generating an exception.

On Windows, in unpacked mode, the rewrite of the extension folder is failing [3], and in packed mode the removal of the xpi file is failing [4].

Any suggestions for what to do next would be appreciated. 

[1] https://gist.github.com/bobsilverberg/892c39dd820269a510aab0e0d6e78c3c#file-test-failure-txt-L78
[2] https://reviewboard.mozilla.org/r/62146/diff/15#index_header
[3] https://gist.github.com/bobsilverberg/892c39dd820269a510aab0e0d6e78c3c#file-windows-test-failures-txt-L121
[4] https://gist.github.com/bobsilverberg/892c39dd820269a510aab0e0d6e78c3c#file-windows-test-failures-txt-L317
Flags: needinfo?(kmaglione+bmo)
The error on OS-X is happening because you're not waiting for the extension to finish starting up before you replace it, so the startup code fails when it tries to read the files that you've removed.

That will also cause locking problems on Windows, but in either case, you need to make sure you're not creating the ZipWriter with MODE_CREATE when you want to update rather than replace the zip file.


Also, please use `yield promiseWriteWebManifestForExtension` rather than using the synchronous form.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
(In reply to Kris Maglione [:kmag] from comment #67)
> The error on OS-X is happening because you're not waiting for the extension
> to finish starting up before you replace it, so the startup code fails when
> it tries to read the files that you've removed.

Thanks Kris. I fixed that and it solved the problem on OS X.

> 
> That will also cause locking problems on Windows, but in either case, you
> need to make sure you're not creating the ZipWriter with MODE_CREATE when
> you want to update rather than replace the zip file.
> 

I tried this on my Windows VM and I am still getting an error when it tries to write the file:

 0:09.32 LOG: Thread-2 ERROR Unexpected exception NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIZipWriter.close]
writeFilesToZip@resource://testing-common/AddonTestUtils.jsm:724:5
AddonTestUtils.promiseWriteFilesToZip<@resource://testing-common/AddonTestUtils.jsm:732:5
promiseWriteFilesToExtension@resource://testing-common/AddonTestUtils.jsm:774:12
writeWebManifestForExtension@c:/mozilla-source/objdir-frontend/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/head_addons.js:727:17
test_reload_to_invalid_version_fails@c:/mozilla-source/objdir-frontend/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_reload.js:171:14
_run_next_test@c:\mozilla-source\testing\xpcshell\head.js:1566:9
do_execute_soon/<.run@c:\mozilla-source\testing\xpcshell\head.js:713:9
_do_main@c:\mozilla-source\testing\xpcshell\head.js:210:5
_execute_test@c:\mozilla-source\testing\xpcshell\head.js:545:5

The docs [1] seem to suggest that if you specify MODE_CREATE but the file already exists that the flag will do nothing, so maybe that's not the solution we need. If I remove the file then it all works, but I cannot remove the file without first flushing the cache, which Andrew suggested isn't something we should do in the flow of the test. If I leave the file in place then I get the above error, with and without MODE_CREATE.

Do you have any further suggestions on this? 

> 
> Also, please use `yield promiseWriteWebManifestForExtension` rather than
> using the synchronous form.

I don't see `promiseWriteWebManifestForExtension` anywhere in the code base. `writeWebManifestForExtension` [2], which I am currently using uses a Promise internally, so is that acceptable? Are you suggesting that I need to create a `promiseWriteWebManifestForExtension` function somewhere and use that instead of `writeWebManifestForExtension`?

[1] PR_CREATE_FILE at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/PR_Open#Parameters
[2] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/head_addons.js#720
Flags: needinfo?(kmaglione+bmo)
(In reply to Bob Silverberg [:bsilverberg] from comment #68)
> > That will also cause locking problems on Windows, but in either case, you
> > need to make sure you're not creating the ZipWriter with MODE_CREATE when
> > you want to update rather than replace the zip file.
> > 
> 
> I tried this on my Windows VM and I am still getting an error when it tries
> to write the file:
> 
> Do you have any further suggestions on this? 

It might help to add MODE_TRUNCATE, but let's just skip this test on Windows
for now.

> > Also, please use `yield promiseWriteWebManifestForExtension` rather than
> > using the synchronous form.
> 
> I don't see `promiseWriteWebManifestForExtension` anywhere in the code base.
> `writeWebManifestForExtension` [2], which I am currently using uses a
> Promise internally, so is that acceptable? Are you suggesting that I need to
> create a `promiseWriteWebManifestForExtension` function somewhere and use
> that instead of `writeWebManifestForExtension`?

Just change `writeWebManifestForExtension` to return the promise rather than
waiting for it, rename it, and then create a `writeWebManifestForExtension`
stub that calls `awaitPromise` on its result.
Flags: needinfo?(kmaglione+bmo)
Latest try run looks good. Let's try landing this again.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a5571e6af4e9
strict_min_version and strict_max_version aren't respected in "Load Temporary Add-on". r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a5571e6af4e9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I was able to reproduce this initial issue on Firefox 50.0a2 (2016-09-08) under Windows 10 64-bit.

Verified fixed on Firefox 51.0a1 (2016-09-08/09) under Windows 10 64-bit and Ubuntu 16.04 32-bit. Incompatible webextensions are no longer installed via about:debugging.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: