571049 - Add-ons should be able to do stuff on uninstall, even if they're disabled

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, enhancement)

Description

[Drew Willcoxon :adw]

If an add-on is uninstalled while it's disabled, it's not notified of uninstall.  If it needs to do some special cleanup on uninstall, like removing persistent storage such as a file, it won't be able if it's first disabled.  Add-ons should always be "notified" on uninstall so they can react, for some definition of "notified".

A couple of ideas:

1. Briefly enable the add-on and send it a message.

2. Let the add-on provide a persistent callback that's called on
   its behalf on uninstall.

Probably this should be a toolkit bug, but filing in Jetpack for now, since it spun out of Jetpack bug 549324.  (See comment 15 and later.)

Comment 1

[Atul Varma [:atul]]

I think another idea mossop had was to have addons be able to specify a series of pre-defined actions that they wanted done at install and uninstall, e.g. "make sure directory PROFILEDIR/foo exists when i'm installed, and make sure it's deleted when i'm uninstalled", which the addon manager could take care of performing. i think this is what MSIs do and it seems kind of nice in the sense that the platform takes care of making everything transactional so the addon doesn't have to. (in fact, I think this may have been some of the motivation for the current install.rdf format--I heard a lot of the things now done automatically by the extension manager used to just be a script left to addon authors back in the olden days.)

Mossop, please educate us!

Comment 2

[Dave Townsend [:mossop]]

This is only true for 3.6 right? Restartless add-ons should get their uninstall method called regardless of whether they are enabled or not on trunk (ignoring some edge cases).

We had various ideas of how to support this ranging from add-ons providing an uninstall script to letting them specify individual files and prefs to be removed on uninstall. We never really reached a good consensus on that though. Now that the restartless forms support it naturally I'm less inclined to put more effort into it.

Comment 3

[Atul Varma [:atul]]

Oh, I didn't realize that about the new addon manager. What are the semantics of that? Will the new addon manager re-evaluate bootstrap.js (e.g. if the addon was disabled for the entire duration of the process so far) and then call its uninstall function? this will mean interesting things for jetpack's bootstrap.js--i think it would basically result in nothing happening, since jetpack would basically say "oh, my addon's not active right now so there's nothing to shut down.". In which case we just need to decide whether we want to briefly start-up the jetpack platform just so all modules can do anything they need to for uninstallation.

If any of that makes sense... hopefully I am not just confusing everyone.

Comment 4

[Dave Townsend [:mossop]]

Yes, bootstrap.js would get loaded at that point and uninstall called and nothing else.

Comment 5

[Drew Willcoxon :adw]

Oh sorry, I noticed that shutdown() was not being called on uninstall-after-disable, but I didn't realize until I tried just now that uninstall() gets called.  That makes sense.  It also makes sense not to backport support for this to 1.9.2.

So this really is just about providing support on the Jetpack side.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 8

[Alexandre Poirot [:ochameau]]

A related discussion took place in bug 627432 and 620541.
Here is a summary:
Addon manager implements undoable uninstall.
So when a user click on uninstall, the addon is immediatly disabled but not uninstalled. Bug 620541 is about being able to differenciate this particular disable event from the regular disable action. But it won't help as we should just disable the addon at this time. We have to wait for the uninstall event  that comes when the manager tab is closed. Then only when we get this final event, we can remove files, preferences and all these kind of stuffs. Bug 627432 is about being able to remove simple storage files. 

And so we get back to this particular bug where we need to find the proper way to handle uninstall. This uninstall action that comes when all CommonJS modules have already received "unload" event.

Comment 9

[Alexandre Poirot [:ochameau]]

Now, let see what kind of options we have to handle uninstall nicely:

1/ Provide to addons a way to give a callback for the late uninstall event, that will be called *after unload event*. (comment 0 from drew)
There is two issues with this approach:
 - addons authors have to be carefull between unload and uninstall events and ensure that uninstall will still work after unload cleanup,
 - as we store a reference to this callback, we will retain in memory most of these modules until uninstall event. AND, in order to delete these reference when the user only disable the addon, we have to either:
      * get an event from the addon manager when it is closed,
      * have 620541 being fixed.

2/ Provide some platform/addon-manager APIs to the addon that will automatically remove files/preferences on uninstall. (comment 1 from Atul)
(We can provide such API ourself, as jetpack modules if we go with the 1/ solution.)

3/ any other ideas?

Dave: Is there such APIs ? (see 2/ point) Is there an event that we can watch for the addon-manager tab close ?

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> Dave: Is there such APIs ? (see 2/ point) Is there an event that we can
> watch for the addon-manager tab close ?

No, we've talked about things along these lines in the past but never implemented anything. You can use a regular DOM unload listener to watch for the add-on manager closing, but I don't know why you'd want to do that. The uninstall method call is the trigger that you should be using, nothing else.

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to comment #10)
> No, we've talked about things along these lines in the past but never
> implemented anything. You can use a regular DOM unload listener to watch for
> the add-on manager closing, but I don't know why you'd want to do that. The
> uninstall method call is the trigger that you should be using, nothing else.

