When a user presses the "Remove" button then Addon.userDisabled = true; (but the Addon.uninstall() method should be called here), then when a user presses "Undo" Addon.userDisabled = true (but Addon.cancelUninstall() should be called here). Then when the user closes the about:addons window, the Addon.uninstall() is finally called. Afaict Addon.cancelUninstall() cannot being called.

(In reply to comment #0) > When a user presses the "Remove" button then Addon.userDisabled = true; (but > the Addon.uninstall() method should be called here), then when a user presses > "Undo" Addon.userDisabled = true (but Addon.cancelUninstall() should be called > here). > > Then when the user closes the about:addons window, the Addon.uninstall() is > finally called. Afaict Addon.cancelUninstall() cannot being called. This doesn't seem to be the case according to the automated tests we have running and I just tested it manually and it all seems to work fine. Do you have some steps to reproduce this problem?

(In reply to comment #1) > Do you have some steps to reproduce this problem? I'm using http://github.com/erikvold/scriptish and put a Scriptish_log("funcName") call into each of the methods I mention I above. You could probably do the same with SlipperyMonkey.

If I'm understanding comment 0 right, that's by design (for restartless addons, at least). When you click remove, it only pretends to remove the addon, so we can undo (which requires that the addon not be deleted, which really uninstalling it would do). When you close the addons manager (or switch views), then it will actually remove it. And since its not actually uninstalling it when you click the remove button, there's no need to call cancelUninstall (I think that method may only be used for non-restartless addons). Additionally, since its not really uninstalling, but we still want the addon to immediately be unloaded, the addon is disabled. When you click undo, it'll re-set it to its original state.

Yeah, the UI thinks that your add-ons don't need a restart to uninstall (which it looks like is true) so it just disables them at first so the user has the option of undoing the install. If they don't undo it then when they close the window or switch to a different list the uninstall method will get called to uninstall it. cancelUninstall will never get called when the add-on doesn't need a restart to uninstall since it is assumed to be already gone. If you want to tell the UI that a restart is necessary to uninstall then you need to add an operationsRequiringRestart property to your script objects, I've added the documentation for that to https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon#Optional_attributes

(In reply to comment #4) > If you want to tell the UI that a restart is necessary to uninstall then you > need to add an operationsRequiringRestart property to your script objects I don't want to do that when a restart is not necessary because that would provide a "restart now" button(s) to the user that should not be displayed. So, how am I supposed to know when a user clicks 'Remove" and not "Disable", so that I can disable other UI accordingly?

I'm re-opening this because it looks as though there is no way for an external add-on developer to know if a user clicked "Remove" or "Disable", and that is something that some external add-on devs will need to know.

I'm not sure what you want here. When the add-on is disabled it should be inactive, basically as far as any other UI in the app is concerned the add-on should be absent. When uninstall is called the add-on should be removed completely and would have to be reinstalled again to be re-used. What are you expecting to be able to do between the user clicking Remove and them closing the add-ons manager that this doesn't allow?

(In reply to comment #7) > What are you expecting to be able to do between the user clicking Remove and > them closing the add-ons manager that this doesn't allow? With GM and Scriptish: There is a status bar icon (which is now a toolbar menu-button for Scriptish) from which a user can activate a menu, which includes as menu items the user scripts which would run on the currently selected content page (and it's iframes) with a check mark beside it which indicates if the script is disabled or enabled, and clicking one of these menu items will toggle the status of the associated user script. So if a user script is simply disabled, then the user should be allowed to change the status from this menu item, if the user clicks uninstall then the menu item should no longer be available, even if the user never closes their about:addons tab. Atm if the user presses the "remove" button, and I simply disable the user script because I can't differentiate between a uninstall and a disable, then the user can still see the script in the status bar menu, and enable it again..

The current situation is really a stepping stone to how this should work, we should be letting the provider handle the uninstall/undo cycle but at the time it seemed good enough to handle it on the UI side.

Sync would also like to distinguish between true disables and uninstall precursor disables. As it is currently implemented, Sync buries its head in the sand and assumes the onDisabled is a real disabled request. There are some corner cases in how synchronized records are played out on other devices, but the end result should all be the same.

Test Pilot also wants to 'really nuke' secondary addon installs as well.

(In reply to Gregg Lind (User Research - Test Pilot) from comment #11) > Test Pilot also wants to 'really nuke' secondary addon installs as well. Could you provide some context?

TestPilot2 (will!) works by installing add-ons (the experiments). When a study is complete, it would be nice to be able to restore the state of the system. Knowing that it went back to "uninstalled" rather than just "disabled" would be both polite. Perhaps I am misunderstanding what is and isn't possible here.

(In reply to Gregg Lind (User Research - Test Pilot) from comment #13) > TestPilot2 (will!) works by installing add-ons (the experiments). When a > study is complete, it would be nice to be able to restore the state of the > system. Knowing that it went back to "uninstalled" rather than just > "disabled" would be both polite. Calling uninstall() through the add-ons manager API will completely uninstall the add-on (where possible). This bug is only about the UI.

This is an old WIP patch I have that IIRC works for heavy themes and extensions. The only problem is lightweight themes which it would be extremely complex to solve until bug 520124 is fixed so I'm waiting on that. In the meantime posting this somewhere I won't accidentally lose it.

I may be missing context here, but why does this need to block on bug 520124? Can't we add per-provider opt-out of the current undo behavior and handle specific providers separately?

(In reply to Georg Fritzsche [:gfritzsche] from comment #16) > I may be missing context here, but why does this need to block on bug 520124? > Can't we add per-provider opt-out of the current undo behavior and handle > specific providers separately? I think that just due to the way Dave had done that patch. I think we probably could do it on a per-provider basis. There is the issue of add-ons, after all - so in general it'd be good for this to not be a required API if possible (providers can opt-in via a flag on their AddonType).

Looking at Dave's old patch, I'm not sure I like/understand the aForcePending parameter. I'd rather have it automatically assume the uninstall can be undone, and have an optional parameter to force uninstall it ASAP.

Opt-in for ASAP uninstall definitely sounds good. Dave, would you consider this and do you have time to finish this up when you're back?

Blair says he can do it, this iteration or next, to help unblock bug 1010397.

Hi Blair, are you taking Bug 612168 during the current iteration?

Yea, think I'm going to need to tackle this bug this iteration, and push bug 1002698 and bug 1014313 into the next iteration.

Thanks for the update Blair. I'll add Bug 612168 to this iteration and remove Bug 1002698 and Bug 1014313. For Bug 612168, should it be marked as [qa+] or [qa-] for verification?

No progress on this yet - too busy with other stuff. But it's not forgotten! (No way it'll make this iteration though.)

(In reply to Georg Fritzsche [:gfritzsche] from comment #16) > I may be missing context here, but why does this need to block on bug 520124? > Can't we add per-provider opt-out of the current undo behavior and handle > specific providers separately? The problem is down to how lightweight and heavyweight themes interact. Uninstalling a lightweight theme has to enable the default heavyweight theme and other complexities. So it's not as simple as just doing it per provider, the lightweight theme provider is strongly tied to the xpiprovider, until bug 520124. It's probably not impossible to unravel but it would just be a lot simpler to do after that and so since it wasn't a priority to do this in the past I chose to defer.

(In reply to Blair McBride [:Unfocused] from comment #18) > Looking at Dave's old patch, I'm not sure I like/understand the > aForcePending parameter. I'd rather have it automatically assume the > uninstall can be undone, and have an optional parameter to force uninstall > it ASAP. Mostly I did it that way for backwards compatibility. I wanted the call to addon.uninstall() to behave the same as it does today for add-ons that call it.

(In reply to Georg Fritzsche [:gfritzsche] from comment #19) > Opt-in for ASAP uninstall definitely sounds good. > > Dave, would you consider this and do you have time to finish this up when > you're back? I could potentially review, I don't have other time to work on this though so good thing Blair has taken it on!

(In reply to Dave Townsend [:mossop] from comment #26) > (In reply to Blair McBride [:Unfocused] from comment #18) > > Looking at Dave's old patch, I'm not sure I like/understand the > > aForcePending parameter. I'd rather have it automatically assume the > > uninstall can be undone, and have an optional parameter to force uninstall > > it ASAP. > > Mostly I did it that way for backwards compatibility. I wanted the call to > addon.uninstall() to behave the same as it does today for add-ons that call > it. Yea, I eventually came to the same conclusion. And not just add-ons calling uninstall(), but other code in the tree too - it's mostly just about:addons interested in undo.

Comment on attachment 8449963 [details] [diff] [review] Patch v1 Oops, forgot to remove some code and check something to do with themes.

Ok, so, themes: I've left themes working as-is. The only time this really becomes troublesome is when we have DSS (dynamic skin switching) enabled, which lets you switch heavyweight themes without restarts. But then, bug 572715 already prevents us from getting into this situation of DSS heavyweight themes and lightweight themes. On top of that, we never ship with DSS enabled and we never use it, because it's always been too buggy to use. It also just generally makes some of the theme code unnecessarily complex. So I don't see any point in supporting it, and have filed bug 1033928 to completely remove support for DSS.

Comment on attachment 8449972 [details] [diff] [review] Patch v1.1 Review of attachment 8449972 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +2564,5 @@ > TYPE_SUPPORTS_ASK_TO_ACTIVATE: 32, > + // The add-on type natively supports undo for restartless uninstalls. > + // If this flag is not specified, the UI is expected to handle this via > + // disabling the add-on, and performing the actual uninstall at a later time. > + TYPE_SUPPORTS_UNDO_RESTARTLESS_UNINSTALL: 64, In general I'm a "death to bitwise flags!" guy, but it was like this when we found it... ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm @@ +4427,5 @@ > * The DBAddonInternal to uninstall > * @throws if the addon cannot be uninstalled because it is in an install > * location that does not allow it > */ > + uninstallAddon: function XPI_uninstallAddon(aAddon, aForcePending) { Update the doc-comment with the new aForcePending parameter; explain (either there or in another comment) that you're using "stage this add-on for uninstall after restart" as a way of deferring what would otherwise be a restartless uninstall (at least, that's what I *think* you're doing, thus the request for an explanatory comment). @@ +5535,5 @@ > + // Remove any staged items for this add-on > + stagedAddon.append(this.addon.id); > + yield removeAsync(stagedAddon); > + stagedAddon.leafName = this.addon.id + ".xpi"; > + yield removeAsync(stagedAddon); if removeAsync can throw/reject (for instance on file access failures) you should do something catchy here. ::: toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js @@ +59,5 @@ > + do_check_true(d.isActive); > + do_check_false(d.userDisabled); > + do_check_eq(Services.prefs.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN), "classic/1.0"); > + > + run_test_1(); Tests in this suite tend to be unreliable if you don't let the stack unwind from inside AddonManager callbacks, so: do_execute_soon(run_test_1); and similarly for all the other places you chain test functions. Better yet, rewrite the new tests in add_task / yield Promise style, which always lets the stack unwind... @@ +79,5 @@ > + do_check_true(t1.userDisabled); > + > + t1.userDisabled = false; > + > + restartManager(); restartManager() inside a callback was the most common cause of failures when I was working on the sqlite=>JSON conversion; I ended up making sure there was always something unwinding the stack in between. If you don't want to switch to Tasks, you can use the callback_soon() function=>function transformer I added to head_addons.js, like: AM.getAddonByID(id, callback_soon(function(t1){...})); ::: toolkit/mozapps/extensions/test/xpcshell/test_undouninstall.js @@ +1,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + */ > + > +// This verifies that forcing undo for uninstall works All the same comments about rewriting to add_task() or otherwise unwinding the stack from callbacks before shutdownManager() or restartManager(). ::: toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini @@ +233,5 @@ > # Bug 676992: test consistently fails on Android > fail-if = os == "android" > [test_types.js] > +[test_undothemeuninstall.js] > +[test_undouninstall.js] Do these really need to run twice (once packed and once unpacked)? I've been putting new tests in xpcshell.ini if they don't cover any aspects of packed/unpacked that our existing tests already do.

Removed from Iteration 33.3. De-prioritized over other higher priority work contained in the Iteration Priority List.

(In reply to :Irving Reid from comment #32) > In general I'm a "death to bitwise flags!" guy, but it was like this when we > found it... *sigh* Yea. We didn't have a nice alternative at the time. As it is, my preferred method involves using a Proxy to make a read-only view of a Set, since there's no built-in way to do that. (Curious if you have an alternative!)

(In reply to Blair McBride [:Unfocused] from comment #34) > (In reply to :Irving Reid from comment #32) > > In general I'm a "death to bitwise flags!" guy, but it was like this when we > > found it... > > *sigh* Yea. We didn't have a nice alternative at the time. As it is, my > preferred method involves using a Proxy to make a read-only view of a Set, > since there's no built-in way to do that. (Curious if you have an > alternative!) I'd probably just make those properties separate attributes of the add-on, so the code is just 'if (addon.supportsUndoRestartlessInstall) {... ' There isn't really a non-awkward way to do it, if the logic reacting to the property lives outside the add-on object itself. (that bumps up against the ideas of the anti-if campaign http://antiifcampaign.com/, some of which could simplify the add-on manager a lot)

About the attached patch: I tried to address most of Irving's review comments but there were two that I don't know the answer for: 1. the comment about removeAsync() possibly throwing in XPIProvider.jsm 2. the question about whether the new xpcshell tests need to run both packed and unpacked

Comment on attachment 8713266 [details] [diff] [review] update of proposed patch from Unfocused Review of attachment 8713266 [details] [diff] [review]: ----------------------------------------------------------------- Ok this is looking good however I'd like to see the test_undouninstall test updated to use the newer BootstrapMonitor instead of the pref setting stuff it does. BootstrapMonitor does a lot of checks when the bootstrap methods are called that the add-on is in the correct state. The add-on's bootstrap.js file should just look like this: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/bootstrap.js. Then in the test call BootstrapMonitor.init(). Then you can use various ways to check that the add-on is in the right state at various points, https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js has examples. Mostly here you'll just use the checkAddonInstalled/Started methods and some checks on the shutdown and startup reasons. ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +3048,3 @@ > VIEW_TYPE_LIST: "list", > > + // Constants describing how add-on types behave. Put a newline after this just so it is clearly separate from the following comment about one of the specific behaviours. ::: toolkit/mozapps/extensions/content/extensions.js @@ +1209,5 @@ > return; > } > > gViewController.popState(function() { > + gViewController.currentViewObj.getListItemForID(aAddon.id).uninstall(true); getListItemForID is giving us the XUL element in the list for the add-on here and calling its uninstall method which is defined in the XBL binding in extensions.xml (I'm sorry you have to look at XBL so soon!). We're not adding a parameter to it so we don't need to add the true parameter here. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm @@ +4976,5 @@ > // TODO hide hidden add-ons (bug 557710) > let wrapper = aAddon.wrapper; > AddonManagerPrivate.callAddonListeners("onOperationCancelled", wrapper); > > + if (aAddon.bootstrap && !aAddon.userDisabled && !this.enableRequiresRestart(aAddon)) { Instead of userDisabled you should check the disabled property here otherwise it would allow starting an add-on that is incompatible with the application. That implies we don't have a test for that case so you'll need to add something to browser_undouninstall.js which installs an incompatible add-on, calls addon.uninstall(true) then cancelUninstall() and checks that the add-on doesn't start up. ::: toolkit/mozapps/extensions/test/browser/browser_uninstalling.js @@ -1095,5 @@ > - }); > - }); > - }); > - }); > -}); I don't know why Blair removed these tests, I think we still need them. ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js @@ +1521,5 @@ > let count = aInstalls.length; > > if (count == 0) { > + if (aCallback) > + aCallback(); It doesn't look like this change is necessary for this patch. @@ +1542,5 @@ > }; > > aInstalls.forEach(function(aInstall) { > + if (aCallback) > + aInstall.addListener(listener); Same here. ::: toolkit/mozapps/extensions/test/xpcshell/test_undothemeuninstall.js @@ +319,5 @@ > + > + do_check_false(t1.isActive); > + do_check_true(t1.userDisabled); > + do_check_true(hasFlag(t1.pendingOperations, AddonManager.PENDING_UNINSTALL)); > + Style nit: trailing whitespace @@ +344,5 @@ > + restartManager(); > + > + [ t1, d ] = yield promiseAddonsByIDs(["theme1@tests.mozilla.org", > + "default@tests.mozilla.org"]); > + Same here @@ +363,5 @@ > +}); > + > +//Tests that uninstalling an enabled lightweight theme offers the option to undo > +add_task(function* uninstallLWTOffersUndo() { > + return; Add a comment saying that lightweight themes don't support undoable uninstall yet.

(In reply to Andrew Swan [:aswan] from comment #37) > About the attached patch: I tried to address most of Irving's review > comments but there were two that I don't know the answer for: > 1. the comment about removeAsync() possibly throwing in XPIProvider.jsm We catch errors from this entire function and log them here: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6081 So I think we're fine there. If we can't remove those files then the install is not going to go so well. > 2. the question about whether the new xpcshell tests need to run both packed > and unpacked I think they do. If an add-on has been forced into the pending state and we restart the add-ons manager we need to make sure that the add-on is uninstalled. That could plausibly differ between the two cases.

(In reply to Dave Townsend [:mossop] from comment #38) > I don't know why Blair removed these tests, I think we still need them. No idea either, FWIW. Although they won't be testing the right thing anymore (wrong addon type).

This applies on top of your patch and makes all tests pass for me. There are a few changes in here: Moved some of the state checks into uninstallAddon and cancelUninstallAddon, it makes a bit more sense for them to be contained there rather than in the wrapper methods. Changed what we send for the requiresRestart parameter for onUninstalling events. I'm not sure it makes complete sense either way but it has generally been used as an indicator of whether an add-on is going away right now or later so the way I have it here fits with that. Fixes for temporary add-ons, turned out to be quite simple as they naturally uninstall after a restart. Removed some test changes where I'd rather just leave things as they are. There are a couple of additional tests that I don't see and I would like to see added: When a restartless add-on is pending uninstall and also incompatible with the application cancelling the uninstall should not call the add-ons startup method. There is a logic error I fixed in this patch that would have caused that. Putting a temporary add-on into the pending uninstall state and cancelling it should work. If you want to add those tests and send it back to me for review then I think we can close this out.

Comment on attachment 8714968 [details] [diff] [review] update 4 of proposed patch from Unfocused Review of attachment 8714968 [details] [diff] [review]: ----------------------------------------------------------------- A few commnents on the test file (I know it may not be your work!) Please apply them to the other test file too if appropriate. ::: toolkit/mozapps/extensions/test/xpcshell/test_undouninstall.js @@ +122,5 @@ > + > + do_check_eq(list.length, 1); > + do_check_eq(list[0].id, "addon1@tests.mozilla.org"); > + > + restartManager(); It's a little nicer to do "yield promiseRestartManager();" @@ +327,5 @@ > + a1.uninstall(); > + ensure_test_completed(); > + > + a1 = yield promiseAddonByID(ID); > + Style nit: This file has a bunch of trailing whitespace in it, please remove those. @@ +696,5 @@ > + do_check_false(a1.isActive); > + do_check_true(a1.userDisabled); > + > + shutdownManager(); > + startupManager(false); Anywhere you do this you can just do "yield promiseRestartManager();"

Comment on attachment 8715959 [details] [diff] [review] update 5 of proposed patch from Unfocused Review of attachment 8715959 [details] [diff] [review]: ----------------------------------------------------------------- Awesome! Just a couple of minor things to change then this can land. If you haven't already please do a full test run through the try server before marking for checkin to make sure we're not upsetting other tests that rely on the add-ons manager for some reason. ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +3048,4 @@ > VIEW_TYPE_LIST: "list", > > + // Constants describing how add-on types behave. > + Style nit: trailing whitespace ::: toolkit/mozapps/extensions/test/browser/head.js @@ +1219,5 @@ > if (!needsRestart) { > this.pendingOperations -= AddonManager.PENDING_UNINSTALL; > this._provider.removeAddon(this); > + } else { > + this.isActive = false; This should only happen if !(this.operationsRequiringRestart & AddonManager.OP_NEEDS_RESTART_DISABLE) ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js @@ +2038,5 @@ > + * @resolve {array} The list of add-ons sent by > + * AddonManaget.getAddonsWithOperationsByTypes to its callback. > + */ > +function promiseAddonsWithOperationsByTypes(aTypes) { > + return new Promise((resolve, reject) => AddonManager.getAddonsWithOperationsByTypes(aTypes, resolve)); reject is never used so you don't need to list it. ::: toolkit/mozapps/extensions/test/xpcshell/test_undouninstall.js @@ +756,5 @@ > + > + let a1 = yield promiseAddonByID(INCOMPAT_ID); > + do_check_neq(a1, null); > + BootstrapMonitor.checkAddonNotStarted(INCOMPAT_ID); > + dump(getStartupReason(INCOMPAT_ID) + '\n'); Remove this before landing.

This needs to be rebased on top of fx-team tip.

Hey Andrew, For the future, commit messages should have the bug # first and the reviewer at the end. E.g. I used: > Bug 612168 - Handle uninstalls of restartless addons in XPIProvider. r=Mossop See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions Cheers

https://hg.mozilla.org/mozilla-central/rev/8a3e27d77ba4 https://hg.mozilla.org/mozilla-central/rev/fdfc5a1808c4

Andrew, there are a few bugs that this blocks, can you go through and see which were fixed by it and mark them accordingly?

This is probably worth including in the updates to developers.

Sorry that took a while but we're down to two unresolved bugs that were blocked on this one, and they both require further work.

From the patch, it looks that the needsRestart flag for the onUninstalling event changes its meaning; it should be noted, at least, in: https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/AddonListener#onUninstalling%28%29 I got confused for a good time seeing how this value was true for langpacks.