Issue 6689082285490176: Issue 1598 - The set up was unable to automatically close IE while uninstall
Description1. Custom action is not being run at all on uninstall now, since it is set in InstallUISequence. Here we set it for InstallExecuteSequence. In this case the custom action will run right before the actual installation, after the user clicks the last "Next". Which is a better place to run it too, IMO.
2. I would remove the partKnown state altogether. Right now we are effectively asking the user 2 times if s/he would like to close IE. For now, since the uninstall runs with /qb let's default to partKnown, so that the installer would close IE on user's consent.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Treat Basic UI as interactive mode #
MessagesTotal messages: 14
Not LGTM. There's a basic bad design in the Windows Installer, which is that you can have states where a current running program does not match the installation state of the program. For example, you can be running a previous version of software when a more current version is the one installed. That's not exactly technically true, since the installation is upgraded at boot, but it's a mismatch for the user experience, because a user thinks they've installed something but it hasn't happened yet. The present custom action is specifically designed for interactivity at install. I hadn't even considered uninstall. The typical default for install is that you can install now and the installation appears when you reboot. This is why the default state for non-interactive install in the CA is "allow", that is, "allow reboot", which also means "allow IE to continue running". This is the wrong default for uninstall. It's not only the wrong default, it can be the wrong behavior entirely. We use an immediate uninstall, not one deferred to reboot (which takes extra effort). For immediate uninstall, you need to ensure that IE is not running, otherwise you get errors. For example, uninstalling the dictionaries while there's still a BHO loaded means no useful strings in the status bar menu. You can use a deferred uninstall script that runs at reboot, but that's something else to set up, and it's not in the current code. At minimum, we need at least one new CA or deferred uninstall. The current CA can terminate with the property "browserRunning" set to true. This is OK only if we have deferred uninstall available. If we don't, then we need another CA (or a heavily modified and overloaded version of the one we have, which I wouldn't recommend). Here are the basic decisions to be made: 1) Is deferred uninstall available? If it is, you can allow uninstall with IE running. 2) Is the behavior for non-interactive uninstall with IE running to try to kill it or to simply abort? (You might have to abort anyway if it can't be killed.) http://codereview.adblockplus.org/6689082285490176/diff/5629499534213120/inst... File installer/src/custom-action/close_application.cpp (right): http://codereview.adblockplus.org/6689082285490176/diff/5629499534213120/inst... installer/src/custom-action/close_application.cpp:225: state = notKnown ; Assigning a state that you just checked is not needed. But if you want to change the default behavior, the correct state would be 'automatic'. The 'notKnown' action in the state machine has a precondition that you're in an interactive session. This block is guarded by the condition that we're not interactive, so you've got a logic error if you assign 'notKnown' here. http://codereview.adblockplus.org/6689082285490176/diff/5629499534213120/inst... File installer/src/msi/adblockplusie.wxs (right): http://codereview.adblockplus.org/6689082285490176/diff/5629499534213120/inst... installer/src/msi/adblockplusie.wxs:186: <InstallExecuteSequence> This doesn't do what you want. The whole point of running the CA we have in the UI sequence is that it has UI elements in it. The UI sequence is only run when the UI is active. The execute sequence is run in all cases, even when run in silent mode. We'll be breaking a very basic integrity requirement of Windows Installer in that circumstance.
That shouldn't have shown up green. I thought "N-o-t L-G-T-M" highlighted differently. Maybe it should be "NOT LGTM".
Oh well. NOT GREEN.
As far as the "partKnown" state goes, that's primarily a workaround of the very limited interactivity you've got available in a CA. The most variety in answers you can get is "yes-no-cancel", which in practice means you can only as yes-no questions. What we want out of the CA, though, is three choices: 1) Shut down IE for me. 2) Wait for me to shut down IE. 3) Don't shut down IE; I'll reboot later. The problem is that you have to select these choices through a series of yes-no questions. If it were up to me, I'd have used a three-option radio button set.
Let's take off our engineer hats and use our user hats for a moment. I mean let's not care about the underlying design at all for a bit. The reason behind this change-set is a bad user experience while uninstalling. It goes like this: * Uninstall is launched, our custom action is not launched * uninstall shows the "Applications in use" dialog. * User chooses to close applications * Uninstaller fails to close applications <--- Bad user experience. That "Applications in use" box pop up no matter if you're running with /qb or without. We should either disable that "Applications in use" or substitute it for something that actually works. I opted for the later option here, since we already have that built anyway. > For immediate uninstall, you need to ensure that IE is not running, otherwise you get errors. I agree, which is why we need to run our custom action to be able to ensure the IE is closed > 2) Is the behavior for non-interactive uninstall with IE running to try to kill > it or to simply abort? (You might have to abort anyway if it can't be killed.) We should let the user decide whether to kill it or abort. Which is what the current patchset does.
On 2015/03/18 18:19:55, Oleksandr wrote: > The reason behind this change-set is a bad user experience while uninstalling. I'm not arguing about that at all. > We should either disable that "Applications in use" or substitute it for > something that actually works. > I opted for the later option here, since we already have that built anyway. I fact we do _not_ have that already built. We have something close that doesn't do the right thing for uninstall. > > For immediate uninstall, you need to ensure that IE is not running, otherwise > you get errors. > > I agree, which is why we need to run our custom action to be able to ensure the > IE is closed _Our_ custom action is the custom action for install. It is not the right custom action for uninstall, not as currently written. It might be possible to set up the right properties in advance of calling the current CA with a bunch of conditionals. That kind of MSI programming is just awful; it's write-only software; it's unreadable unless you deal with it all the time. > > 2) Is the behavior for non-interactive uninstall with IE running to try to > kill > > it or to simply abort? (You might have to abort anyway if it can't be killed.) > > We should let the user decide whether to kill it or abort. In a non-interactive session, you can't use an interactive means to let the user decide. That's why criterion (2) above requires specifying a default for non-interactive sessions. If you want them to be able to specify a non-default behavior for a non-interactive session, you can use a property definition that the user can put on the command line. I forget what happens when you try and ask for a dialog box in a non-interactive session. I seem to remember it just aborts the CA, but I don't recall exactly. And whatever it does, it does it worse with the older version of Windows Installer that shipped with XP. > Which is what the > current > patchset does. The current patch set introduces a new kinds of failures. -- It tries to create a dialog box in a non-interactive session, which violates the API contract. -- It changes the behavior of the _installation_ behavior. And it doesn't even guarantee that IE is closed at the end of the custom action. The easiest way to get this working is to write a second CA behaves like you want. It should only return a success code if IE is closed (which is not how the current version works, and not how the change set under review works, either); otherwise, it should abort. That custom action needs to be guarded so that it only runs on uninstall. For that matter, it's a separate defect that the current CA can run on during an interactive uninstall; my bad.
I think the core question here is this: what should be an interactive mode and what shouldn't. Currently non-interactive are uimode 2 and 3. That is silent and basic UI. Which is wrong, I think. In the code and in the comments here you state that Basic UI (/qb) installations can't have UI. This doesn't seem to be true, from my tests. Nor have I found any documentation backing that. Even more, Windows Installer itself creates already mentioned "Applications in use" dialog box when running in Basic UI mode. Our dialog boxes are displayed in Basic UI mode as well. Hence I don't think we are breaking API contracts.
On 2015/03/19 02:14:46, Oleksandr wrote:
> In the code and in the comments here you state that Basic UI (/qb)
installations
> can't have UI.
> This doesn't seem to be true, from my tests. Nor have I found any
documentation
> backing that.
Here's the documentation:
Determining UI Level from a Custom Action
https://msdn.microsoft.com/en-us/library/windows/desktop/aa368281%28v=vs.85%2...
> For example, a custom action that has a dialog box should only display the
dialog
> when the user interface level is Full UI or Reduced UI, it should not
display the
> dialog if the user interface level is Basic UI or None.
User Interface Levels
https://msdn.microsoft.com/en-us/library/windows/desktop/aa372391%28v=vs.85%2...
> Basic UI
> Does not display any authored dialog boxes.
As usual, there's no lack of ambiguity in the documentation. In the second page,
"authored dialog boxes" means (I think) dialogs in the dialog table of the MSI.
But the first page also has an explicit recommendation for CA's. The URL in the
source code comment is for a less-informative page; we should add the first link
above into the code.
> Even more, Windows Installer itself creates already mentioned "Applications in
> use" dialog box
> when running in Basic UI mode.
Basic UI doesn't mean zero-UI; that's silent mode (level=2).
> Our dialog boxes are displayed in Basic UI mode
> as well. Hence I don't
> think we are breaking API contracts.
As far as dialog boxes being displayed in Basic UI mode _for you_, that may well
work. But you're (very likely) on Windows Installer 5.0, and I suspect that this
advice dates back to a previous version of WI. Here's a table of what shipped
when.
Released Versions of Windows Installer
https://msdn.microsoft.com/en-us/library/aa371185%28v=vs.85%29.aspx
If we're going to violate an explicit recommendation, we should really test it
(_before_ shipping) on these previous versions.
I have a vague recollection of reading about the reason for this advice, but
it's a pretty vague memory. I do seem to recall that the advice was justified.
----------
In larger issues, the present change set (even barring UI level issues) doesn't
work, since it's possible for the user to make a series of CA dialog button
presses that leaves IE running on uninstall. We still need code that checks
whether we're doing install or uninstall and behaves accordingly.
Here's a relevant SO question:
How to add a WiX custom action that happens only on uninstall (via MSI)?
http://stackoverflow.com/questions/320921/how-to-add-a-wix-custom-action-that...
Short answer: Look at the "INSTALLED" property.
I have tested this on Windows XP as well and it worked the same way. I don't think the links you've posted actually explicitly state this can't be done too. The first link only describes what would be good to have, I think. It wouldn't be possible had our CustomAction been scheduled in InstallUISequence - that's for sure. But scheduling it in InstallExecuteSequence changes stakes. > In larger issues, the present change set (even barring UI level issues) doesn't > work, since it's possible for the user to make a series of CA > dialog button > presses that leaves IE running on uninstall. We still need code that checks > whether we're doing install or uninstall and behaves accordingly. If it doesn't work with our custom action there is still "Application in use" dialog box left, which will take care of that situation.
On 2015/03/20 05:13:05, Oleksandr wrote:
> I have tested this on Windows XP as well and it worked the same way.
Tested under XP running an uninstall from the command line ("msiexec /x")
separately with "/qb"?
> I don't think the links you've posted actually explicitly state this can't be
> done too. The first link only describes what would be good to have, I think.
It's a recommendation because there can be, evidently, problems with doing it
otherwise. I don't think the problems were deterministic, but had something to
do with current processes and window messages or somesuch.
Given how arcane Windows Installer is, I cannot agree with violating a very
clear recommendation to avoid doing something that this change set is doing.
> If it doesn't work with our custom action there is still "Application in use"
> dialog box left, which will take care of that situation.
The FilesInUse dialog does not exist before Windows Installer 4.0 (Vista), when
the Reboot Manager was introduced. So you still need to to the right thing with
XP.
But relying on FilesInUse to fix your problems is a bad user experience. It's
tantamount to the same complaint you've made about "partKnown" involved asking
the user twice about something (a critique I agree with but don't know how to
change within the constraints of WI). Here you're saying that it's OK to put up
a second dialog box that asks the same thing they think they just did.
-----
I've realized we have a confusion here. There are two problems being discussed,
and I'm not convinced we've even been talking about the same one each time.
Problem #1. The problem in defect report #1598.
https://issues.adblockplus.org/ticket/1598
This is an uninstall that happens from the control panel. This runs the MSI
with full UI. The present change has no effect when running with full UI. In
that case the UI sequence executes and the UI level is 5. In #msg6 above you
characterized it this way:
> * Uninstall is launched, our custom action is not launched
> * uninstall shows the "Applications in use" dialog.
In fact, our CA _is_ being launched (see "how to reproduce" step 4). What
happened is that the CA did not manage to kill IE, and so the FilesInUse dialog
popped up. The problem here appears to be that our user was confused about what
was happening more than anything else.
Problem #2. Our CA is not running under ordinary upgrade.
We run the MSI with "/qb" during automatic update from within the engine.
That's the only time it runs in Basic UI mode without explicit user
intervention. The only way to get Basic UI level through this kind of command.
Running as "/qb" was a quick hack to suppress the full installation dialog
sequence. It was done to get things out the door quickly at the last minute; it
was not part of an overall UX design plan. Indeed, I have come to consider the
use of "/qb" on upgrade to be something of a defect. The fundamentals are that
we need some kind of UI to gather user preferences about closing IE on upgrade,
and "/qb" means that we don't have a good options in the MSI.
Problem #3. There are circumstances where we may uninstall prematurely and
corrupt IE with a half-uninstalled ABP plugin.
This has arisen in discussion here. This problem can co-occur with problem
#1, but can also appear separately. This problem also needs to be fixed, and it
seems that the fix might not be best done with the current CA.
The present change set only attempts to address problem #2, which is not the
identified issue in the review description. So do we actually have a problem
with the CA not running at all on upgrade? (Seems possible.) If so, what's the
issue #?
If we are trying fix problem #2, we either capture user preferences in the MSI,
which means ditching "/qb", or we can do it in advance while the engine is still
running. If the engine is running, then IE is running, so we know the CA will
run. I anticipated this kind of thing in the current CA code. There's a property
AVOIDREBOOT that can avoid most all of the UI in the CA. (The one difference is
asking for retry in the case it fails to shut down IE.)
There's an additional issue we need to come to terms with. We have different
requirements for behavior depending whether we're installing or upgrading or
uninstalling, and we are not at present distinguishing between them.
---
tl;dr version: This change set causes more problems than it solves, if it even
solves them.
Tested under XP running an uninstall from the command line ("msiexec /x")
> separately with "/qb"?
Of course. Works the same way on Windows Installer 3.0, 4.5 and 5.0. Both during
install and uninstall.
> The FilesInUse dialog does not exist before Windows Installer 4.0 (Vista),
when
> the Reboot Manager was introduced. So you still need to to the right thing
with
> XP.
Even without FilesInUse the uninstall completes without problems. The files are
deleted as soon as IE is closed.
> But relying on FilesInUse to fix your problems is a bad user experience. It's
> tantamount to the same complaint you've made about "partKnown" involved asking
> the user twice about something (a critique I agree with but don't know how to
> change within the constraints of WI). Here you're saying that it's OK to put
up
> a second dialog box that asks the same thing they think they just did.
That's the price the user has to pay for not going the default route. I agree
this is not optimal but to make it optimal we would have to rewrite the whole
logic of the custom action, eliminate the current double question to close IE
from the Custom Action and disallow to keep IE open. Also, the same can happen
right now during the install, and that seems to be working out fine for us. So
I'm fine with not refactoring this.
> Problem #1. The problem in defect report #1598.
> https://issues.adblockplus.org/ticket/1598
> This is an uninstall that happens from the control panel. This runs the
MSI
> with full UI.
No. Uninstall from control panel launches in Basic UI. ie /qb
> > * Uninstall is launched, our custom action is not launched
> > * uninstall shows the "Applications in use" dialog.
> In fact, our CA _is_ being launched (see "how to reproduce" step 4). What
> happened is that the CA did not manage to kill IE, and so the FilesInUse
dialog
> popped up. The problem here appears to be that our user was confused about
what
> was happening more than anything else.
Since uninstall from Control Panel is launched in Basic UI, our custom action is
not launched. What step 4 in "how to reproduce" describes is a FilesInUse
dialog.
> Problem #2. Our CA is not running under ordinary upgrade.
> We run the MSI with "/qb" during automatic update from within the engine.
> That's the only time it runs in Basic UI mode without explicit user
> intervention. The only way to get Basic UI level through this kind of command.
> Running as "/qb" was a quick hack to suppress the full installation dialog
> sequence. It was done to get things out the door quickly at the last minute;
it
> was not part of an overall UX design plan. Indeed, I have come to consider the
> use of "/qb" on upgrade to be something of a defect. The fundamentals are that
> we need some kind of UI to gather user preferences about closing IE on
upgrade,
> and "/qb" means that we don't have a good options in the MSI.
I disagree. /qb let's us eliminate all the unnecessary clicks during the
upgrade. There is no point in showing a bunch of standard installer screens on
each upgrade.
> Problem #3. There are circumstances where we may uninstall prematurely and
> corrupt IE with a half-uninstalled ABP plugin.
> This has arisen in discussion here. This problem can co-occur with problem
> #1, but can also appear separately. This problem also needs to be fixed, and
it
> seems that the fix might not be best done with the current CA.
I don't see how this is related at all. The custom action is running before any
changes are made. Even if it weren't, the incorrect return from the Custom
Action would rollback the installation.
> If we are trying fix problem #2, we either capture user preferences in the
MSI,
> which means ditching "/qb"
It doesn't mean ditching "/qb", based on my tests.
> There's an additional issue we need to come to terms with. We have different
> requirements for behavior depending whether we're installing or upgrading or
> uninstalling, and we are not at present distinguishing between them.
Why would we have different requirements for install and uninstall? The
generalization the IE has to be closed before install/uninstall seems fine to
me.
TD;DR version: There is no evidence this change set causes any problems, and it
solves the problem of the uninstaller not being able to close IE.
On 2015/03/20 19:37:33, Oleksandr wrote: > No. Uninstall from control panel launches in Basic UI. ie /qb That's news to me. And /qb isn't the only way of suppressing the default UI. You can make authored UI in the MSI tables contingent upon conditions. ----- What this change set needs, desperately, is enough documentation and commentary that someone who doesn't know all the intricacies of Windows Installer to be able to audit the code and determine that it's behaving correctly. Such writing would need to examine all the cases where the CA executes, not just the most common ones. We have .dox files available for such writing; it doesn't all need to be in comments. I don't have anything new to say. I remain unconvinced that this code works right. Perhaps if I see enough writing on it I might be persuaded. That hasn't happened yet. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