I want to listen to such event in case you are not able to fix bug 620541. 
This is for the Disable case. As I can't differenciate the Disable event for "only disabling" or "uninstall", I need to keep uninstall callback references until I get the uninstall event, or in our disable case, the closing of the addon manager.
But if you are able to fix bug 620541, I'd be able to delete all these references on Disable event.
Does that make sense?

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> (In reply to comment #10)
> > No, we've talked about things along these lines in the past but never
> > implemented anything. You can use a regular DOM unload listener to watch for
> > the add-on manager closing, but I don't know why you'd want to do that. The
> > uninstall method call is the trigger that you should be using, nothing else.
> 
> I want to listen to such event in case you are not able to fix bug 620541.

It's unlikely that bug will get fixed in the near future. I'm considering wontfixing it as it isn't really a problem. Changing that behaviour won't help you as far as I can tell, just shift your problem to a different use case.

> This is for the Disable case. As I can't differenciate the Disable event for
> "only disabling" or "uninstall", I need to keep uninstall callback
> references until I get the uninstall event, or in our disable case, the
> closing of the addon manager.

You shouldn't need to differentiate between the two cases because you should behave the same way for both.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #12)
> You shouldn't need to differentiate between the two cases because you should
> behave the same way for both.

We've been thinking differently about these two actions for one particular case: stored addon data.

Our thinking has been that "disable" means the user doesn't want to get rid of the addon permanently and intends to reenable it later (or at least isn't sure whether or not to reenable it later), whereas "uninstall" means the user wants to get rid of the addon and doesn't intend to reinstall it later.

For the disable action, we have been planning to retain stored data, so a user who reenables an addon gets it back in the state the addon was in when it was disabled.

For the uninstall action, on the other hand, we have been planning to remove stored data, on the principle that once a user uninstalls an addon, we shouldn't leave its stored data lying around clogging up the user's system.

This is akin to temporarily disabling one's account on Facebook versus permanently deleting one's account.  In the former case, one can reenable the account, no harm no foul, all data is retained.  In the latter case, all data is deleted.

