Dave Townsend [:mossop] Reporter
	Description • 14 years ago

There are a couple of cases where we need better testing of having the same add-on in multiple install locations and ensuring that the relevant add-on is visible when ones in different locations are enabled/disabled/installed/uninstalled. In particular restartless add-ons don't reveal hidden add-ons when uninstalled.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 1 • 11 years ago

Sachin discovered one case where this doesn't appear to be working correctly: Installing a newer version of an addon that's already in a locked location. If the newer version is restartless, the upgrade notification doesn't seem to show in about:addons.

	Robert Helmer [:rhelmer]
	Comment 2 • 8 years ago

We're using this now for temporary add-ons and considering it for system add-ons, I'll add some better test coverage.

	Robert Helmer [:rhelmer]
	Comment 3 • 8 years ago

(In reply to Dave Townsend [:mossop] from comment #0)
> There are a couple of cases where we need better testing of having the same
> add-on in multiple install locations and ensuring that the relevant add-on
> is visible when ones in different locations are
> enabled/disabled/installed/uninstalled. In particular restartless add-ons
> don't reveal hidden add-ons when uninstalled.

From manually testing just now this still appears to be the case - however if I refresh about:addons then the add-on that was hidden re-appears.

	Robert Helmer [:rhelmer]
	Comment 4 • 8 years ago

(In reply to Blair McBride [:Unfocused] (UNAVAILABLE) from comment #1)
> Sachin discovered one case where this doesn't appear to be working
> correctly: Installing a newer version of an addon that's already in a locked
> location. If the newer version is restartless, the upgrade notification
> doesn't seem to show in about:addons.

This still seems to be happening - if I have an add-on in a locked location, then install an add-on over that has a valid updates property, this is logged:

Update manifest for ${addonId} did not contain an updates property

Presumably it's using the updates property of the now-hidden add-on in the locked location.

This feels like an edge case really, but I don't see why we shouldn't support it. For some locations (like the temporary install location) we explicitly disable the ability to update, but for the regular profile location it seems like we could allow that.

	Robert Helmer [:rhelmer]
	Comment 5 • 8 years ago

(In reply to Robert Helmer [:rhelmer] from comment #3)
> (In reply to Dave Townsend [:mossop] from comment #0)
> > There are a couple of cases where we need better testing of having the same
> > add-on in multiple install locations and ensuring that the relevant add-on
> > is visible when ones in different locations are
> > enabled/disabled/installed/uninstalled. In particular restartless add-ons
> > don't reveal hidden add-ons when uninstalled.
> 
> From manually testing just now this still appears to be the case - however
> if I refresh about:addons then the add-on that was hidden re-appears.

Ah, the reason for this is that the uninstall of the higher-precedence add-on is pending, so the user can choose to undo. This means install and startup on the hidden add-on is pending too.

Since this only happens in the UI-driven case, I don't think we want to make undo more complicated so the current behavior is OK.

I'll write a test to make sure it does the right thing in all the enabled/disabled/installed/uninstalled cases when AddonManager is used directly from an xpcshell test.

	Robert Helmer [:rhelmer]
	Comment 6 • 8 years ago

I have a few xpcshell tests that shows the following all work:

1) install a default system add-on
2) load an upgrade over this
3) load a regular extension over this
4) regular extension can be disabled/enabled, does not reveal underlying
5) upgrading the regular extension works
6) removing each reveals the hidden extensions in the proper order

#5 contradicts what I saw in manual testing for comment 4 so it's possible that I made a mistake setting that up, I'll try it again.

The one area where we do have problems is in temporary add-ons - the TemporaryInstallLocation's uninstallAddon method doesn't do anything, while the MutableDirectoryLocation does do some work around revealing addons in the right order so I suspect we need to do the same there.

I am going to file a separate bug for the temporary addon issue.

	Robert Helmer [:rhelmer]
	Comment 8 • 8 years ago

