620541 - ADDON_DISABLE (4) sent for shutdown reason when uninstalling

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Ed Lee :Mardak]

I believe it's because uninstalling doesn't actually uninstall until you close/refresh about:addons. So the add-on gets disabled first, but the reason sent is ADDON_DISABLE instead of ADDON_UNINSTALL.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yep.

Comment 2

[Drew Willcoxon :adw]

This breaks Jetpack.  Flipping the tracking switches to maybe get this on someone's radar.

Comment 3

[Dave Townsend [:mossop]]

Wasn't aware it was such a problem, I'll take a look at it. Tracking isn't really for this purpose though.

Comment 4

[Dave Townsend [:mossop]]

Forgot just how much work this was likely to entail. Why is it jetpack is so broken by this? The simple workaround is just to do whatever cleanup you wanted to do in the shutdown method (when ADDON_UNINSTALL is passed) in the uninstall method instead.

Comment 5

[Drew Willcoxon :adw]

Thanks Dave, I'll look into that.

Comment 6

[Alexandre Poirot [:ochameau]]

I think it would be helpfull in some cases to fix this bug but it won't solve our jetpack issue. Even if we know that shutdown call is about uninstall we have to split unload in two part: 
 - addon disabling with mainly UI removal
 - addon removing with mainly fs and preferences removal
Then we can unload/unregister commonjs modules, but not immidiatly on shutdown call, as they need to be kept alive to remove stuff later on uninstall call.

Dave: can you take a look at my comments in bug 627432 to confirm that we are going to keep "undoable uninstall" feature and so, we have to make bigger change in jetpack unload mechanism?

Comment 7

[Dave Townsend [:mossop]]

Based on this being quite a challenge to do and it looking like the Addon's SDK still needs to handle this case anyway I'm dropping this off my radar for the time being. Let me know if I've misread the situation.

Comment 8

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

(In reply to comment #6)
> Dave: can you take a look at my comments in bug 627432 to confirm that we
> are going to keep "undoable uninstall" feature and so, we have to make
> bigger change in jetpack unload mechanism?

Bug 627432, comment 8 suggests we are indeed going to keep undoable uninstall, and it also points out two other cases in which the SDK needs to be able to handle uninstall of a disabled addon.

So it doesn't sound like a fix for this bug will really address the SDK's issue, and, as Dave suggests in bug 571049, comment 12, this bug should be wontfixed, and we should implement support for disable-then-uninstall in the SDK.

Comment 9

[Wladimir Palant]

(In reply to Dave Townsend (:Mossop) from comment #4)
> The simple workaround is just to do whatever cleanup you
> wanted to do in the shutdown method (when ADDON_UNINSTALL is passed) in the
> uninstall method instead.

Nope, the uninstall method simply isn't getting called - the extension is already disabled. In other words, the extension currently has no chance to distinguish a regular disable action from uninstall. Personally, I wouldn't care whether the uninstall can be undone - I would expect an ADDON_UNINSTALL notification allowing me to clean up.

Comment 10

[Wladimir Palant]

Please ignore the comment above - it is being called, just not when I expected...

Comment 11

[Kris Maglione [:kmag]]

I think the main problem with this is that it's natural for add-ons to have torn down the code which is in a position to preform uninstall actions by the time the uninstall callback is called.

The most obvious case is Jetpack, where modules will have been destroyed at shutdown, and without some kind of special-purpose uninstall script, would have to be reinitialized to call uninstall listeners. This is obviously unacceptable, since most Jetpack add-ons begin their initialization as soon as main.js is loaded, and would therefore make any number of slow and noticeable UI changes before being disabled and running their uninstall code.

I'm not exactly hostile to the idea of a special uninstall script for Jetpack add-ons, but it's still less than ideal to have uninstall code run after the bulk of an add-on has been torn down. My tendency would be to keep uninstall cleanup code near to the code that made the changes that need to be cleaned up, which this makes impossible.

Comment 12

[Michael Kraft [:morac]]

I agree with comment #11.  By the time the uninstall method is my modules have been unloaded.  Only the most basic of functionality is left.  That means I either need to reload my modules, execute uninstall stuff and unload the modules again or duplicate the code in bootstrap.js.

Comment 13

[TGOS]

(In reply to Michael Kraft [:morac] from comment #12)
> By the time the uninstall method is my modules
> have been unloaded.  Only the most basic of functionality is left.

And you really need more than that basic functionality to perform the final clean up? I would say the final clean up shouldn't do much more than maybe removing created files (e.g. in the profile folder), deleting created preferences (to keep pref system clean and for privacy reasons) and maybe undoing modifications if possible.

I never had a problem doing all this in the "uninstall()" method (NOT in "shutdown()") as even if shutdown was called before, my add-on is still fully functional, except for functionality I intentionally destroyed in "shutdown()" and this functionality is hardly needed for the final clean-up (and even if it was, it is easy to temporarily restore it).

So IMHO this bug is invalid and should be closed. "shutdown()" is simply never called with ADDON_UNINSTALL, only "uninstall()" may be called with that argument and per definition an add-on is always first disabled before it is uninstalled (that's what the documentation says and that's the way it always has been working).

This may mean some more work for some add-on developers, well, so be it. Life is no pony farm and we cannot make everybody happy. Right now this bug is in a state of uncertainty, nothing has happened within almost 4 years and nothing will happen within the next 4 years and any waiting for a solution is futile. Add-on developers had to work around it already for years or live with the imperfection. If you close this bug as "works as intended", you kill at least all uncertainty. Bug 582002 doesn't really depend on it, one could use alternative methods to make a second manager aware of the state of each add-on and bug 627432 doesn't depend on it either, as one could give Jetpack an extra uninstall hook to solve the problem.

Comment 14

[Dave Townsend [:mossop]]

This will probably be fixed if we ever get around to fixing bug 612168.

(In reply to TGOS from comment #13)
> So IMHO this bug is invalid and should be closed. "shutdown()" is simply
> never called with ADDON_UNINSTALL, only "uninstall()" may be called with
> that argument and per definition an add-on is always first disabled before
> it is uninstalled (that's what the documentation says and that's the way it
> always has been working).

Which documentation says this? The formal bootstrap.js documentation is pretty clear that ADDON_UNINSTALL is one of the possible reasons passed to shutdown(): https://developer.mozilla.org/en-US/Add-ons/Bootstrapped_extensions#shutdown

Comment 15

[Andrew Swan [:aswan]]

This appears to be fixed but I would appreciate confirmation that I'm not overlooking something before closing.

I believe the logic in question lives at these two lines:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4905
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4922

And it is tested in multiple cases including for example:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_undouninstall.js#318

Comment 16

[Kris Maglione [:kmag]]

This is probably going to change the behavior of some of the SDK shutdown/uninstall code. Someone should probably check whether those bugs are fixed (or whether we've created new ones).

Comment 17

[Andrew Swan [:aswan]]

whoops, didn't create permalinks for the dxr links in comment 15, they should be:

https://dxr.mozilla.org/mozilla-central/rev/ac39fba33c6daf95b2cda71e588ca18e2eb752ab/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4905
https://dxr.mozilla.org/mozilla-central/rev/ac39fba33c6daf95b2cda71e588ca18e2eb752ab/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4922

https://dxr.mozilla.org/mozilla-central/rev/ac39fba33c6daf95b2cda71e588ca18e2eb752ab/toolkit/mozapps/extensions/test/xpcshell/test_undouninstall.js#318

Comment 18

[Dave Townsend [:mossop]]

Yes I believe that this is fixed too