:( Due to bug 620541. Fixing that one means this one is fixed.

P1 1.0, not that we have any control over it (other than pressing the Firefox/platform team to fix it).

Drew: is there any chance you might be able to tackle the Firefox/Toolkit bug to address this issue?

There's a nonzero chance I might be able to tackle it, but I'm not planning on it.

In bug 620541 Dave says we can just use the uninstall() function in bootstrap.js rather than shutdown().

I took some time to dig this bug. So When we disable an addon, we only get `shutdown` called with DISABLE reason. If we remove an addon, we immediatly get `shutdown` called with DISABLE reason too, then when we close about:addons, the addon is finally removed and uninstall is called. We can't move unload execution in uninstall, as we won't receive any call other than shutdown in disable case. So we may have to distinguish disable step from module unload in order to : 1/ first tell module to remove all registered stuff 2/ then, later on, only if it uninstall action, ask module to uninstall and remove all stuff 3/ finally, unload js modules But this appears to big a way bigger change, that may either delay 1.0 or become a post 1.0 feature/fix. Do you see any better, may be simplier way to handle undoable uninstall ? Having "uninstall" reason during shutdown call won't help us, as we should not uninstall it during this step.

Thanks for looking. Yeah, that does appear to be an unpleasantly big change. Given bug 620541, I don't see any way around it... Could you explain all of this to Dave in bug 620541 (or just point him to your comment here)? (In reply to comment #5) > Having "uninstall" reason during shutdown call won't help us, as we should > not uninstall it during this step. I might be misunderstanding you, but prior to bug 620541, when an add-on was uninstalled shutdown() was only called once with an ADDON_UNINSTALL reason, and that worked fine.

(In reply to comment #6) > (In reply to comment #5) > > Having "uninstall" reason during shutdown call won't help us, as we should > > not uninstall it during this step. > > I might be misunderstanding you, but prior to bug 620541, when an add-on was > uninstalled shutdown() was only called once with an ADDON_UNINSTALL reason, > and that worked fine. I think it was before Dave? implemented "undoable" uninstall. So yes, having only one event "shutdown with uninstall reason" would fix our issue but it is incompatible with undoable uninstall feature that has to fire two different event: - disable addon and remove all UI, unregister event listener and all, - finally remove the addon and all stored data on fs/memory/preferences and all.

Yeah it was the undoable uninstall that probably made this problem more obvious to you. We want to make some changes to how that works (which should allow us to fix bug 620541) however even if that is done you'll still have a problem in some cases. The current handling of undoable uninstalls is basically the same as the following case, which you need to be able to handle too: 1. Firefox starts 2. bootstrap.js is loaded 3. startup is called 4. User open add-ons manager and clicks disable 5. shutdown is called with ADDON_DISABLE 6. User clicks remove 7. User closes add-ons manager 8. uninstall is called Or consider perhaps a more tricky case to be handled: (User has previously disabled the add-on) 1. Firefox starts 2. User opens add-ons manager and clicks remove 3. User closes add-ons manager 4. bootstrap.js is loaded 5. uninstall is called

Let's move this discussion to the right bug. i.e. bug 571049. Then when we will solve this bigger uninstall issues, we will be able to solve the simple storage one.

Is there a workaround so that simple-storage is purged on uninstall? Can the add-on detect that it's being uninstalled and not just disabled? Then potentially going through ss.storage and deleting all keys?

If the addon is uninstalled while enabled, then you should be able to work around the bug some of the time by using `require("unload").when()` <https://addons.mozilla.org/en-US/developers/docs/sdk/1.0/packages/api-utils/docs/unload.html#when%28callback%29> to register a listener that deletes all keys when called with `reason` *uninstall*. However, if the addon is uninstalled while disabled, that workaround won't work because of bug 571049.

(In reply to Myk Melez [:myk] from comment #11) > If the addon is uninstalled while enabled, then you should be able to work > around the bug some of the time by using `require("unload").when()` > <https://addons.mozilla.org/en-US/developers/docs/sdk/1.0/packages/api-utils/ > docs/unload.html#when%28callback%29> to register a listener that deletes all > keys when called with `reason` *uninstall*. I think the point of this bug and the one or two other related bugs is that this doesn't work at all. If it did, we could fix this bug. when() is never called with an "uninstall" reason anymore.

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

https://wiki.mozilla.org/Labs/Jetpack/Release_Notes/1.2 said this bug is fixed? I tested using my addon (Fastest Search), which stores custom search engines in Simple Storage. After repack with 1.2, I uninstalled my addon, then reinstalled and found all engine stats are still there, meaning simple storage wasn't cleaned. But regardless, I actually do NOT want simple storage cleaned when my addon is uninstalled. That's because I want the engines to persist like preference, just in case the user changes mind and wants to install the addon again and they would still have their previously customized engines back. I'm not storing the engines in prefs because the JSON dump of engines would be very big. The thing is, now that https://bugzilla.mozilla.org/show_bug.cgi?id=571049 is fixed, I don't see why this bug/feature has to be fixed. Shouldn't the addon authors clean up simple storage when addon's uninstalled? That way, for those who need to keep stuff persistent in simple storage, it'd still be there next time user installs again. Of course, I could also move engines to prefs (if pref string does not have a small limit in length - I can't find doc on whether there's a limit or not, but I assume there is) instead, if possible.

(In reply to Mingyi Liu from comment #14) > https://wiki.mozilla.org/Labs/Jetpack/Release_Notes/1.2 said this bug is > fixed? No, the release notes actually say this bug is a known issue. > But regardless, I actually do NOT want simple storage cleaned when my addon > is uninstalled. That's because I want the engines to persist like > preference, just in case the user changes mind and wants to install the > addon again and they would still have their previously customized engines > back. I certainly understand this line of reasoning. However, the reason we treat the current behavior as a bug is that we think users expect and would prefer for information stored by an addon to be removed from their computer when they uninstall the addon in most cases. We do intend to keep that information around when users disable an addon, however, figuring that users are more likely to re-enable an addon they have previously disabled but less likely to re-install an addon they have previously uninstalled. In other words, disablement is more of a temporary action, even though it can be permanent; while uninstallation is more of a permanent action, even though it can be temporary. > The thing is, now that https://bugzilla.mozilla.org/show_bug.cgi?id=571049 > is fixed, I don't see why this bug/feature has to be fixed. Bug 571049 is not fixed. > Shouldn't the addon authors clean up simple storage when addon's uninstalled? The SDK's high-level APIs are designed to simplify addon development and improve addon usability by making these kinds of decisions for developers based on the behavior the API designers think users expect and most benefit from. In this case, I think it's better for the API to clean up storage automatically when the addon is uninstalled, so developers don't have to do so (or forget and leave potentially sensitive personal information lying around that both developers and users expect think has been removed), and because users expect that behavior and most benefit from it (especially when it's sensitive personal information). However, we recognize that there are exceptions--use cases the APIs don't satisfy, situations in which users expect and most benefit from different behavior--and we provide mechanisms for addon developers to utilize APIs with different behavior, of which the primary one is the ability for developers to craft their own APIs. > Of course, I could also move engines to prefs (if pref string does not have > a small limit in length - I can't find doc on whether there's a limit or > not, but I assume there is) instead, if possible. You could do that, but my recommendation would be to stick with Simple Storage and its default behavior. Alternately, you could develop your own module that does what Simple Storage does but with the slightly different behavior you prefer. You could even base your module on Simple Storage by copying its code and making the necessary modifications.

(In reply to Myk Melez [:myk] [@mykmelez] from comment #11) > If the addon is uninstalled while enabled, then you should be able to work > around the bug some of the time by using `require("unload").when()` > <https://addons.mozilla.org/en-US/developers/docs/sdk/1.0/packages/api-utils/ > docs/unload.html#when%28callback%29> to register a listener that deletes all > keys when called with `reason` *uninstall*. This is currently (24-nightly) not working. `require("unload").when()` will be called with *disable* once you click Remove in the Add-ons-Manager and nothing more. So `reason` *uninstall* does never happen.

There seems to be consensus that a good default would be: 1. If an addon is disabled, retain its associated storage. 2. If an addon is uninstalled, wipe it. Ideally, addons would be able to do this themselves via receiving an "uninstall" type onUnload, but due to bug 571049 this never happens. It also appears to be difficult to fix, and possibly controversial (bug 571049, comment 23). Is it reasonable for a patch to implement this default behavior, and we can worry about the more generic case later? This is a privacy concern many addons (the developers probably don't even realize it), including Mozilla's Lightbeam [0]. [0] https://github.com/mozilla/lightbeam/issues/472

I don't get it. Why don't you just define another method "onUninstall()" which is only called if "uninstall()" is called in the "bootstrap.js" with the argument "ADDON_UNINSTALL"? This is maybe 5 lines of code, solves all problems mentioned here and makes everybody happy. "onUnload()" it the place for all actions to be performed when Firefox is quit or the add-on is disabled and "onUninstall()" is the place for clean up code to be performed before the add-on is permanently removed (and per definition, an add-on is always first disabled before it is removed, so "onUnload()" may be followed directly by "onUninstall()", developers must be prepared for that). Whether you later add default clean-up to the SDK or not, doesn't matter and this solution will work the way "bootstrap.js" works today, no need to make any changes or depend on bug 620541. All the discussion here seems a waste of time IMHO. The bug was opened more than 3 years ago and the fix I just suggested could have been implemented years ago. I wrote add-on code using "bootstrap.js" directly in the past and I never had issues with putting the permanent clean-up code into "uninstall()" instead of "shutdown()".

Of course "onUinstall()" would have an argument about the reason, you don't want to perform a full clean-up it reason is upgrade. You may do so if reason is downgrade (if older versions may break e.g. because some data formats have changed). Reason for "onUninstall()" would be: uninstall, upgrade, downgrade. Possible reasons for "onUnload()" would be everything but "uninstall".

(In reply to Garrett Robinson [:grobinson] from comment #17) > There seems to be consensus that a good default would be: > > 1. If an addon is disabled, retain its associated storage. > 2. If an addon is uninstalled, wipe it. > > Ideally, addons would be able to do this themselves via receiving an > "uninstall" type onUnload, but due to bug 571049 this never happens. It also > appears to be difficult to fix, and possibly controversial (bug 571049, > comment 23). > > Is it reasonable for a patch to implement this default behavior, and we can > worry about the more generic case later? > > This is a privacy concern many addons (the developers probably don't even > realize it), including Mozilla's Lightbeam [0]. > > [0] https://github.com/mozilla/lightbeam/issues/472 Yes I think this is all correct, we can fix this use case without the generic use case fix, so I think we should do that and not make this depend on bug 571049 nor bug 620541. Irakli does this sound reasonable?

I am inclining to think that weather data stored by add-on is wiped during uninstall or not should be a users decision. Although this brings up an interesting UX challenge. I think what we should do is provide sane default (If an addon is disabled, retain its associated storage, If an addon is uninstalled, wipe it), but also add an option for users to opt out of this. I think this can be achieved as follows: 1. We need to add a preference per add-on to decide if stored data should be erased on uninstall. 2. If preference is not set consider it being set to true. That way: 1. sane behavior will match the one discussed above. 2. Add-on authors will be able to provide different default by providing different default via simple prefs. 3. Users will have a UI and an ultimate power to decide weather data should be erased (I wish we could have better UX for this, but this is better than nothing).

I think on addon review we should ensure that on uninstall addons delete their stuff. A user shouldn't have to hit "reset profile" to get that fresh behavior back, uninstalling all addons should give them that clean-ness.

(In reply to noitidart from comment #22) > I think on addon review we should ensure that on uninstall addons delete > their stuff. A user shouldn't have to hit "reset profile" to get that fresh > behavior back, uninstalling all addons should give them that clean-ness. So instead of fixing the issue once at the platform level, you'll be putting more burden on all addon devs (for no good reason anyway given Firefox can keep track of which simple-storage data is generated by which addon and erase it when necessary).