The worker process launcher can now ask the worker to crash.

The worker process launcher can now ask the worker process to crash.

When the worker process launcher detects or is notified of a protocol violation:
- It immediately stops processing IPCs from the worker, and requests that it crash.
- The worker process may respect the request to crash, producing a dump for debugging.
- If the worker ignores the request then it will be killed after a grace period has elapsed.

BUG=179215
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187055

[alexeypa (please no reviews), 2013-03-06 21:02:42]

PTAL.

[Wez, 2013-03-07 01:38:35]

On 2013/03/06 21:02:42, alexeypa wrote:
> PTAL.

nit: The CL description has some typos. It may also read more clearly as a set
of bullet-point properties, i.e:

When the worker process launcher detects or is notified of a protocol violation:
- It immediately stops processing IPCs from the worker, and requests that it
crash.
- The worker process may respect the request to crash, producing a dump for
debugging.
- If the worker ignores the request then it will be killed after a grace period
has elapsed.

[Wez, 2013-03-07 01:54:34]

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:49: const int kDelayTimeoutSeconds
= 5;
nit: Naming.  e.g. kCrashRequestTimeoutSeconds?

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:49: const int kDelayTimeoutSeconds
= 5;
5s is probably too short for many systems, BTW.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:98: static void Destruct(const
Core* core);
We use an explicit separate struct e.g. CoreTraits to achieve this in other
places.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:130: base::OneShotTimer<Core>
delay_timer_;
nit: Naming.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:210: &Core::StopWorker);
StopWorker "stops the worker asynchronously". Is it tought enough for our needs?
 Should it be KillWorker? :)

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:78: // Asks the worker process to
crash generating a dump and closes
nit: "... crash and generate a dump, and ..."

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:80: // to the generated dump.
Restarts the worker process forcefully, if it does
nit: "... it is on the stack in the dump."

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:80: // to the generated dump.
Restarts the worker process forcefully, if it does
nit: Does it always restart it, or just kill it and let higher-level code
restart it if need be?

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher_unittest.cc:150: // Receives mesages
sent to the worker process.
typo

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher_unittest.cc:474: // comply.
How - are we relying on the timeout? i.e. will this test always take 5s?

[alexeypa (please no reviews), 2013-03-07 21:28:30]

PTAL.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:49: const int kDelayTimeoutSeconds
= 5;
On 2013/03/07 01:54:35, Wez wrote:
> nit: Naming.  e.g. kCrashRequestTimeoutSeconds?

Done.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:49: const int kDelayTimeoutSeconds
= 5;
On 2013/03/07 01:54:35, Wez wrote:
> 5s is probably too short for many systems, BTW.

It is enough because the timeout is, strictly speaking, optional. We can kill
the process at the spot. It is just waiting a little bit can give us a bit of
useful information.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:98: static void Destruct(const
Core* core);
On 2013/03/07 01:54:35, Wez wrote:
> We use an explicit separate struct e.g. CoreTraits to achieve this in other
> places.

WorkerProcessLauncher::Core is a private class declared in
WorkerProcessLauncher. Having a separate Traits structure will pollute
WorkerProcessLauncher with unnecessary "friend" declarations.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:130: base::OneShotTimer<Core>
delay_timer_;
On 2013/03/07 01:54:35, Wez wrote:
> nit: Naming.

Done.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:210: &Core::StopWorker);
On 2013/03/07 01:54:35, Wez wrote:
> StopWorker "stops the worker asynchronously". Is it tought enough for our
needs?
>  Should it be KillWorker? :)

It is tougher than Chuck Norris (as long as we are in user mode).

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:78: // Asks the worker process to
crash generating a dump and closes
On 2013/03/07 01:54:35, Wez wrote:
> nit: "... crash and generate a dump, and ..."

Done.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:80: // to the generated dump.
Restarts the worker process forcefully, if it does
On 2013/03/07 01:54:35, Wez wrote:
> nit: "... it is on the stack in the dump."

Done.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:80: // to the generated dump.
Restarts the worker process forcefully, if it does
On 2013/03/07 01:54:35, Wez wrote:
> nit: Does it always restart it, or just kill it and let higher-level code
> restart it if need be?

It always restarts it. If the caller decide to stop the process it should delete
the entire WorkerProcessLauncher object.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher_unittest.cc:150: // Receives mesages
sent to the worker process.
On 2013/03/07 01:54:35, Wez wrote:
> typo

Done.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher_unittest.cc:474: // comply.
On 2013/03/07 01:54:35, Wez wrote:
> How - are we relying on the timeout? i.e. will this test always take 5s?

Good point. I added reduced the delay to 1ms when running the test.

[Wez, 2013-03-08 01:45:15]

LGTM w/ nits

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:49: const int kDelayTimeoutSeconds
= 5;
On 2013/03/07 21:28:30, alexeypa wrote:
> On 2013/03/07 01:54:35, Wez wrote:
> > 5s is probably too short for many systems, BTW.
> 
> It is enough because the timeout is, strictly speaking, optional. We can kill
> the process at the spot. It is just waiting a little bit can give us a bit of
> useful information.

