1281077 - system add-ons should not be disabled by users

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Helmer [:rhelmer]]

Add-ons have a userDisabled state, however system add-ons should not be disabled by users.

AddonManager should ignore this state when addon.isSystem is true.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: Bug 1281077 - do not allow hidden add-ons to be user disabled.

A few reviewer notes about this:

I looked into using the PERM_CAN_DISABLE permission instead, but that seems mostly advisory at this point. I could do this instead if you'd prefer.

This throws an exception to be consistent with what we do if someone attempts to disable the default theme.

Generally AddonManager seems to prefer to log and return a value for the client to
check. I made sure that about:performance doesn't break at least (about:perf is
the driving reason behind this change, see bug 1260450 for context).

Review commit: https://reviewboard.mozilla.org/r/61428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61428/

Comment 2

[Robert Helmer [:rhelmer]]

Comment on attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

I see a mistake in the test, let me fix that before you bother to review.

Comment 3

[Robert Helmer [:rhelmer]]

Comment on attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

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

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

https://reviewboard.mozilla.org/r/61428/#review58338

The change itself looks good and I think you have the essential tests.  I'm not sure about the purpose of some of the other tests (see line comments below) but I think you cover the important parts.

::: toolkit/mozapps/extensions/test/xpcshell/test_nodisable_hidden.js:13
(Diff revision 2)
> +const NORMAL_ID = "[e-mail]";
> +const NORMAL_ID2 = "[e-mail]";

did you mean for these to be the same?

::: toolkit/mozapps/extensions/test/xpcshell/test_nodisable_hidden.js:20
(Diff revision 2)
> +const SYSTEM_ID = "[e-mail]";
> +const SYSTEM_ID2 = "[e-mail]";
> +
> +createAppInfo("[e-mail]", "XPCShell", "1", "42");
> +
> +function* createNormalAddon(id, version, targetApp) {

nitpick, the only real difference between these two functions is the directory they write to, you could just have a single createAddon() that also takes dir as a parameter.

::: toolkit/mozapps/extensions/test/xpcshell/test_nodisable_hidden.js:83
(Diff revision 2)
> +  // install a new, normal add-on that is incompatible to ensure that appDisabled
> +  // state works correctly.
> +  let incompatAddon = yield promiseAddonByID(NORMAL_ID2);
> +  do_check_neq(incompatAddon, null);
> +  do_check_eq(incompatAddon.version, "2.0");
> +  do_check_eq(incompatAddon.name, "Test disabling hidden add-ons, non-hidden add-on case.");
> +  do_check_false(incompatAddon.isCompatible);
> +  do_check_true(incompatAddon.appDisabled);
> +  do_check_false(incompatAddon.userDisabled);
> +  do_check_false(incompatAddon.isActive);
> +  do_check_eq(incompatAddon.type, "extension");

I'm not sure what's being tested here?  You're upgrading the userDisabled add-on to one that is incompatible and checking that that reset userDisabled to false but did get marked with appDisabled true?  Seems like a reasonable test, but I'm not sure what it has to do with system addons?

::: toolkit/mozapps/extensions/test/xpcshell/test_nodisable_hidden.js:136
(Diff revision 2)
> +  // install a new system add-on that is incompatible to ensure that appDisabled
> +  // state works correctly.
> +  let incompatAddon = yield promiseAddonByID(SYSTEM_ID2);
> +  do_check_neq(incompatAddon, null);
> +  do_check_eq(incompatAddon.version, "2.0");
> +  do_check_eq(incompatAddon.name, "Test disabling hidden add-ons, hidden system add-on case.");
> +  do_check_false(incompatAddon.isCompatible);
> +  do_check_true(incompatAddon.appDisabled);
> +  do_check_false(incompatAddon.userDisabled);
> +  do_check_false(incompatAddon.isActive);
> +  do_check_eq(incompatAddon.type, "extension");

I don't understand how this test is related to the changes in this bug?

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

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

Comment 6

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/61428/#review58338

> I'm not sure what's being tested here?  You're upgrading the userDisabled add-on to one that is incompatible and checking that that reset userDisabled to false but did get marked with appDisabled true?  Seems like a reasonable test, but I'm not sure what it has to do with system addons?

You're right, this should just be removed.

This (and the one below) were intended to show that `appDisabled` still works, but we have other tests that cover that.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62e6d24988ec
do not allow hidden add-ons to be user disabled. r=aswan

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/62e6d24988ec