I'm certainly open to being talked into different behavior.  Perhaps our assumptions are incorrect!  I'm just laying out the rationale that underpins them, which has made sense to us since we worked it out and is the reason we have these (two?) bugs about differentiating between the two actions.

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> (In reply to comment #12)
> > You shouldn't need to differentiate between the two cases because you should
> > behave the same way for both.
> 
> We've been thinking differently about these two actions for one particular
> case: stored addon data.
> 
> Our thinking has been that "disable" means the user doesn't want to get rid
> of the addon permanently and intends to reenable it later (or at least isn't
> sure whether or not to reenable it later), whereas "uninstall" means the
> user wants to get rid of the addon and doesn't intend to reinstall it later.
> 
> For the disable action, we have been planning to retain stored data, so a
> user who reenables an addon gets it back in the state the addon was in when
> it was disabled.

But I'm assuming that you want the following two scenarios to have the same end result:

1. User clicks remove in the add-ons manager and (assuming they don't undo it) the add-on gets uninstalled
2. User clicks disable in the add-ons manager and sometime later (maybe after restarting Firefox) the user clicks remove and the add-on gets uninstalled.

Removing stored data when an add-on is removed I can understand, we have had requests that we support that for some time. I'm pretty sure though that you want to remove that data in both those cases listed above, to behave differently would seem quite bizarre to me and probably pretty surprising to the user.

The point that I've been trying to explain is that right now the calls we make to the SDK when the user uninstalls an enabled add-on through the UI (case 1) are identical to the calls we make to the SDK for case 2. If we were to do the work necessary to fix bug 620541 in Firefox it would help to remove stored data in case 1 only. If on the other hand we do the work necessary in the SDK to support removing stored data in case 2 then it should also automatically work for case 1.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #14)
> But I'm assuming that you want the following two scenarios to have the same
> end result:
> 
> 1. User clicks remove in the add-ons manager and (assuming they don't undo
> it) the add-on gets uninstalled
> 2. User clicks disable in the add-ons manager and sometime later (maybe
> after restarting Firefox) the user clicks remove and the add-on gets
> uninstalled.

Yes, absolutely.


> The point that I've been trying to explain is that right now the calls we
> make to the SDK when the user uninstalls an enabled add-on through the UI
> (case 1) are identical to the calls we make to the SDK for case 2. If we
> were to do the work necessary to fix bug 620541 in Firefox it would help to
> remove stored data in case 1 only. If on the other hand we do the work
> necessary in the SDK to support removing stored data in case 2 then it
> should also automatically work for case 1.

Ah, sorry, I misunderstood.  Indeed, a fix for bug 620541 doesn't sound adequate, and we should address the issue of removing data on uninstall by fixing this bug instead.

Comment 16

[Alexandre Poirot [:ochameau]]

(In reply to comment #12)
> It's unlikely that bug will get fixed in the near future. I'm considering
> wontfixing it as it isn't really a problem. Changing that behaviour won't
> help you as far as I can tell, just shift your problem to a different use
> case.

You are right, in comment 9, I was thinking only about uninstall that comes after a startup. So it doesn't take care of uninstall that comes first. And so my proposal doesn't make sense ...

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: 1st option: have a specific "uninstall" module

Here is a first concrete proposal of many:
Have an "uninstall" module, like main module. You may specify a custom name in your package.json file, or it will search for an "uninstall.js" file in your lib directory.
Then, this module will be loaded on uninstall event, after regular module had been unloaded during shutdown event.

To be clear, here is how AddonManager events are dispatched:
- startup: when an addon is enable (on addon install, firefox startup or addon re-enabled)
- shutdown: when an addon is disabled or uninstalled or on firefox shutdown
- uninstall: when about:addons is closed and the addon had been asked to uninstall, comes after shutdown

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: 2nd option: call main.uninstall method

My second proposal would be to load main module again during uninstall event, and call "uninstall" method instead of "main".
I'm not confortable with this because developers may easily forget to use main method and instead just write instruction in top context.
And we may load various unecessary module during uninstall (that may do unwanted "things").

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: 3rd option: call main method with "uninstall" reason

During uninstall event, we call main method on main module, with options.loadReason being equal to "uninstall".
exports.main = function (options) {
  options.loadReason = "uninstall"
}
This proposal is quite similar to the previous one, developers will have to be extremelly carefull of these changes.

Comment 20

[Alexandre Poirot [:ochameau]]

Ooops, I mixed two previous patches, so title and comment doesn't match the attachment. So attachment 543768 [details] [diff] [review] is related to comment 19 and attachment 543770 [details] [diff] [review] is for comment 18 :/

Comment 21

[Alexandre Poirot [:ochameau]]

Attachment: 4th option: Offer a way to register uninstall scripts/modules dynamically

Finally, here is my favorite option. Definitively the most complex one to implement, but over all previous ones, it offer a way for *any* modules to have some uninstall instructions.
For example, how would we handle uninstall in simple-storage with previous options? We would need final developpers to call a method on simple-storage on uninstall, or we would have to implmenent additional way of handling api-utils modules uninstall.

In this proposal, we have a new api-utils/uninstall module, that we may use like this:
require("uninstall").registerScript("console.log('do thing during uninstall')");
require("uninstall").registerModule("my-custom-uninstall-module");
It is close to content script as we can either specify a script string or a module path.
The main benefit of this module is that any module can register something to do during uninstall, so it would be really easy to clean simple storage and any other internal API during uninstall event.
But this won't work with static linker as it won't detect custom modules used on uninstall and even less script string that contains requires!

These 4 options do not intend to be final implementation, but only primilinary work in order to help choosing one way. Any other idea or improvements is welcomed.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Marking anything that potentially looks like a feature request as "enhancement", sorry for the bugspam. :)

Comment 23

[Benjamin Smedberg]

When we specced out addon uninstall actions, I was very adamant that disabled addons should *not* be allowed to do any scripted actions when they are uninstalled. This was to give the user control and make sure that addons which were disabled for "security" reasons could not start running code.

I'm opposed to the basic premise of this bug, unless I misunderstand it.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Re-prioritizing all 1.1-targeted feature requests to 1.2.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 27

[Louis-Rémi BABE]

I think addon authors should also have the ability to take actions when the addon is simply disabled.
For example: my addon modifies the url used by the WifiGeoProvider service. As soon as it is disabled, this pref should be reset to google's API

To answer bsmedberg concerns: if addons want to misbehave, they don't have to wait for the (potential) uninstall hooks.

Comment 28

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Louis-Rémi BABE from comment #27)
> To answer bsmedberg concerns: if addons want to misbehave, they don't have
> to wait for the (potential) uninstall hooks.

If I create a malicious add-on and then it gets blocklisted (disabled), then it would certainly be beneficial to me for this kind of hook to exist. If the user decides to uninstall the add-on after it is disabled, I could use that hook to continue doing malicious stuff, or even install a new variation of the same code with a different id.

It'd be nice if this hook could somehow limit what add-ons can do so that they can only clean up after themselves. However, that is not currently possible.

Comment 29

[soufian.j]

Malicious addons should and will not appear on the AMO site. So you don't have to expect a malicious addon to be exposed on a wide scale to the public, and then causing harm perpetually after it is disabled/blocklisted.

I think the risk is slim and the benefit of keeping control over your addon is bigger.

Comment 30

[Dave Townsend [:mossop]]

(In reply to Soufian Jaouani from comment #29)
> Malicious addons should and will not appear on the AMO site. So you don't
> have to expect a malicious addon to be exposed on a wide scale to the
> public, and then causing harm perpetually after it is disabled/blocklisted.

We can't design assuming that the user will only have add-ons installed from AMO. For most users the opposite is true.

Comment 31

[Thomas Oberndörfer]

require('sdk/system/unload').when() never gets an "uninstall". Click on Remove for an active Add-on in the Add-ons-Manager results only in a "disable". Tested on FF24 with Addon SDK firefox24, Linux x86_64.

Comment 32

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562