Closed Bug 557710 Opened 10 years ago Closed 2 years ago

Check over behaviour w.r.t multiple install locations

Categories

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

defect

Tracking

()

RESOLVED INACTIVE

People

(Reporter: mossop, Assigned: rhelmer)

References

Details

(Whiteboard: [rewrite])

Attachments

(2 files)

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.
Assignee: dtownsend → nobody
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.
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
Blocks: 1273709
(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.
Blocks: 1295732
No longer blocks: 1295732
Blocks: 1204156
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.
Attachment #8785475 - Flags: review?(aswan) → 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

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.
Attachment #8786203 - Flags: review?(kmaglione+bmo) → review+
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
(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).
Depends on: 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.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.