1260450 - about:performance allows users to disable system add-ons

Status: RESOLVED INCOMPLETE

(Toolkit :: Performance Monitoring, defect)

Description

[Dave Townsend [:mossop]]

We currently don't offer this option to users so we shouldn't include it in about:performance. It is also incorrectly offering the option to uninstall add-ons that can't be uninstalled.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

How can we find out whether an add-on is system and/or can't be uninstalled?

Comment 2

[Dave Townsend [:mossop]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #1)
> How can we find out whether an add-on is system and/or can't be uninstalled?

addon.isSystem will tell you if it is a system add-on but it might be worth using addon.hidden to simply hide any add-ons that we don't intend to appear in the main UI.
addon.permissions will include AddonManager.PERM_CAN_UNINSTALL if the add-on can be uninstalled, that applies to all add-ons.

Comment 3

[Adam Roach [:abr]]

This is a bit more critical than it may at first appear. While disabling system add-ons has a theoretical ability to break the browser (if we ever decide to ship an important component in this way), the big problem here is that users can disable add-ons with about:performance, and then be completely unable to re-enable them. Every other part of the system treats this as special and hides them.

I think it's useful to show them in about:performance, but we need to remove the disable/uninstall buttons.

dbaron -- I think this is your module. Can we get this prioritized?

Comment 4

[Adam Roach [:abr]]

Since this rolled out into release, I'm also tagging Laura for her awareness: the Go Faster team might need to take action to help users recover from the "user disabled a system addon and now can't re-enable it" state.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

While I'm a fan of about:performance, I haven't had anything to do with it; redirecting to Yoric and Mossop per https://hg.mozilla.org/mozilla-central/filelog/5f95858f8ddf21ea2271a12810332efd09eff138/toolkit/components/aboutperformance/content/aboutPerformance.xhtml

Comment 6

[Dave Townsend [:mossop]]

Yeah as long as we don't give any UI to enabling these we should disable them. Andy might be able to find some resources to fix this.

Comment 7

[Robert Helmer [:rhelmer]]

We should be able to tell from Telemetry how many users are in this state. I believe that we only send active add-ons to Telemetry, we can tell which of these are system add-ons, and we know which are supposed to be enabled for each Firefox release.

I've just tested and system add-ons are updated regardless of being enabled, but we don't have a reliable way to force-enable from remote. We could add the ability to do this, but I'm pretty sure we just don't ever want users to disable system add-ons.

I think we should have two fixes here:

1) AddonManager should ignore the userDisabled state for system addons
2) about:performance hide the "uninstall" and "disable" buttons if addon.isSystem

I'll file a bug for #1 and I have a patch for #2 which I'll push momentarily.

Comment 8

[Robert Helmer [:rhelmer]]

Attachment: Bug 1260450 - about:performance should not offer to disable or uninstall hidden or non-removable add-ons

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

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #7)
> 1) AddonManager should ignore the userDisabled state for system addons

Filed bug 1281077.

This way we don't have to worry about any callers accidentally exposing this, and it'll also fix the problem for existing users in this state.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #7)
> 2) about:performance hide the "uninstall" and "disable" buttons if
> addon.isSystem

Oh I had missed comment 2, I'll update the patch to do it this way and be less specific to system add-ons:

(In reply to Dave Townsend [:mossop] from comment #2)
> (In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from
> comment #1)
> > How can we find out whether an add-on is system and/or can't be uninstalled?
> 
> addon.isSystem will tell you if it is a system add-on but it might be worth
> using addon.hidden to simply hide any add-ons that we don't intend to appear
> in the main UI.
> addon.permissions will include AddonManager.PERM_CAN_UNINSTALL if the add-on
> can be uninstalled, that applies to all add-ons.

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 8763720 [details]
Bug 1260450 - about:performance should not offer to disable or uninstall hidden or non-removable add-ons

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

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8763720 [details]
Bug 1260450 - about:performance should not offer to disable or uninstall hidden or non-removable add-ons

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

Comment 13

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/59852/#review56840

Looks good, but a test would be useful.

::: toolkit/components/aboutperformance/content/aboutPerformance.js:763
(Diff revision 3)
> -
> -        let eltUninstall = document.createElement("button");
> -        eltUninstall.textContent = "Uninstall";
> -        eltSpan.appendChild(eltUninstall);
>  
>          let eltRestart = document.createElement("button");

Let's not create `eltRestart` if we don't need it.

::: toolkit/components/aboutperformance/content/aboutPerformance.js:772
(Diff revision 3)
>  
>          eltRestart.addEventListener("click", () => {
>            Services.startup.quit(Services.startup.eForceQuit | Services.startup.eRestart);
>          });
>          AddonManager.getAddonByID(delta.diff.addonId, addon => {
> +          if (!addon.hidden) {

I'd rather go for
```js
if (addon.hidden) {
   return; // Document why we should return here.
}
```

::: toolkit/components/aboutperformance/content/aboutPerformance.js:789
(Diff revision 3)
> -            eltUninstall.classList.add("hidden");
> +              eltUninstall.classList.add("hidden");
> -            eltRestart.classList.remove("hidden");
> +              eltRestart.classList.remove("hidden");
> -          });
> +            });
> +          }
> +
> +          if (addon.permissions & AddonManager.PERM_CAN_UNINSTALL) {

Here, too, some documentation is needed. In particular since I didn't find any documentation on `permissions` when I first wrote this code.

Comment 14

[Robert Helmer [:rhelmer]]

Comment on attachment 8763720 [details]
Bug 1260450 - about:performance should not offer to disable or uninstall hidden or non-removable add-ons

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

Comment 15

[Robert Helmer [:rhelmer]]

Thanks for the review! Made the requested changes.

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #13)
> https://reviewboard.mozilla.org/r/59852/#review56840
> 
> Looks good, but a test would be useful.

I'll add this next.

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 8763720 [details]
Bug 1260450 - about:performance should not offer to disable or uninstall hidden or non-removable add-ons

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

Comment 17

[Andy McKay]

A bit late to the story here, but why does about:performance need to offer these buttons at all. Can't it just bounce users over to about:addons and reduce the amount of code?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

(In reply to Andy McKay [:andym] from comment #17)
> A bit late to the story here, but why does about:performance need to offer
> these buttons at all. Can't it just bounce users over to about:addons and
> reduce the amount of code?

Doesn't look like a very nice UX.

Comment 19

[Andy McKay]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #18)
> Doesn't look like a very nice UX.

Since this isn't a complete user facing interface but rather a technical developer tool I figure that limiting the amount of code, maintenance and chance of errors (now and in the future) is more of a priority.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8763720 [details]
Bug 1260450 - about:performance should not offer to disable or uninstall hidden or non-removable add-ons

https://reviewboard.mozilla.org/r/59852/#review56924

lgtm, thanks

::: toolkit/components/aboutperformance/content/aboutPerformance.js:765
(Diff revision 5)
> -        eltDisable.textContent = "Disable";
> -        eltSpan.appendChild(eltDisable);
>  
> -        let eltUninstall = document.createElement("button");
> -        eltUninstall.textContent = "Uninstall";
> -        eltSpan.appendChild(eltUninstall);
> +        AddonManager.getAddonByID(delta.diff.addonId, addon => {
> +          if (addon.hidden) {
> +            // Hidden add-ons should not be disabled,

Nit: "disabled or uninstalled"

Comment 21

[Robert Helmer [:rhelmer]]

(In reply to Andy McKay [:andym] from comment #19)
> (In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from
> comment #18)
> > Doesn't look like a very nice UX.
> 
> Since this isn't a complete user facing interface but rather a technical
> developer tool I figure that limiting the amount of code, maintenance and
> chance of errors (now and in the future) is more of a priority.

I agree with Yoric that being able to directly disable/uninstall an add-on with a single click is nicer than having to click-through, especially if we are just going to send users to the about:addons Extensions tab.

However, we could click-through to the add-on "Show More Information" screen, which gives a description of the add-on and allows the same disable/enable buttons, so it's not a long way to go. Users may have forgotten what the add-on does so this might be useful information to have in the context of choosing whether to turn off an add-on.

Also, we are going to change the UX of about:addons to use an enable/disable slider soon rather than having the current UI, it will end up being more work for us later to have to remember to fix about:performance too so I think there's a very real maintenance cost to having the enable/disable functionality directly in about:performance.

Most urgently - since there's a concern about users accidentally permanently disabling system add-ons right now via the current about:performance UI, why don't I do this:

1) finish up current patch, get it landed and on the trains (maybe uplift too, it's trivial)
2) work up a patch to forward this to the appropriate place in add-ons manager

#2 is less urgent, and that'll give us time to think about this and get the UX right, while not delaying the more primary concern of this bug. That bit can just ride the trains out.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

I'm very fine with someone taking over development of about:performance (especially since I'm now in Connected Devices). Personally, I'd say that the next priority would be bug 1199987.

Comment 23

[Andy McKay]

(In reply to Robert Helmer [:rhelmer] from comment #21)
Sounds good to me, sorry don't want to derail the current plans of fixing a bug - just long term maintenance is a concern.

Comment 24

[Robert Helmer [:rhelmer]]

(In reply to Andy McKay [:andym] from comment #23)
> (In reply to Robert Helmer [:rhelmer] from comment #21)
> Sounds good to me, sorry don't want to derail the current plans of fixing a
> bug - just long term maintenance is a concern.

No worries - I filed bug 1281261 so we can continue the discussion there, and I'll get the current bug finished up.

Comment 25

[Robert Helmer [:rhelmer]]

OK the only thing holding us up here is testing - right now the about:performance mochitest doesn't install test add-ons, and I want to do that rather than relying on whichever ones we happen to be shipping with Firefox.

Comment 26

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #25)
> OK the only thing holding us up here is testing - right now the
> about:performance mochitest doesn't install test add-ons, and I want to do
> that rather than relying on whichever ones we happen to be shipping with
> Firefox.

Setting up tests is a bit more involved than I thought at first - system add-ons need to be present at startup, and there isn't a great way to do that in mochitests AFAIK (but will look into this - just copying a file before the test begins would be enoug).

For xpcshell tests we have a way to restart AddonManager but it's a bit involved to port over just for this. There may be a simpler approach that I am missing, but right now looks like we need to port over a bunch of test code just to test this.

It might be a better use of time to focus instead on bug 1281077 which will just remove the ability to disable system add-ons (and possibly uplift that), then we can spend more time figuring out exactly what we want to do with about:performance UX around disabling add-ons.

Bug 1281261 might be a better option here, about:addons already has all the testing that we care about.

Comment 27

[Robert Helmer [:rhelmer]]

Bug 1281077 turned out to be pretty straightforward - assuming that sticks, about:performance will no longer be able to disable system add-ons (or any add-ons that we consider "hidden"), since there's no UI to re-enable these.

I am pretty sure a regular Firefox update will re-enable all system add-ons since that causes a reset (meaning any out-of-band updates for the previous build are removed, and the built-in versions in the new app are used instead), but will confirm this. If that's the case, we shouldn't need to do any special remediation.

The about:performance UI as of right now still offers to disable hidden/system add-ons though, which is confusing/misleading (but not harmful), so we should finish up attachment 8763720 [details] or do bug 1281261 instead to finish this bug out.

Comment 28

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #27)
> I am pretty sure a regular Firefox update will re-enable all system add-ons
> since that causes a reset (meaning any out-of-band updates for the previous
> build are removed, and the built-in versions in the new app are used
> instead), but will confirm this. If that's the case, we shouldn't need to do
> any special remediation.

OK tested this locally, and it's not quite this simple right now unfortunately. We should probably just re-enable any disabled system add-ons automatically on app update, I'll file a bug for that.

I tested that updates get installed+enabled regardless of the state of the built-in add-ons, but I suspect they will be disabled again on the next Firefox update, when it falls back to the default set (I haven't set up enough to test this case but that's my assumption right now).

Comment 29

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #28)
> (In reply to Robert Helmer [:rhelmer] from comment #27)
> > I am pretty sure a regular Firefox update will re-enable all system add-ons
> > since that causes a reset (meaning any out-of-band updates for the previous
> > build are removed, and the built-in versions in the new app are used
> > instead), but will confirm this. If that's the case, we shouldn't need to do
> > any special remediation.
> 
> OK tested this locally, and it's not quite this simple right now
> unfortunately. We should probably just re-enable any disabled system add-ons
> automatically on app update, I'll file a bug for that.

Bug 1283963

Comment 30

[Robert Helmer [:rhelmer]]

This was removed as part of bug 1309946 - addons no longer appear in about:performance

What was changed in this bug is that system add-ons cannot be user disabled by any means, so if any UI accidentally exposes it then it will have no effect.