Right, but if the timeout is short then on slower, older systems we lose that
data more easily. Hopefully not a big deal.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:98: static void Destruct(const
Core* core);
On 2013/03/07 21:28:30, alexeypa wrote:
> On 2013/03/07 01:54:35, Wez wrote:
> > We use an explicit separate struct e.g. CoreTraits to achieve this in other
> > places.
> 
> WorkerProcessLauncher::Core is a private class declared in
> WorkerProcessLauncher. Having a separate Traits structure will pollute
> WorkerProcessLauncher with unnecessary "friend" declarations.
> 

Surely the friend declaration would be in the Core definition, here, though?

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher_unittest.cc:474: // comply.
On 2013/03/07 21:28:30, alexeypa wrote:
> On 2013/03/07 01:54:35, Wez wrote:
> > How - are we relying on the timeout? i.e. will this test always take 5s?
> 
> Good point. I added reduced the delay to 1ms when running the test.

nit: Could that be zero ms, i.e. kill it as soon as the message is processed?

https://chromiumcodereview.appspot.com/12545006/diff/8001/remoting/host/win/w...
File remoting/host/win/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/12545006/diff/8001/remoting/host/win/w...
remoting/host/win/worker_process_launcher.h:93: // Hooks that allow test code to
call the corresponding methods of |Core|.
nit: Since these are test specific the convention would be something like:

ResetLaunchSuccessTimeoutForTest()
SetKillProcessTimeoutForTest()

so that automated tools can catch if they're (ab)used elsewhere.

[alexeypa (please no reviews), 2013-03-08 17:43:19]

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:98: static void Destruct(const
Core* core);
On 2013/03/08 01:45:15, Wez wrote:
> Surely the friend declaration would be in the Core definition, here, though?

No, there will be one in WorkerProcessLauncher and another in
WorkerProcessLauncher::Core.

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/12545006/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher_unittest.cc:474: // comply.
On 2013/03/08 01:45:15, Wez wrote:
> On 2013/03/07 21:28:30, alexeypa wrote:
> > On 2013/03/07 01:54:35, Wez wrote:
> > > How - are we relying on the timeout? i.e. will this test always take 5s?
> > 
> > Good point. I added reduced the delay to 1ms when running the test.
> 
> nit: Could that be zero ms, i.e. kill it as soon as the message is processed?

Done.

https://chromiumcodereview.appspot.com/12545006/diff/8001/remoting/host/win/w...
File remoting/host/win/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/12545006/diff/8001/remoting/host/win/w...
remoting/host/win/worker_process_launcher.h:93: // Hooks that allow test code to
call the corresponding methods of |Core|.
On 2013/03/08 01:45:15, Wez wrote:
> nit: Since these are test specific the convention would be something like:
> 
> ResetLaunchSuccessTimeoutForTest()
> SetKillProcessTimeoutForTest()
> 
> so that automated tools can catch if they're (ab)used elsewhere.

Done.

[commit-bot: I haz the power, 2013-03-08 17:55:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12545006/3002

[commit-bot: I haz the power, 2013-03-08 17:55:39]

Presubmit check for 12545006-3002 failed and returned exit status 1.

INFO:root:Found 18 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  remoting/host/win/worker_process_launcher.cc:88
    void ResetLaunchSuccessTimeoutForTest(); \
  remoting/host/win/worker_process_launcher.cc:91
    void SetKillProcessTimeoutForTest(const base::TimeDelta& timeout); \
  remoting/host/win/worker_process_launcher.cc:415
    core_->ResetLaunchSuccessTimeoutForTest(); \
  remoting/host/win/worker_process_launcher.cc:420
    core_->SetKillProcessTimeoutForTest(timeout);

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    remoting/host/chromoting_messages.h

Presubmit checks took 4.6s to calculate.

[alexeypa (please no reviews), 2013-03-08 17:59:38]

Cris, could you please take a look at remoting/host/chromoting_messages.h? The
CL just merges two messages with identical parameters into one.

[alexeypa (please no reviews), 2013-03-08 18:11:29]

On 2013/03/08 17:59:38, alexeypa wrote:
> Cris, could you please take a look at remoting/host/chromoting_messages.h? The
> CL just merges two messages with identical parameters into one.

It looks like Cris is at the conference in Vancouver today. Justin, could you
take a look at remoting/host/chromoting_messages.h? Thanks.

[jschuh, 2013-03-08 20:52:38]

On 2013/03/08 18:11:29, alexeypa wrote:
> On 2013/03/08 17:59:38, alexeypa wrote:
> > Cris, could you please take a look at remoting/host/chromoting_messages.h?
The
> > CL just merges two messages with identical parameters into one.
> 
> It looks like Cris is at the conference in Vancouver today. Justin, could you
> take a look at remoting/host/chromoting_messages.h? Thanks.

title pretty much explains the security implications. ipc security lgtm.

[commit-bot: I haz the power, 2013-03-08 20:59:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12545006/3002

[commit-bot: I haz the power, 2013-03-08 22:37:59]

Change committed as 187055