Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 6219169376763904: Issue #1686 - Kill iexplore.exe and AdblockplusEngine.exe processes from the installer (Closed)

Created:
March 13, 2015, 12:37 p.m. by Oleksandr
Modified:
June 1, 2015, 9:20 a.m.
Reviewers:
Eric
CC:
sergei
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Wait after everything is closed #

Total comments: 5

Patch Set 3 : Log error #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -9 lines) Patch
M installer/src/custom-action/close_application.cpp View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M installer/src/installer-lib/process.h View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M installer/src/installer-lib/process.cpp View 1 2 2 chunks +16 lines, -4 lines 1 comment Download

Messages

Total messages: 7
Oleksandr
March 13, 2015, 12:39 p.m. (2015-03-13 12:39:05 UTC) #1
Eric
Mostly correct. http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/installer/src/installer-lib/process.cpp File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/installer/src/installer-lib/process.cpp#newcode281 installer/src/installer-lib/process.cpp:281: default : This should be "case 4 ...
March 13, 2015, 4:49 p.m. (2015-03-13 16:49:47 UTC) #2
Oleksandr
March 20, 2015, 5:32 a.m. (2015-03-20 05:32:50 UTC) #3
Eric
The appropriate logging for ShutDown() would be not just for forcible termination, but at each ...
March 20, 2015, 8:53 a.m. (2015-03-20 08:53:16 UTC) #4
sergei
http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/installer/src/installer-lib/process.cpp File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/6219169376763904/diff/5629499534213120/installer/src/installer-lib/process.cpp#newcode304 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/installer/src/installer-lib/process.cpp ...
April 2, 2015, 8:04 a.m. (2015-04-02 08:04:09 UTC) #5
Oleksandr
April 13, 2015, 4:53 a.m. (2015-04-13 04:53:41 UTC) #6
Eric
May 14, 2015, 3:09 p.m. (2015-05-14 15:09:25 UTC) #7
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 = "

Powered by Google App Engine
This is Rietveld