Kill child processes on bad Mojo messages

Kill child processes on bad Mojo messages

Hooks RenderProcessHostImpl and BrowserChildProcessHostImpl
up to the new EDK API for process error notifications. If
any Mojo bindings receive a malformed message directly from
a child process, this will result in the child's termination.

BUG=607293
Committed: https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c
Cr-Commit-Position: refs/heads/master@{#400085}

[Ken Rockot(use gerrit already), 2016-06-10 23:02:52]

rockot@chromium.org changed reviewers:
+ jam@chromium.org

[Ken Rockot(use gerrit already), 2016-06-10 23:03:00]

Patchset #1 (id:1) has been deleted

[jam, 2016-06-13 18:30:39]

https://codereview.chromium.org/2054303002/diff/40001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:244:
mojo::edk::ProcessErrorCallback(),
why not do it here as well?

in general we try to maintain parity between renderer and other processes

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_browsertest.cc:240: 
nit: extra line

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2638: LOG(ERROR) <<
"Killing render process process: " << error;
nit: no need, since bad_message::ReceivedBadMessage does this

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:190: void
OnMojoProcessError(const std::string& error);
nit: this should be private.

also, no nit to document this method since it's obvious
(https://google.github.io/styleguide/cppguide.html#Implementation_Comments)


lastly, maybe OnMojoError is clearer name?

[Ken Rockot(use gerrit already), 2016-06-13 22:32:38]

Patchset #4 (id:80001) has been deleted

[Ken Rockot(use gerrit already), 2016-06-13 22:32:41]

Patchset #3 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2016-06-13 22:36:40]

https://codereview.chromium.org/2054303002/diff/40001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:244:
mojo::edk::ProcessErrorCallback(),
On 2016/06/13 at 18:30:39, jam wrote:
> why not do it here as well?
> 
> in general we try to maintain parity between renderer and other processes

done

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_browsertest.cc:240: 
On 2016/06/13 at 18:30:39, jam wrote:
> nit: extra line

done

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2638: LOG(ERROR) <<
"Killing render process process: " << error;
On 2016/06/13 at 18:30:39, jam wrote:
> nit: no need, since bad_message::ReceivedBadMessage does this

The message logged by ReceivedBadMessage is not particularly useful for a
developer encountering this. The message here may include details about the
specific Mojo interface and method name which triggered the error.

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:190: void
OnMojoProcessError(const std::string& error);
On 2016/06/13 at 18:30:39, jam wrote:
> nit: this should be private.

Oops, done

> 
> also, no nit to document this method since it's obvious
(https://google.github.io/styleguide/cppguide.html#Implementation_Comments)
> 

Done

> 
> lastly, maybe OnMojoError is clearer name?

Done

[Ken Rockot(use gerrit already), 2016-06-13 22:37:05]

rockot@chromium.org changed reviewers:
+ isherman@chromium.org

[Ken Rockot(use gerrit already), 2016-06-13 22:37:05]

+isherman@ please take a look at histograms

[jam, 2016-06-13 23:27:13]

lgtm

https://codereview.chromium.org/2054303002/diff/40001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2054303002/diff/40001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:244:
mojo::edk::ProcessErrorCallback(),
On 2016/06/13 22:36:40, Ken Rockot wrote:
> On 2016/06/13 at 18:30:39, jam wrote:
> > why not do it here as well?
> > 
> > in general we try to maintain parity between renderer and other processes
> 
> done

nit: please update cl desc & title

https://codereview.chromium.org/2054303002/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/2054303002/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:563:
mojo::edk::ChildProcessLaunched(process_.Handle(),
nit: ChildProcessLaunche's documentation should mention the thread that
process_error_callback_ is called back on (i.e. if it's fixed or it could be
called back on any thread)

[Ilya Sherman, 2016-06-13 23:28:22]

histograms.xml lgtm

[Ken Rockot(use gerrit already), 2016-06-14 00:24:08]

Description was changed from

==========
Kill render processes on bad Mojo messages

Hooks RPHI up to the new EDK API for process error
notifications. If any Mojo bindings receive a malformed message
directly from a render process, this will result in the renderer's
termination.

BUG=607293
==========

to

==========
Kill child processes on bad Mojo messages

Hooks RenderProcessHostImpl and BrowserChildProcessHostImpl
up to the new EDK API for process error notifications. If
any Mojo bindings receive a malformed message directly from
a child process, this will result in the child's termination.

BUG=607293
==========

[Ken Rockot(use gerrit already), 2016-06-14 00:28:02]

https://codereview.chromium.org/2054303002/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/2054303002/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:563:
mojo::edk::ChildProcessLaunched(process_.Handle(),
On 2016/06/13 at 23:27:12, jam wrote:
> nit: ChildProcessLaunche's documentation should mention the thread that
process_error_callback_ is called back on (i.e. if it's fixed or it could be
called back on any thread)

done

[Ken Rockot(use gerrit already), 2016-06-14 00:28:06]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-06-14 00:28:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2054303002/#ps120001
(title: ".")

[commit-bot: I haz the power, 2016-06-14 00:28:44]

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

[commit-bot: I haz the power, 2016-06-14 00:28:46]

This CL has an open dependency (Issue 2064903002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Ken Rockot(use gerrit already), 2016-06-16 04:14:53]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-06-16 04:14:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2054303002/#ps190001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-16 04:15:04]

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

[commit-bot: I haz the power, 2016-06-16 04:46:29]

Committed patchset #8 (id:190001)

[commit-bot: I haz the power, 2016-06-16 04:46:33]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 04:48:35]

Description was changed from

==========
Kill child processes on bad Mojo messages

Hooks RenderProcessHostImpl and BrowserChildProcessHostImpl
up to the new EDK API for process error notifications. If
any Mojo bindings receive a malformed message directly from
a child process, this will result in the child's termination.

BUG=607293
==========

to

==========
Kill child processes on bad Mojo messages

Hooks RenderProcessHostImpl and BrowserChildProcessHostImpl
up to the new EDK API for process error notifications. If
any Mojo bindings receive a malformed message directly from
a child process, this will result in the child's termination.

BUG=607293

Committed: https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c
Cr-Commit-Position: refs/heads/master@{#400085}
==========

[commit-bot: I haz the power, 2016-06-16 04:48:37]

Patchset 8 (id:??) landed as
https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c
Cr-Commit-Position: refs/heads/master@{#400085}