Use TaskScheduler in EnsureProcessTerminated().

Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with PostDelayedTaskWithTraits. This allows
EnsureProcessTerminated to be called from any context.

This CL also removes usage of base::win::ObjectWatcher from
EnsureProcessTerminated(). Before, the process handle was closed
as soon as the process exited. Now, the process is terminated
(if need be) and the handle is closed only when the timeout
expires.

BUG=675631
Review-Url: https://codereview.chromium.org/2598483004
Cr-Commit-Position: refs/heads/master@{#442576}
Committed: https://chromium.googlesource.com/chromium/src/+/c407aeeec23b49178d20068a39a762e2a6599537

[fdoray, 2016-12-21 19:50:48]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 19:51:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 21:00:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 21:00:26]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-12-22 13:03:48]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org

[fdoray, 2016-12-22 13:03:48]

PTAL

AFAICT, this is only used on the process launcher thread [1],  file thread [2],
and IO thread [3] i.e. after TaskScheduler initialization.

[1]
https://cs.chromium.org/chromium/src/content/browser/child_process_launcher.c...
[2]
https://cs.chromium.org/chromium/src/components/storage_monitor/storage_monit...
[3]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/messaging/...

[fdoray, 2016-12-22 13:04:55]

Description was changed from

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with CreateTaskRunnerWithTraits. This allows
EnsureProcessTerminated to be called from any type of task.

BUG=675631
==========

to

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with CreateTaskRunnerWithTraits. This allows
EnsureProcessTerminated to be called from any kind of task.

BUG=675631
==========

[fdoray, 2016-12-22 13:05:07]

Description was changed from

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with CreateTaskRunnerWithTraits. This allows
EnsureProcessTerminated to be called from any kind of task.

BUG=675631
==========

to

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with CreateTaskRunnerWithTraits. This allows
EnsureProcessTerminated to be called from anywhere.

BUG=675631
==========

[gab, 2016-12-22 20:23:02]

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

