Fix the windows implementation of KillProcess and WaitForSingleProcess to not...

Fix the windows implementation of KillProcess and WaitForSingleProcess to not close the process handle that they do not own.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9400

[tommi (sloooow) - chröme, 2009-02-06 22:56:03]

lgtm

(btw, I didn't see a reviewer list)

http://codereview.chromium.org/24004/diff/2001/3002
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/24004/diff/2001/3002#newcode69
Line 69: base::CloseProcessHandle(handle);
if CloseProcessHandle returns a value then maybe it's a good idea to use an
EXPECT check on those to catch bugs like when functions close the handle when
they shouldn't

[TVL, 2009-02-06 22:56:25]

http://codereview.chromium.org/24004/diff/2001/3005
File base/process_util_win.cc (right):

http://codereview.chromium.org/24004/diff/2001/3005#newcode176
Line 176: bool result = !!TerminateProcess(process, exit_code);
while you're here, should the "!!" be cleaned up?

[M-A Ruel, 2009-02-07 20:54:39]

Sorry for the delay. :(

http://codereview.chromium.org/24004/diff/2009/3028
File base/process_util.h (right):

http://codereview.chromium.org/24004/diff/2009/3028#newcode156
Line 156: bool KillProcess(DWORD process_id, int exit_code, bool wait);
Function overloading is disapproved by style. Even if the code was already
faulty before your change, please rename the specialization.

http://codereview.chromium.org/24004/diff/2009/3027
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/24004/diff/2009/3027#newcode221
Line 221: CloseProcessHandle(handle);
Why do you specify base:: in the previous calls but not there?

[stoyan, 2009-02-09 18:04:06]

http://codereview.chromium.org/24004/diff/2009/3028
File base/process_util.h (right):

http://codereview.chromium.org/24004/diff/2009/3028#newcode156
Line 156: bool KillProcess(DWORD process_id, int exit_code, bool wait);
On 2009/02/07 20:54:39, M-A wrote:
> Function overloading is disapproved by style. Even if the code was already
> faulty before your change, please rename the specialization.

Done.

http://codereview.chromium.org/24004/diff/2009/3027
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/24004/diff/2009/3027#newcode221
Line 221: CloseProcessHandle(handle);
On 2009/02/07 20:54:39, M-A wrote:
> Why do you specify base:: in the previous calls but not there?

Done.

[M-A Ruel, 2009-02-09 18:31:56]

lgtm with the 80 cols fix. I added 2 nits, it's up to you. No need to re-upload.

http://codereview.chromium.org/24004/diff/3037/3042
File base/process_util_win.cc (right):

http://codereview.chromium.org/24004/diff/3037/3042#newcode257
Line 257: const ProcessFilter* filter) :
nit: fix the alignment there while at it. It's sooo wrong.

http://codereview.chromium.org/24004/diff/3037/3042#newcode321
Line 321: result = KillProcessById((*entry).th32ProcessID, exit_code, true) &&
result;
nit: That code seems like an extract from C++ for dummies. Can you rewrite:
while (ProcessEntry* entry = iter.NextProcessEntry())
  result &= KillProcessById(entry->th32ProcessID, exit_code, true);

http://codereview.chromium.org/24004/diff/3037/3046
File chrome/browser/metrics/metrics_service_uitest.cc (right):

http://codereview.chromium.org/24004/diff/3037/3046#newcode94
Line 94: base::KillProcessById(process_id, 0xc0000005, true);  // Fake Access
Violation.
80 cols, move the comment on its own line above.

[stoyan, 2009-02-09 18:44:37]

http://codereview.chromium.org/24004/diff/3037/3042
File base/process_util_win.cc (right):

http://codereview.chromium.org/24004/diff/3037/3042#newcode257
Line 257: const ProcessFilter* filter) :
On 2009/02/09 18:31:56, M-A wrote:
> nit: fix the alignment there while at it. It's sooo wrong.

Done.

http://codereview.chromium.org/24004/diff/3037/3042#newcode321
Line 321: result = KillProcessById((*entry).th32ProcessID, exit_code, true) &&
result;
On 2009/02/09 18:31:56, M-A wrote:
> nit: That code seems like an extract from C++ for dummies. Can you rewrite:
> while (ProcessEntry* entry = iter.NextProcessEntry())
>   result &= KillProcessById(entry->th32ProcessID, exit_code, true);
> 

This is not the same.
We have to kill all the processes (or at least try). If we failed on one, no
attempt will be made to kill the others.
Rewritten as: if(!KillProcess()) result = false; which seems more natural to me.

http://codereview.chromium.org/24004/diff/3037/3046
File chrome/browser/metrics/metrics_service_uitest.cc (right):

http://codereview.chromium.org/24004/diff/3037/3046#newcode94
Line 94: base::KillProcessById(process_id, 0xc0000005, true);  // Fake Access
Violation.
On 2009/02/09 18:31:56, M-A wrote:
> 80 cols, move the comment on its own line above.

Done.

[M-A Ruel, 2009-02-09 18:50:17]

lgtm if you remove the if (!Kill...). I think it should continue killing as much
as it can.

http://codereview.chromium.org/24004/diff/2028/3061
File base/process_util_win.cc (right):

http://codereview.chromium.org/24004/diff/2028/3061#newcode258
Line 258: started_iteration_(false),
Actually the : must be on this line and at +4.

http://codereview.chromium.org/24004/diff/2028/3061#newcode321
Line 321: if (!KillProcessById((*entry).th32ProcessID, exit_code, true))
I disagree, we should kill as much as we can.

[stoyan, 2009-02-09 18:59:42]

http://codereview.chromium.org/24004/diff/2028/3061
File base/process_util_win.cc (right):

http://codereview.chromium.org/24004/diff/2028/3061#newcode258
Line 258: started_iteration_(false),
On 2009/02/09 18:50:17, M-A wrote:
> Actually the : must be on this line and at +4.

Done.

http://codereview.chromium.org/24004/diff/2028/3061#newcode321
Line 321: if (!KillProcessById((*entry).th32ProcessID, exit_code, true))
On 2009/02/09 18:50:17, M-A wrote:
> I disagree, we should kill as much as we can.

result does not affect loop exiting.

[M-A Ruel, 2009-02-09 19:02:50]

On 2009/02/09 18:59:42, stoyan wrote:
> http://codereview.chromium.org/24004/diff/2028/3061#newcode321
> Line 321: if (!KillProcessById((*entry).th32ProcessID, exit_code, true))
> On 2009/02/09 18:50:17, M-A wrote:
> > I disagree, we should kill as much as we can.
> 
> result does not affect loop exiting.

Uh, I don't know why I misread your code. Sorry. LGTM.

M-A

[tommi (sloooow) - chröme, 2009-02-09 19:42:50]

http://codereview.chromium.org/24004/diff/3037/3042
File base/process_util_win.cc (right):

http://codereview.chromium.org/24004/diff/3037/3042#newcode321
Line 321: result = KillProcessById((*entry).th32ProcessID, exit_code, true) &&
result;
> "If we failed on one, no attempt will be made to kill the others."

Are you sure that's true?

I don't think you're short-circuiting correctly here since you evaluate 'result'
second and not first.

In this case I think you always try to kill every process, although the return
value will be false if one of them failed.