Crash for bad message in BrowserFilter

Crash for bad message in BrowserFilter

This matches behavior in RenderProcessHostImpl::ShutdownForBadMessage.
This is particularly important on android, where in-process gpu and
renderers are used in production, and ChildProcessLauncher does not
support killing the browser process.

Note, not using run_renderer_in_process() check because BrowserFilter
may be used for other types of child processes.

BUG=682023
Review-Url: https://codereview.chromium.org/2640083002
Cr-Commit-Position: refs/heads/master@{#445421}
Committed: https://chromium.googlesource.com/chromium/src/+/1e846547dbff976d7758b59d836f3573e4446c32

[boliu, 2017-01-18 19:02:05]

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

[commit-bot: I haz the power, 2017-01-18 19:03:15]

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

[boliu, 2017-01-18 19:42:02]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2017-01-19 15:12:43]

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

[boliu, 2017-01-19 15:12:44]

ptal

I wrapped it in OS_ANDROID because I only tested on android, and only android
webview ships with single process. I'm not sure if the check works on windows.

[jam, 2017-01-20 15:55:13]

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
content/public/browser/browser_message_filter.cc:165: if
(base::Process::Current().Handle() == peer_process_.Handle()) {
usually this is checked via
RenderProcessHost::run_renderer_in_process()

also since this applies to all platforms, no need for the android check

[boliu, 2017-01-20 16:09:54]

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
content/public/browser/browser_message_filter.cc:165: if
(base::Process::Current().Handle() == peer_process_.Handle()) {
On 2017/01/20 15:55:13, jam wrote:
> usually this is checked via
> RenderProcessHost::run_renderer_in_process()
> 
> also since this applies to all platforms, no need for the android check

Can BrowserMessageFilter be used with non-renderer child processes, like gpu? If
so, I think run_renderer_in_process isn't good enough here?

[jam, 2017-01-21 01:39:49]

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
content/public/browser/browser_message_filter.cc:165: if
(base::Process::Current().Handle() == peer_process_.Handle()) {
On 2017/01/20 16:09:54, boliu wrote:
> On 2017/01/20 15:55:13, jam wrote:
> > usually this is checked via
> > RenderProcessHost::run_renderer_in_process()
> > 
> > also since this applies to all platforms, no need for the android check
> 
> Can BrowserMessageFilter be used with non-renderer child processes, like gpu?
If
> so, I think run_renderer_in_process isn't good enough here?

run_renderer_in_process is really for single process mode, it was just called
that because it was before we had other process types.


The UI thread version of this method has a check for that:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

[boliu, 2017-01-23 16:07:35]

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
content/public/browser/browser_message_filter.cc:165: if
(base::Process::Current().Handle() == peer_process_.Handle()) {
On 2017/01/21 01:39:49, jam wrote:
> On 2017/01/20 16:09:54, boliu wrote:
> > On 2017/01/20 15:55:13, jam wrote:
> > > usually this is checked via
> > > RenderProcessHost::run_renderer_in_process()
> > > 
> > > also since this applies to all platforms, no need for the android check
> > 
> > Can BrowserMessageFilter be used with non-renderer child processes, like
gpu?
> If
> > so, I think run_renderer_in_process isn't good enough here?
> 
> run_renderer_in_process is really for single process mode, it was just called
> that because it was before we had other process types.
> 
> 
> The UI thread version of this method has a check for that:
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

What about --in-process-gpu, but renderer still runs in child processes? This
happens on low memory devices on chrome on android.

Really a theoretical question I suppose, since there are no
BrowserMessageFilters that talks to the gpu process afaict. But still
correctness first?

[jam, 2017-01-23 16:55:29]

lgtm

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
content/public/browser/browser_message_filter.cc:165: if
(base::Process::Current().Handle() == peer_process_.Handle()) {
On 2017/01/23 16:07:35, boliu wrote:
> On 2017/01/21 01:39:49, jam wrote:
> > On 2017/01/20 16:09:54, boliu wrote:
> > > On 2017/01/20 15:55:13, jam wrote:
> > > > usually this is checked via
> > > > RenderProcessHost::run_renderer_in_process()
> > > > 
> > > > also since this applies to all platforms, no need for the android check
> > > 
> > > Can BrowserMessageFilter be used with non-renderer child processes, like
> gpu?
> > If
> > > so, I think run_renderer_in_process isn't good enough here?
> > 
> > run_renderer_in_process is really for single process mode, it was just
called
> > that because it was before we had other process types.
> > 
> > 
> > The UI thread version of this method has a check for that:
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...
> 
> What about --in-process-gpu, but renderer still runs in child processes? This
> happens on low memory devices on chrome on android.
> 
> Really a theoretical question I suppose, since there are no
> BrowserMessageFilters that talks to the gpu process afaict. But still
> correctness first?

ok you convinced me, but can you removed the android check?

[boliu, 2017-01-23 17:55:50]

Description was changed from

==========
Crash for bad message in BrowserFilter

For android, detect single process, and crash the browser if detected.
This is because Android cannot terminate non-child processes.

BUG=682023
==========

to

==========
Crash for bad message in BrowserFilter

This matches behavior in RenderProcessHostImpl::ShutdownForBadMessage.
This is particularly important on android, where in-process gpu and
renderers are used in production, and ChildProcessLauncher does not
support killing the browser process.

Note, not using run_renderer_in_process() check because BrowserFilter
may be used for other types of child processes.

BUG=682023
==========

[boliu, 2017-01-23 17:57:29]

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2640083002/diff/20001/content/public/browser/...
content/public/browser/browser_message_filter.cc:165: if
(base::Process::Current().Handle() == peer_process_.Handle()) {
On 2017/01/23 16:55:28, jam wrote:
> On 2017/01/23 16:07:35, boliu wrote:
> > On 2017/01/21 01:39:49, jam wrote:
> > > On 2017/01/20 16:09:54, boliu wrote:
> > > > On 2017/01/20 15:55:13, jam wrote:
> > > > > usually this is checked via
> > > > > RenderProcessHost::run_renderer_in_process()
> > > > > 
> > > > > also since this applies to all platforms, no need for the android
check
> > > > 
> > > > Can BrowserMessageFilter be used with non-renderer child processes, like
> > gpu?
> > > If
> > > > so, I think run_renderer_in_process isn't good enough here?
> > > 
> > > run_renderer_in_process is really for single process mode, it was just
> called
> > > that because it was before we had other process types.
> > > 
> > > 
> > > The UI thread version of this method has a check for that:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...
> > 
> > What about --in-process-gpu, but renderer still runs in child processes?
This
> > happens on low memory devices on chrome on android.
> > 
> > Really a theoretical question I suppose, since there are no
> > BrowserMessageFilters that talks to the gpu process afaict. But still
> > correctness first?
> 
> ok you convinced me, but can you removed the android check?

Done, also updated the comment, and CL description

[boliu, 2017-01-23 17:57:38]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-01-23 17:57:39]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2640083002/#ps40001
(title: "remove OS_ANDROID")

[commit-bot: I haz the power, 2017-01-23 17:57:59]

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

[jam, 2017-01-23 18:09:36]

lgtm

[commit-bot: I haz the power, 2017-01-23 18:56:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485194258876120,
"parent_rev": "a2a76710490ee762c3d38589d2582a4649b059f4", "commit_rev":
"1e846547dbff976d7758b59d836f3573e4446c32"}

[commit-bot: I haz the power, 2017-01-23 18:57:14]

Description was changed from

==========
Crash for bad message in BrowserFilter

This matches behavior in RenderProcessHostImpl::ShutdownForBadMessage.
This is particularly important on android, where in-process gpu and
renderers are used in production, and ChildProcessLauncher does not
support killing the browser process.

Note, not using run_renderer_in_process() check because BrowserFilter
may be used for other types of child processes.

BUG=682023
==========

to

==========
Crash for bad message in BrowserFilter

This matches behavior in RenderProcessHostImpl::ShutdownForBadMessage.
This is particularly important on android, where in-process gpu and
renderers are used in production, and ChildProcessLauncher does not
support killing the browser process.

Note, not using run_renderer_in_process() check because BrowserFilter
may be used for other types of child processes.

BUG=682023

Review-Url: https://codereview.chromium.org/2640083002
Cr-Commit-Position: refs/heads/master@{#445421}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e846547dbff976d7758b59d836f...
==========

[commit-bot: I haz the power, 2017-01-23 18:57:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1e846547dbff976d7758b59d836f...