Issue #1686 - Kill iexplore.exe and AdblockplusEngine.exe processes from the installer

Issue #1686 - Kill iexplore.exe and AdblockplusEngine.exe processes from the installer

[Eric, 2015-03-13 16:49:47]

Mostly correct.

http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/inst...
File installer/src/installer-lib/process.cpp (right):

http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:281: default :
This should be "case 4 :" and we should retain the previous "default:". The
reason is that we want to execute the wait loop below to give the system some
time to finish terminating the process.

We don't want a false negative because of a zombie process that exists longer
than normal.

http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:283: * We're out of ways to try to shut
down. Oh well. Take cover. It gets violent here.
Good comment.

http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:288: if (tmpHandle != NULL)
We can use implicit conversion to bool here.

I would use "if (!tmpHandle) continue;", but that's just me.

[Eric, 2015-03-20 08:53:16]

The appropriate logging for ShutDown() would be not just for forcible
termination, but at each stage along the way. We don't have support for that at
present but could add it. ProcessCloser::Refresh() clears the old PID list and
makes a new one from scratch. This is reliable (since it also catches
newly-created processes), but does not itself provide a way to comparing the old
PID list to the new. 

It is perhaps beyond the scope of this change set to fix this deficiency, but if
this remains a problem we should implement it, since logging in the MSI enables
field reports as a whole and a place to record more detail about internal faults
while closing.

http://codereview.adblockplus.org/6219169376763904/diff/5724160613416960/inst...
File installer/src/installer-lib/process.cpp (right):

http://codereview.adblockplus.org/6219169376763904/diff/5724160613416960/inst...
installer/src/installer-lib/process.cpp:283: * We're out of ways to try to shut
down. Oh well. Take cover. It gets violent here.
Should restore the first half of the comment to its original location.

http://codereview.adblockplus.org/6219169376763904/diff/5724160613416960/inst...
installer/src/installer-lib/process.cpp:290: TerminateProcess(tmpHandle, 0);
Given how unreliable it's turning out to be to shut down IE, it would be a good
idea to log the error code associated with a failure of TerminateProcess() here.
That would mean changing the signature of ShutDown() to pass a log instance. Had
I realized what trouble this would be, I would have done that the first time.

The error that would most useful to log here would be absence of the
PROCESS_TERMINATE access right. In practice, that would require running an MSI
as administrator, but if that's the trick to make it work, there's no
information coming out of a failure here that would even provide a clue toward
that.

[sergei, 2015-04-02 08:04:09]

http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/inst...
File installer/src/installer-lib/process.cpp (right):

http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:304: std::this_thread::sleep_for(
std::chrono::milliseconds( 30 ) ) ;
no spaces?

http://codereview.adblockplus.org/6219169376763904/diff/5724160613416960/inst...
File installer/src/installer-lib/process.cpp (right):

http://codereview.adblockplus.org/6219169376763904/diff/5724160613416960/inst...
installer/src/installer-lib/process.cpp:290: TerminateProcess(tmpHandle, 0);
On 2015/03/20 08:53:16, Eric wrote:
> Given how unreliable it's turning out to be to shut down IE, it would be a
good
> idea to log the error code associated with a failure of TerminateProcess()
here.
> That would mean changing the signature of ShutDown() to pass a log instance.
Had
> I realized what trouble this would be, I would have done that the first time.
> 
> The error that would most useful to log here would be absence of the
> PROCESS_TERMINATE access right. In practice, that would require running an MSI
> as administrator, but if that's the trick to make it work, there's no
> information coming out of a failure here that would even provide a clue toward
> that.

Merely want to add that MSI is already configured to run with elevated
privileges and running MSI as Administrator still could be not enough to
terminate the process.

http://codereview.adblockplus.org/6219169376763904/diff/5724160613416960/inst...
installer/src/installer-lib/process.cpp:290: TerminateProcess(tmpHandle, 0);
On 2015/03/20 08:53:16, Eric wrote:
> Given how unreliable it's turning out to be to shut down IE, it would be a
good
> idea to log the error code associated with a failure of TerminateProcess()
here.
> That would mean changing the signature of ShutDown() to pass a log instance.
Had
> I realized what trouble this would be, I would have done that the first time.
> 
> The error that would most useful to log here would be absence of the
> PROCESS_TERMINATE access right. In practice, that would require running an MSI
> as administrator, but if that's the trick to make it work, there's no
> information coming out of a failure here that would even provide a clue toward
> that.

As well as it would be good to log the state to better understand which part
failed to shut down IE.

[Eric, 2015-05-14 15:09:25]

LGTM.

If you want to clarify the log message along the lines I suggested, that would
be fine too.

http://codereview.adblockplus.org/6219169376763904/diff/5724160613416960/inst...
File installer/src/installer-lib/process.cpp (right):

http://codereview.adblockplus.org/6219169376763904/diff/5724160613416960/inst...
installer/src/installer-lib/process.cpp:290: TerminateProcess(tmpHandle, 0);
On 2015/04/02 08:04:09, sergei wrote:
> Merely want to add that MSI is already configured to run with elevated
> privileges

The process 'msiexec.exe' runs with elevated privileges. Custom actions, such as
the present code, run in a separate container process and do not necessarily
have the same privileges.

There are ways of doing that, but we're not using them. Given that the whole
purpose of this CA is to avoid an annoyance (rebooting), it's not clear to me
that forcibly closing IE is worth it; the cure (side-effects of premature
termination of IE) may be worse than the disease (the annoyance of rebooting).

http://codereview.adblockplus.org/6219169376763904/diff/5750085036015616/inst...
File installer/src/installer-lib/process.cpp (right):

http://codereview.adblockplus.org/6219169376763904/diff/5750085036015616/inst...
installer/src/installer-lib/process.cpp:291: stream << "Can't open process for
termination. Error: " << GetLastError();
This message should indicate a warning rather than an error. This avoids
confusing reporting with respect to a race condition.

The issue is that we are working from a process snapshot in the past, not the
perfectly-current process set. During the interval between then and now, the IE
process may have closed itself down as previously instructed. As a result, a
failure of 'OpenProcess' does not in itself reflect whether we have a process
that terminated itself and is now gone (not an error, just the effect of the
race condition), and a hard error from a process that's still present that we
can't open.

It would be possible to add more checks to distinguish these two, but I'm not
sure it's worth it.

A suggestion:
"Did not open a process scheduled for termination; it may or may not still
exist. OpenProcess() error code = "