1297028 - Disco Pane uninstall option is not disabled for globally-installed add-ons

Status: RESOLVED FIXED

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

Description

[Vasilica Tamas, QA [:vtamas]]

Attachment: unexpected error.png

[Note]
This is a follow-up bug for Bug 1294483

[Affected versions]:
Firefox 48.0.1 (20160817112116)
Firefox 49.0b5 (20160818050015)
Firefox 50.0a2 (2016-08-21)
Firefox 51.0a1 (2016-08-21)

[Affected platforms]:
Windows 10 32-bit
Windows 10 64-bit 

[Steps to reproduce]:
1.Install one of the 4 add-ons displayed in Disco Pane (e.g.Adblock Plus) via registry key.
2.Launch Firefox with a clean profile.
3.Check in Addons Manager -> Extensions that the add-on is enabled.
4.Go to Add-ons Manager -> Get Add-ons and uninstall the add-on.

[Expected results]:
Dico Pane uninstall option is not available for globally-installed add-ons.

[Actual results]:
“An unexpected error occurred during uninstallation.” error message is displayed.

[Additional notes]:  
- I am attaching a screenshot where I captured this issue.

Comment 1

[Andrew Swan [:aswan]]

I'm not sure if we want richer UX to communicate why the extension can't be uninstalled but I think the straightforward plumbing that we need to do here is to expose the permissions property from AddonWrapper through to content-visible Addon instances and then have AMO disable the uninstall button if permissions.PERM_CAN_UNINSTALL is not set.

Comment 2

[Andrew Swan [:aswan]]

Front-end (AMO) work to be tracked here: https://github.com/mozilla/addons-frontend/issues/959

Comment 3

[Andrew Swan [:aswan]]

The permissions field in the underlying object is a combination of these flags:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#3269-3280

The Addon interface has attributes like state and error that use these sorts of symbolic constants but  exposing those to content through webidl is difficult to do in a maintainable way, so we just mapped those fields back to strings (i.e., addon.state is a string like "STATE_DOWNLOADING" rather than a number like 1 that has to be mapped back to a corresponding symbol).  That won't really work though for a flags field since we can have 0, 1, or more flags set.  We could build a comma-separated string or use an array or something, but either of those feel like way overkill, I'm inclined to just add a boolean attribute to Addon called mayUninstall.

I'd rather not keep tweaking this interface, Stuart do you think the simple boolean attribute needed for this bug will be sufficient?  Or to put it another way, how likely do you think it is that we'll discover a need to see those other permissions from AMO content soon?

Comment 4

[Stuart Colville [:muffinresearch]]

I don't know the use-cases for when those other permissions might be relevant to amo. 

If we know that there's a possibility we will need to take them into account in the future then exposing more of them at the same time might be a good idea.

Otherwise just taking the approach of exposing the single mayUninstall (or maybe canUninstall) as a boolean probably makes the most sense and we can worry about the others as we need them.

Comment 5

[Markus Jaritz [:markus] (UX)]

Why would an add-on not be uninstallable?
Sounds like this would be something that a user has chosen very carefully - and therefor would know about.
Or like something done by a third-party in which case it should be uninstallable.
Did I miss a relevant case?
How often does this happen?

Comment 6

[Andrew Swan [:aswan]]

(In reply to Markus Jaritz [:maritz] (UX) from comment #5)
> Why would an add-on not be uninstallable?

If an add-on is installed in a system-wide location (such as the system-wide windows registry as described in this bug report or the linux "distribution" directories to give two examples), then the add-on cannot be uninstalled by an individual user.  This would typically be something set up by a system administrator.  The user may well have the capability to uninstall the add-on outside of the browser, but they cannot do it from within the browser.
I believe that in this case, users are allowed to disable the add-on, I think that making the switch do that in this scenario would be feasible but Stuart can say for sure.

Comment 7

[Stuart Colville [:muffinresearch]]

Fwiw - the UX questions around how to represent this this data have been filed over in mozilla/addons-frontend. [1]

[1 https://github.com/mozilla/addons-frontend/issues/959

Comment 8

[Andrew Swan [:aswan]]

Attachment: Bug 1297028 Expose Addon.canUninstall flag to content

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

Comment 9

[Andrew Swan [:aswan]]

bz: I need a dom peer to sign off on this, feel free to redirect if you like, but the webidl changes are pretty trivial.
rhelmer: the rest of it (everything that isn't webidl) is for you...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8784220 [details]
Bug 1297028 Expose Addon.canUninstall flag to content

https://reviewboard.mozilla.org/r/73750/#review71598

r=me on the webidl bit.

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 8784220 [details]
Bug 1297028 Expose Addon.canUninstall flag to content

https://reviewboard.mozilla.org/r/73750/#review71614

I'd probably make it `canUninstall` to match the AddonManager constant, but it's perfectly clear as-is so up to you.

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8784220 [details]
Bug 1297028 Expose Addon.canUninstall flag to content

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

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8784220 [details]
Bug 1297028 Expose Addon.canUninstall flag to content

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0db6683f22c5
Expose Addon.canUninstall flag to content r=bz,rhelmer

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0db6683f22c5

Comment 16

[Sylvestre Ledru [:Sylvestre]]

This is probably too late for 49 but we do want to uplift that to aurora/50?

Comment 17

[Andrew Swan [:aswan]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> This is probably too late for 49 but we do want to uplift that to aurora/50?

I doubt it, this is an obscure case and the corresponding AMO changes haven't yet been made.  I think letting this ride the trains is fine.  If Andy or Stuart feels differently they can jump in.