https://codereview.chromium.org/2598483004/diff/1/base/process/kill_win.cc#ne...
base/process/kill_win.cc:202: CreateTaskRunnerWithTraits(
Should we add PostDelayedTaskWithTraits()?

[fdoray, 2017-01-06 17:36:01]

Description was changed from

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with CreateTaskRunnerWithTraits. This allows
EnsureProcessTerminated to be called from anywhere.

BUG=675631
==========

to

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with PostDelayedTaskWithTraits. This allows
EnsureProcessTerminated to be called from any context.

BUG=675631
==========

[fdoray, 2017-01-06 17:36:48]

PTAnL

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

https://codereview.chromium.org/2598483004/diff/1/base/process/kill_win.cc#ne...
base/process/kill_win.cc:202: CreateTaskRunnerWithTraits(
On 2016/12/22 20:23:02, gab wrote:
> Should we add PostDelayedTaskWithTraits()?

Done.

[gab, 2017-01-09 17:36:38]

https://codereview.chromium.org/2598483004/diff/20001/base/process/kill_win.cc
File base/process/kill_win.cc (left):

https://codereview.chromium.org/2598483004/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:205: Owned(new TimerExpiredTask(std::move(process)))),
TimerExpiredTask::watcher_::task_runner_ is set to
SequencedTaskRunnerHandle::Get() and watcher_ calls things on that.

TimerExpiredTask isn't thread-safe so previously implicitly depended on that
being the same as the ThreadTaskRunnerHandle::Get() this task was posted to in
practice...

The simplest thing is probably to just use SequencedTaskRunnerHandle::Get() to
post this task (i.e. instead of modifying ObjectWatcher to allow specifying a
watch sequence)?

Another option since no one observes the notification anyways is to simplify
TimerExpiredTask into a single inline lambda that does 
[] (Process process) { if (WaitForSingleObject(process.Handle(), 0) ==
WAIT_OBJECT_0) return; process.Terminate(...); }

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

https://codereview.chromium.org/2598483004/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:208: Owned(new TimerExpiredTask(std::move(process)))),
Passed(MakeUnique(...)) instead of Owned(new ...) while you're here.

[fdoray, 2017-01-09 21:16:10]

Description was changed from

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with PostDelayedTaskWithTraits. This allows
EnsureProcessTerminated to be called from any context.

BUG=675631
==========

to

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with PostDelayedTaskWithTraits. This allows
EnsureProcessTerminated to be called from any context.

This CL also removes usage of base::win::ObjectWatcher from
EnsureProcessTerminated(). Before, the process handle was closed
as soon as the process exited. Now, the process is terminated
(if need be) and the handle is closed only when the timeout
expires.

BUG=675631
==========

[fdoray, 2017-01-09 21:16:49]

PTAnL

https://codereview.chromium.org/2598483004/diff/20001/base/process/kill_win.cc
File base/process/kill_win.cc (left):

https://codereview.chromium.org/2598483004/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:205: Owned(new TimerExpiredTask(std::move(process)))),
On 2017/01/09 17:36:38, gab wrote:
> TimerExpiredTask::watcher_::task_runner_ is set to
> SequencedTaskRunnerHandle::Get() and watcher_ calls things on that.
> 
> TimerExpiredTask isn't thread-safe so previously implicitly depended on that
> being the same as the ThreadTaskRunnerHandle::Get() this task was posted to in
> practice...
> 
> The simplest thing is probably to just use SequencedTaskRunnerHandle::Get() to
> post this task (i.e. instead of modifying ObjectWatcher to allow specifying a
> watch sequence)?
> 
> Another option since no one observes the notification anyways is to simplify
> TimerExpiredTask into a single inline lambda that does 
> [] (Process process) { if (WaitForSingleObject(process.Handle(), 0) ==
> WAIT_OBJECT_0) return; process.Terminate(...); }

Done.

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

https://codereview.chromium.org/2598483004/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:208: Owned(new TimerExpiredTask(std::move(process)))),
On 2017/01/09 17:36:38, gab wrote:
> Passed(MakeUnique(...)) instead of Owned(new ...) while you're here.

n/a

[gab, 2017-01-09 21:27:10]

lgtm, hadn't realized when I proposed it that not using the watcher would mean
keeping the handle open, but AFAIK that merely means the kernel won't delete the
dead process' object which really isn't a big deal for 2 seconds IMO.

CC grt as base/win owner in case he thinks otherwise

[fdoray, 2017-01-09 21:29:56]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-01-09 21:30:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 22:14:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 22:14:33]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[grt (UTC plus 2), 2017-01-10 12:32:13]

On 2017/01/09 21:27:10, gab wrote:
> lgtm, hadn't realized when I proposed it that not using the watcher would mean
> keeping the handle open, but AFAIK that merely means the kernel won't delete
the
> dead process' object which really isn't a big deal for 2 seconds IMO.
> 
> CC grt as base/win owner in case he thinks otherwise

I think it's okay; wfh?

By the way, it looks like NativeMessageProcessHost::~NativeMessageProcessHost
might be holding it wrong -- EnsureProcessTerminated is supposed to be called
after a normal attempt to get rid of a process. Is there some non-obvious place
where NMPH would have told the child proc to go away before calling EPT?  Are
there any other callers of this function on Windows (others I see look like
they're in POSIX/OSX code)?

[fdoray, 2017-01-10 13:02:10]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-10 13:02:10]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2598483004/#ps60001
(title: "include")

[commit-bot: I haz the power, 2017-01-10 13:02:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 14:06:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484053330327600,
"parent_rev": "21b5a9c16d1c0936c4743270d999823cda28593c", "commit_rev":
"c407aeeec23b49178d20068a39a762e2a6599537"}

[commit-bot: I haz the power, 2017-01-10 14:07:00]

Description was changed from

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with PostDelayedTaskWithTraits. This allows
EnsureProcessTerminated to be called from any context.

This CL also removes usage of base::win::ObjectWatcher from
EnsureProcessTerminated(). Before, the process handle was closed
as soon as the process exited. Now, the process is terminated
(if need be) and the handle is closed only when the timeout
expires.

BUG=675631
==========

to

==========
Use TaskScheduler in EnsureProcessTerminated().

Before this CL, EnsureProcessTerminated() posted a task to the
current thread using ThreadTaskRunnerHandle to terminate a process
after a timeout. Unfortunately, ThreadTaskRunnerHandle doesn't work
when used from sequenced or parallel tasks.

Since there is no reason to terminate the process on the thread
from which EnsureProcessTerminated() is called, this CL replaces
ThreadTaskRunnerHandle with PostDelayedTaskWithTraits. This allows
EnsureProcessTerminated to be called from any context.

This CL also removes usage of base::win::ObjectWatcher from
EnsureProcessTerminated(). Before, the process handle was closed
as soon as the process exited. Now, the process is terminated
(if need be) and the handle is closed only when the timeout
expires.

BUG=675631

Review-Url: https://codereview.chromium.org/2598483004
Cr-Commit-Position: refs/heads/master@{#442576}
Committed:
https://chromium.googlesource.com/chromium/src/+/c407aeeec23b49178d20068a39a7...
==========

[commit-bot: I haz the power, 2017-01-10 14:07:01]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c407aeeec23b49178d20068a39a7...

[Will Harris, 2017-01-10 16:32:59]

lgtm - but the notification was removed, was this because it wasn't being used?
I couldn't find any users of it.