(In reply to Robert Helmer [:rhelmer] from comment #6)
> I am going to file a separate bug for the temporary addon issue.

Bug 1298545.

	Robert Helmer [:rhelmer]
	Comment 9 • 8 years ago
mozreview-review

Comment on attachment 8785475 [details]
Bug 557710 - add tests for behavior when the same restartless add-on is present in multiple install locations

https://reviewboard.mozilla.org/r/74658/#review72528

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:663
(Diff revision 1)
>   * @param   aDir
>   *          The install directory to add the extension to
>   * @param   aId
>   *          An optional string to override the default installation aId
> - * @param   aExtraFile
> - *          An optional dummy file to create in the extension
> + * @param   {Object} aExtraFiles
> + *          An optional object containing file names and their contents.

BTW I filed bug 1298467 to make `writeInstallRDFToDir` consist with this too - it requires modifying a bunch of tests.

I will probably move more test code out of `head_addons.js` and into `AddonTestUtils` but this change is enough to make the current test easier.

	Andrew Swan [:aswan]
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8785475 [details]
Bug 557710 - add tests for behavior when the same restartless add-on is present in multiple install locations

https://reviewboard.mozilla.org/r/74658/#review72550

This looks good, but can you also include temporary addons since they have a pretty different install/uninstall code path?
The stuff with system upgrades will also get more compelling when  install/uninstall are restartless, right now the AOM just needs to build up the right state at startup rather than making the live transition when an update comes or goes.

::: toolkit/mozapps/extensions/test/xpcshell/data/test_overload.json:6
(Diff revision 1)
> +{
> +  "addons": {
> +    "test_overload@tests.mozilla.org": {
> +      "updates": [
> +        { "version": "3.0",
> +          "update_link": "http://localhost:%PORT%/addons/test_overload_v3.xpi"

what substitutes %PORT% here?  or, is this file actually ever referenced?  it looks like the upgrade is manually put into the upgrades directory...

::: toolkit/mozapps/extensions/test/xpcshell/test_overloading.js:12
(Diff revision 1)
> +const PREF_SYSTEM_ADDON_SET = "extensions.systemAddonSet";
> +const PREF_UPDATE_SECURITY = "extensions.checkUpdateSecurity";
> +
> +// The test extension uses an insecure update url.
> +Services.prefs.setBoolPref(PREF_UPDATE_SECURITY, false);
> +// The test uses unsigned add-ons

But this is xpcshell, you shouldn't need this...
If the test does rely on this, it will break when this reaches beta where signing cannot be disabled.

	Andrew Swan [:aswan]
	Comment 11 • 8 years ago
mozreview-review-reply

Comment on attachment 8785475 [details]
Bug 557710 - add tests for behavior when the same restartless add-on is present in multiple install locations

https://reviewboard.mozilla.org/r/74658/#review72550

Oh ha, I just saw bug 1298545 after writing this.  I'm assuming that's why temporary addons aren't included here?

	Robert Helmer [:rhelmer]
	Comment 12 • 8 years ago
mozreview-review-reply

Comment on attachment 8785475 [details]
Bug 557710 - add tests for behavior when the same restartless add-on is present in multiple install locations

https://reviewboard.mozilla.org/r/74658/#review72550

Yes, it seems involved enough to be its own bug.

> what substitutes %PORT% here?  or, is this file actually ever referenced?  it looks like the upgrade is manually put into the upgrades directory...

%PORT% is substituted in `head_addons.js`: https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/toolkit/mozapps/extensions/test/xpcshell/head_addons.js#1124

The JSON update file is referenced from the install manifest in one of the test (`updateURL`)

> But this is xpcshell, you shouldn't need this...
> If the test does rely on this, it will break when this reaches beta where signing cannot be disabled.

Hm good point, I'll look into this.

	Robert Helmer [:rhelmer]
	Comment 13 • 8 years ago
mozreview-review-reply

Comment on attachment 8785475 [details]
Bug 557710 - add tests for behavior when the same restartless add-on is present in multiple install locations

https://reviewboard.mozilla.org/r/74658/#review72550

> Hm good point, I'll look into this.

Ah this is the system add-on upgrade specifically, let me see what can be done here.

	Kris Maglione [:kmag]
	Comment 16 • 8 years ago
mozreview-review

Comment on attachment 8786203 [details]
Bug 557710 - add support for system add-ons to fake testing cert

https://reviewboard.mozilla.org/r/75178/#review73302

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:454
(Diff revision 1)
>  
>            let id = yield this.getIDFromManifest(manifestURI);
>  
>            let fakeCert = {commonName: id};
> +          if (this.isSystemAddon(file)) {
> +            fakeCert["organizationalUnit"] = "Mozilla Components";

I wonder if we should include an OU either way? We should probably just use "Preliminary" by default.

	Pulsebot
	Comment 18 • 8 years ago

Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/809e880145b1
add tests for behavior when the same restartless add-on is present in multiple install locations r=aswan
https://hg.mozilla.org/integration/autoland/rev/63812e8894ce
add support for system add-ons to fake testing cert r=kmag

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 8 years ago

Backed out for xpcshell failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=2804297&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=2804351&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/8542925e7c36

	Robert Helmer [:rhelmer]
	Comment 20 • 8 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> Backed out for xpcshell failures.
> https://treeherder.mozilla.org/logviewer.html#?job_id=2804297&repo=autoland
> https://treeherder.mozilla.org/logviewer.html#?job_id=2804351&repo=autoland
> 
> https://hg.mozilla.org/integration/autoland/rev/8542925e7c36

Oh I misread these as being unrelated intermittent failures, but they are not - I'll get the tests fixed up.

	Robert Helmer [:rhelmer]
	Comment 21 • 8 years ago

(In reply to Robert Helmer [:rhelmer] from comment #20)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> > Backed out for xpcshell failures.
> > https://treeherder.mozilla.org/logviewer.html#?job_id=2804297&repo=autoland
> > https://treeherder.mozilla.org/logviewer.html#?job_id=2804351&repo=autoland
> > 
> > https://hg.mozilla.org/integration/autoland/rev/8542925e7c36
> 
> Oh I misread these as being unrelated intermittent failures, but they are
> not - I'll get the tests fixed up.

The real problem is that I addressed comment 16 after doing a try run, which is why this wasn't caught before autoland ("Preliminary" in the OU causes the signed state to be AddonManager.SIGNEDSTATE_PRELIMINARY which breaks some current tests).

I think "Preliminary" in the OU of the mocked cert is changing the meaning of these tests in a way we don't want. We set to OU to be something like "XPCShell Tests" instead perhaps.

	Robert Helmer [:rhelmer]
	Comment 25 • 8 years ago

Ah, `writeInstallRDFToXPI` is used indirectly in more places than I realized at first - I am going to refactor that first (bug 1298467).

	BMO Automation
	Comment 26 • 6 years ago

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

	Pulsebot
	Comment 28 • 4 years ago

Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/798aaee84ad1
test behavior with addons installed to multiple locations r=rhelmer

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 31 • 4 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7902ce3bdd4d6911f95f4642481cda691062ee1e

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 32 • 4 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a143d9079ecedca2531c715b9288da0668c1bbc

	Pulsebot
	Comment 33 • 4 years ago

Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd703950a5b1
fix precedence handling with addons installed to multiple locations r=rhelmer,aswan