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.

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.

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

(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.

(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.

(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.

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.

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

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.

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.

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?

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.

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.

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.

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

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

(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.

(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.

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

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.

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

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