Upgrade the windows specific version of LaunchProcess to avoid raw handles.

Upgrade the windows specific version of LaunchProcess to avoid raw handles.

This change implies that extensions::LaunchNativeProcess also changes to
return base::Process, and that requires base::EnsureProcessTerminated to
deal with base:Process (as it basically claims ownership of the process).

This CL fixes some leaks all around.

BUG=417532
Committed: https://crrev.com/6b687a5e232c80539772dc3dbe35b98095064c38
Cr-Commit-Position: refs/heads/master@{#306687}

Committed: https://crrev.com/61812774784a9ad2e874e737ebc1b6507da314e2
Cr-Commit-Position: refs/heads/master@{#306963}

[rvargas (doing something else), 2014-11-26 02:28:11]

rvargas@chromium.org changed reviewers:
+ cpu@chromium.org, finnur@chromium.org, gab@chromium.org

[rvargas (doing something else), 2014-11-26 02:28:12]

finnur -> extensions
gab -> installer
cpu -> everything else

[grt (UTC plus 2), 2014-11-26 16:14:14]

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:154:
base::Process cmd_process = base::LaunchProcess(command.c_str(), options);
Gab discovered in crbug.com/436072 that this launch is problematic because the
launched process's CWD is Chrome's version directory. Since the spawned process
keeps that handle open for its lifetime, it interferes with Chrome updates and
uninstalls. Could you make this set the launched proc's CWD to
command_line.GetProgram().DirName() (or anything other than Chrome's CWD)? It
looks like it'll take plumbing in launch{.h,_win.cc} to make this possible, so
it's probably best to do it in its own CL. Thanks.

[rvargas (doing something else), 2014-11-26 18:16:20]

Thanks

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:154:
base::Process cmd_process = base::LaunchProcess(command.c_str(), options);
On 2014/11/26 16:14:13, grt wrote:
> Gab discovered in crbug.com/436072 that this launch is problematic because the
> launched process's CWD is Chrome's version directory. Since the spawned
process
> keeps that handle open for its lifetime, it interferes with Chrome updates and
> uninstalls. Could you make this set the launched proc's CWD to
> command_line.GetProgram().DirName() (or anything other than Chrome's CWD)? It
> looks like it'll take plumbing in launch{.h,_win.cc} to make this possible, so
> it's probably best to do it in its own CL. Thanks.

That is certainly interesting, but that seems out of scope of the current change
(and the bug it is trying to fix)

[grt (UTC plus 2), 2014-11-26 19:40:07]

On 2014/11/26 18:16:20, rvargas wrote:
> Thanks
> 
>
https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
> (right):
> 
>
https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:154:
> base::Process cmd_process = base::LaunchProcess(command.c_str(), options);
> On 2014/11/26 16:14:13, grt wrote:
> > Gab discovered in crbug.com/436072 that this launch is problematic because
the
> > launched process's CWD is Chrome's version directory. Since the spawned
> process
> > keeps that handle open for its lifetime, it interferes with Chrome updates
and
> > uninstalls. Could you make this set the launched proc's CWD to
> > command_line.GetProgram().DirName() (or anything other than Chrome's CWD)?
It
> > looks like it'll take plumbing in launch{.h,_win.cc} to make this possible,
so
> > it's probably best to do it in its own CL. Thanks.
> 
> That is certainly interesting, but that seems out of scope of the current
change
> (and the bug it is trying to fix)

Agreed, hence the "Could you...?" and "...in its own CL." :-)

[rvargas (doing something else), 2014-11-27 00:51:59]

On 2014/11/26 19:40:07, grt wrote:
> On 2014/11/26 18:16:20, rvargas wrote:
> > Thanks
> > 
> >
>
https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
> > File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
> > chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:154:
> > base::Process cmd_process = base::LaunchProcess(command.c_str(), options);
> > On 2014/11/26 16:14:13, grt wrote:
> > > Gab discovered in crbug.com/436072 that this launch is problematic because
> the
> > > launched process's CWD is Chrome's version directory. Since the spawned
> > process
> > > keeps that handle open for its lifetime, it interferes with Chrome updates
> and
> > > uninstalls. Could you make this set the launched proc's CWD to
> > > command_line.GetProgram().DirName() (or anything other than Chrome's CWD)?
> It
> > > looks like it'll take plumbing in launch{.h,_win.cc} to make this
possible,
> so
> > > it's probably best to do it in its own CL. Thanks.
> > 
> > That is certainly interesting, but that seems out of scope of the current
> change
> > (and the bug it is trying to fix)
> 
> Agreed, hence the "Could you...?" and "...in its own CL." :-)

I got that. I guess I was trying to avoid saying that I don't think I'm going to
have time for that.

[gab, 2014-11-27 13:28:45]

installer lgtm + comments/questions in other parts as I read all of this most
interesting CL :-)!

Cheers,
Gab

https://codereview.chromium.org/759903002/diff/1/base/process/kill_win.cc
File base/process/kill_win.cc (right):

https://codereview.chromium.org/759903002/diff/1/base/process/kill_win.cc#new...
base/process/kill_win.cc:53: Process process_;
If I'm reading this correctly, the changes in this file now make it such that
the HANDLE won't be closed until TimerExpiredTask::TimedOut() kicks in as it is
that task that owns the TimerExpiredTask object [1] (which now in turn owns the
HANDLE through process_).

Should we have some kind of explicit process_.release() which would both release
its handle and ensure that |!process_.IsValid()|?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/process/kill_...

https://codereview.chromium.org/759903002/diff/1/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/759903002/diff/1/base/process/launch_win.cc#n...
base/process/launch_win.cc:239: win::ScopedHandle process;
s/process/process_handle
(for naming consistency with everything else in this file and this CL)

https://codereview.chromium.org/759903002/diff/1/base/process/launch_win.cc#n...
base/process/launch_win.cc:246: bool LaunchProcess(const CommandLine& cmdline,
Is this still needed? (and similarly should that definition be ifdef'ed out in
the header on OS_WIN?)

Or are we not there yet? Perhaps this is actually what you meant by the comment
on line 235, sorry about the noise if that's the case :-)!

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:360:
base::EnsureProcessTerminated(process_.Pass());
Is it possible that this fixes http://crbug.com/436072 ? (i.e., maybe the
process was kept alive simply because there was a leaked handle opened to it
even though the process itself had been terminated? or do terminated processes
really get terminated regardless and opened handles just keep the process info
alive, not the process itself?)

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_process_launcher.h (right):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_process_launcher.h:71:
base::Process* process_handle,
s/process_handle/process

[Finnur, 2014-11-27 13:58:10]

Extensions rubberstamp LGTM.

[Finnur, 2014-11-27 13:58:26]

... With one question:

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(left):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:362:
process_handle_ = base::kNullProcessHandle;
Is it possible that this is being nulled to prevent EnsureProcessTerminated
being called twice on Mac if code runs twice? Is that benign perhaps?

[gab, 2014-11-27 14:29:51]

Replying to finnur inline.

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(left):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:362:
process_handle_ = base::kNullProcessHandle;
On 2014/11/27 13:58:26, Finnur wrote:
> Is it possible that this is being nulled to prevent EnsureProcessTerminated
> being called twice on Mac if code runs twice? Is that benign perhaps?

By doing process_.Pass() above, I'd assume |process_| itself becomes invalid,
which provides the same safety guard as before on line 351, no?

[Finnur, 2014-11-27 16:41:59]

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(left):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:362:
process_handle_ = base::kNullProcessHandle;
Haven't used Pass() all that much, but if that's how it works then that's fine.

[gab, 2014-11-27 16:47:39]

NP, explanation below.

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(left):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:362:
process_handle_ = base::kNullProcessHandle;
On 2014/11/27 16:41:59, Finnur wrote:
> Haven't used Pass() all that much, but if that's how it works then that's
fine. 

Process is a move-only type and .Pass() here invokes the move-only constructor
which results in the parameter taking the ScopedHandle from |process_| @ [1] --
and the handle is thus closed the minute the parameter goes out of scope.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce...

[rvargas (doing something else), 2014-12-01 20:50:36]

Thanks for all the comments.

Updated a new version.

https://codereview.chromium.org/759903002/diff/1/base/process/kill_win.cc
File base/process/kill_win.cc (right):

https://codereview.chromium.org/759903002/diff/1/base/process/kill_win.cc#new...
base/process/kill_win.cc:53: Process process_;
On 2014/11/27 13:28:44, gab wrote:
> If I'm reading this correctly, the changes in this file now make it such that
> the HANDLE won't be closed until TimerExpiredTask::TimedOut() kicks in as it
is
> that task that owns the TimerExpiredTask object [1] (which now in turn owns
the
> HANDLE through process_).
> 
> Should we have some kind of explicit process_.release() which would both
release
> its handle and ensure that |!process_.IsValid()|?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/process/kill_...

You're right, there is no need to extend the handle lifetime beyond
OnObjectSignaled, so I could call process_.Close() there. I don't think there's
any danger in keeping the process object around for a while after it the process
dies as all resources should be released at exit time (other than the
handle/object).

I've been resisting to add a release method to Process (I'm assuming you mean a
way to grab the handle from the object) as that's easy to misuse.

I'm adding Close() back, let me know if that doesn't address your concern.

https://codereview.chromium.org/759903002/diff/1/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/759903002/diff/1/base/process/launch_win.cc#n...
base/process/launch_win.cc:239: win::ScopedHandle process;
On 2014/11/27 13:28:44, gab wrote:
> s/process/process_handle
> (for naming consistency with everything else in this file and this CL)

Done.

https://codereview.chromium.org/759903002/diff/1/base/process/launch_win.cc#n...
base/process/launch_win.cc:246: bool LaunchProcess(const CommandLine& cmdline,
On 2014/11/27 13:28:44, gab wrote:
> Is this still needed? (and similarly should that definition be ifdef'ed out in
> the header on OS_WIN?)
> 
> Or are we not there yet? Perhaps this is actually what you meant by the
comment
> on line 235, sorry about the noise if that's the case :-)!

Yeah, this function is what prevents me from switching right away the contract
of the win-specific override :(. And this will take multiple CLs as this version
is used everywhere.

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/759903002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:360:
base::EnsureProcessTerminated(process_.Pass());
On 2014/11/27 13:28:44, gab wrote:
> Is it possible that this fixes http://crbug.com/436072 ? (i.e., maybe the
> process was kept alive simply because there was a leaked handle opened to it
> even though the process itself had been terminated? or do terminated processes
> really get terminated regardless and opened handles just keep the process info
> alive, not the process itself?)

A handle should only keep alive the process object (in the kernel) so that the
exit code is retrievable, but all resources required for the process itself
should be released at exit time.

So I don't think that leaks fixed by this CL (as it does) will solve that bug.

[gab, 2014-12-01 21:31:05]

lgtm++

https://codereview.chromium.org/759903002/diff/1/base/process/kill_win.cc
File base/process/kill_win.cc (right):

https://codereview.chromium.org/759903002/diff/1/base/process/kill_win.cc#new...
base/process/kill_win.cc:53: Process process_;
On 2014/12/01 20:50:36, rvargas wrote:
> On 2014/11/27 13:28:44, gab wrote:
> > If I'm reading this correctly, the changes in this file now make it such
that
> > the HANDLE won't be closed until TimerExpiredTask::TimedOut() kicks in as it
> is
> > that task that owns the TimerExpiredTask object [1] (which now in turn owns
> the
> > HANDLE through process_).
> > 
> > Should we have some kind of explicit process_.release() which would both
> release
> > its handle and ensure that |!process_.IsValid()|?
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/process/kill_...
> 
> You're right, there is no need to extend the handle lifetime beyond
> OnObjectSignaled, so I could call process_.Close() there. I don't think
there's
> any danger in keeping the process object around for a while after it the
process
> dies as all resources should be released at exit time (other than the
> handle/object).
> 
> I've been resisting to add a release method to Process (I'm assuming you mean
a
> way to grab the handle from the object) as that's easy to misuse.

Indeed, I meant reset(), not release() (my bad) -- i.e. a way to clear the
state, not to obtain it.

But Handle() already provides a way to obtain the raw state, so isn't your
concern already in place? Or do you think it's fine in that case because
ownership of the handle is maintained and as long as the caller respects the
implicit contract it works fine (just like scoped_ptr::get().

> 
> I'm adding Close() back, let me know if that doesn't address your concern.

Close() fits this perfectly.

[cpu_(ooo_6.6-7.5), 2014-12-01 22:36:13]

lgtm  with one question.

Second I think you need owners for remoting and for UI.

https://codereview.chromium.org/759903002/diff/20001/components/storage_monit...
File components/storage_monitor/storage_monitor_linux.cc (right):

https://codereview.chromium.org/759903002/diff/20001/components/storage_monit...
components/storage_monitor/storage_monitor_linux.cc:218: if
(!base::LaunchProcess(command, options, &handle))
can't we port 218 already?

[cpu_(ooo_6.6-7.5), 2014-12-01 22:37:35]

nevermind on the UI file, that file lgtm

[rvargas (doing something else), 2014-12-01 23:02:54]

rvargas@chromium.org changed reviewers:
+ wez@chromium.org

[rvargas (doing something else), 2014-12-01 23:02:54]

Thanks.

+wez for remoting.

https://codereview.chromium.org/759903002/diff/20001/components/storage_monit...
File components/storage_monitor/storage_monitor_linux.cc (right):

https://codereview.chromium.org/759903002/diff/20001/components/storage_monit...
components/storage_monitor/storage_monitor_linux.cc:218: if
(!base::LaunchProcess(command, options, &handle))
On 2014/12/01 22:36:13, cpu wrote:
> can't we port 218 already?

Nope. This is Linux code and I have not started porting that version yet.

[rvargas (doing something else), 2014-12-02 21:07:47]

I forgot to reply to this comment.

https://codereview.chromium.org/759903002/diff/1/base/process/kill_win.cc
File base/process/kill_win.cc (right):

https://codereview.chromium.org/759903002/diff/1/base/process/kill_win.cc#new...
base/process/kill_win.cc:53: Process process_;
On 2014/12/01 21:31:05, gab wrote:
> But Handle() already provides a way to obtain the raw state, so isn't your
> concern already in place? Or do you think it's fine in that case because
> ownership of the handle is maintained and as long as the caller respects the
> implicit contract it works fine (just like scoped_ptr::get().

It would be better to not have a Handle method at all, but that's kind of hard
to do. At least the interface there implies that there is no transfer of
ownership and storing that handle is either dangerous or plain wrong.

[rvargas (doing something else), 2014-12-03 20:25:03]

rvargas@chromium.org changed reviewers:
+ sergeyu@chromium.org
- wez@chromium.org

[rvargas (doing something else), 2014-12-03 20:25:04]

+sergeyu for remoting

[Wez, 2014-12-03 20:33:58]

wez@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2014-12-03 20:33:58]

remoting/ LGTM

[rvargas (doing something else), 2014-12-03 21:05:55]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-12-03 21:06:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759903002/40001

[commit-bot: I haz the power, 2014-12-03 22:24:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-03 22:25:14]

Patchset 3 (id:??) landed as
https://crrev.com/6b687a5e232c80539772dc3dbe35b98095064c38
Cr-Commit-Position: refs/heads/master@{#306687}

[Rune Fevang, 2014-12-03 23:41:24]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/780653003/ by rfevang@chromium.org.

The reason for reverting is: Looks like it broke the 'Google Chrome Win'
builder:

FAILED: ninja -t msvc -e environment.x86 -- c:\b\build\goma/gomacc
"c:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC
@obj\win8\delegate_execute\delegate_execute.chrome_util.obj.rsp /c
..\..\win8\delegate_execute\chrome_util.cc
/Foobj\win8\delegate_execute\delegate_execute.chrome_util.obj
/Fdobj\win8\delegate_execute\delegate_execute.cc.pdb 
c:\b\build\slave\google-chrome-rel-win\build\src\win8\delegate_execute\chrome_util.cc(119)
: error C2665: 'base::LaunchProcess' : none of the 3 overloads could convert all
the argument types
       
c:\b\build\slave\google-chrome-rel-win\build\src\base\process\launch.h(161):
could be 'bool base::LaunchProcess(const base::CommandLine &,const
base::LaunchOptions &,base::ProcessHandle *)'
        while trying to match the argument list '(base::string16,
base::LaunchOptions, base::win::ScopedHandle *)'.

[rvargas (doing something else), 2014-12-04 02:28:50]

cpu, do you mind looking at the change to win8/delegate_execute/chrome_util.cc ?

[cpu_(ooo_6.6-7.5), 2014-12-04 23:55:07]

lgtm

[rvargas (doing something else), 2014-12-05 01:26:09]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-12-05 01:27:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759903002/60001

[commit-bot: I haz the power, 2014-12-05 03:15:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-05 03:15:53]

Patchset 4 (id:??) landed as
https://crrev.com/61812774784a9ad2e874e737ebc1b6507da314e2
Cr-Commit-Position: refs/heads/master@{#306963}