android: Limit num renderer to service slots

android: Limit num renderer to service slots

This is a temporary workaround crbug.com/693484. The issue is chrome on
android expects the OS to not be able to maintain more than 20 service
processes. So the current code can only has 20 sandboxed slots, but
tells chrome that it can create as many renderer as it wants. On some
OEM devices, android has been tweaked to allow more than 20 background
processes, and this leads to a catastrophic failure case of not being
able to create new renderers.

The real fix of chrome actively killing LRU child is not ready, so this
is an imperfect workaround, to tell chrome to not create more than 20
renderers.

Note this will not cover all cases since this is a soft limit, and there
are other non-renderer sandboxed child processes beyond renderer. But
this should cover a large chunk of use cases to be address this severe
failure case in the short term.

BUG=693484
Review-Url: https://codereview.chromium.org/2793623002
Cr-Commit-Position: refs/heads/master@{#461482}
Committed: https://chromium.googlesource.com/chromium/src/+/cbef3d37319db3e6153494db5f97df9412959856

[boliu, 2017-03-31 19:02:49]

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

[commit-bot: I haz the power, 2017-03-31 19:03:37]

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

[boliu, 2017-03-31 21:03:45]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2017-04-01 00:28:25]

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

[boliu, 2017-04-01 00:28:26]

comfortable stamping this?

[Jay Civelli, 2017-04-03 16:19:48]

How does the Chrome native code handles the killing of the service when we reach
the maximum number of processes? Does it kill the process with kill()? In which
case, won't the framework restart the service automatically?

https://codereview.chromium.org/2793623002/diff/40001/content/browser/child_p...
File content/browser/child_process_launcher_helper.h (right):

https://codereview.chromium.org/2793623002/diff/40001/content/browser/child_p...
content/browser/child_process_launcher_helper.h:173: static size_t
GetNumberOfRendererSlots();
I am starting to get weary of adding more stuff in ifdef's here.
I tried hard when I refactored this to get rid of almost all of them.
I am OK with this one for now. My plan is at some point to revisit this ifdef
block and remove it entirely from here.

https://codereview.chromium.org/2793623002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_unittest.cc (left):

https://codereview.chromium.org/2793623002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_unittest.cc:73: // Verify that
by default the limit on Android returns max size_t.
Should we add a TODO to bring that test back if/when we address this bug more
appropriately?

[boliu, 2017-04-03 16:33:03]

pushing back a bit on all comments :p

On 2017/04/03 16:19:48, Jay Civelli wrote:
> How does the Chrome native code handles the killing of the service when we
reach
> the maximum number of processes? Does it kill the process with kill()? In
which
> case, won't the framework restart the service automatically?

Native code doesn't kill renderers. This just tell native code to *try not to*
create more than 20 renderers. And as I mentioned in the description, there are
lots of corner cases where it doesn't work. But getting the "main" case correct
is still worth something...

https://codereview.chromium.org/2793623002/diff/40001/content/browser/child_p...
File content/browser/child_process_launcher_helper.h (right):

https://codereview.chromium.org/2793623002/diff/40001/content/browser/child_p...
content/browser/child_process_launcher_helper.h:173: static size_t
GetNumberOfRendererSlots();
On 2017/04/03 16:19:48, Jay Civelli wrote:
> I am starting to get weary of adding more stuff in ifdef's here.
> I tried hard when I refactored this to get rid of almost all of them.
> I am OK with this one for now. My plan is at some point to revisit this ifdef
> block and remove it entirely from here.

I suppose this doesn't have to go through CPL. RPHI can directly call into some
java thing. But that does involve adding more jni files and whatnot, so I
settled on this. I can add new files if you don't think it's worth it.

https://codereview.chromium.org/2793623002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_unittest.cc (left):

https://codereview.chromium.org/2793623002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_unittest.cc:73: // Verify that
by default the limit on Android returns max size_t.
On 2017/04/03 16:19:48, Jay Civelli wrote:
> Should we add a TODO to bring that test back if/when we address this bug more
> appropriately?

I think this is a really dumb check and should not exist period. Why does it
matter what that "infinite" constant is? The code below already shows creating
more renderers than practical is still allowed.

[Jay Civelli, 2017-04-03 16:40:50]

https://codereview.chromium.org/2793623002/diff/40001/content/browser/child_p...
File content/browser/child_process_launcher_helper.h (right):

https://codereview.chromium.org/2793623002/diff/40001/content/browser/child_p...
content/browser/child_process_launcher_helper.h:173: static size_t
GetNumberOfRendererSlots();
On 2017/04/03 16:33:03, boliu wrote:
> On 2017/04/03 16:19:48, Jay Civelli wrote:
> > I am starting to get weary of adding more stuff in ifdef's here.
> > I tried hard when I refactored this to get rid of almost all of them.
> > I am OK with this one for now. My plan is at some point to revisit this
ifdef
> > block and remove it entirely from here.
> 
> I suppose this doesn't have to go through CPL. RPHI can directly call into
some
> java thing. But that does involve adding more jni files and whatnot, so I
> settled on this. I can add new files if you don't think it's worth it.

That's fine for now.

https://codereview.chromium.org/2793623002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_unittest.cc (left):

https://codereview.chromium.org/2793623002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_unittest.cc:73: // Verify that
by default the limit on Android returns max size_t.
On 2017/04/03 16:33:03, boliu wrote:
> On 2017/04/03 16:19:48, Jay Civelli wrote:
> > Should we add a TODO to bring that test back if/when we address this bug
more
> > appropriately?
> 
> I think this is a really dumb check and should not exist period. Why does it
> matter what that "infinite" constant is? The code below already shows creating
> more renderers than practical is still allowed.

Makes sense.

[Jay Civelli, 2017-04-03 16:40:58]

lgtm

[boliu, 2017-04-03 16:44:35]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2017-04-03 16:45:15]

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-04-03 18:35:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491237875123810,
"parent_rev": "b6d0c9a06ab573af760763fcd7ecf76ae2c87695", "commit_rev":
"cbef3d37319db3e6153494db5f97df9412959856"}

[commit-bot: I haz the power, 2017-04-03 18:36:23]

Description was changed from

==========
android: Limit num renderer to service slots

This is a temporary workaround crbug.com/693484. The issue is chrome on
android expects the OS to not be able to maintain more than 20 service
processes. So the current code can only has 20 sandboxed slots, but
tells chrome that it can create as many renderer as it wants. On some
OEM devices, android has been tweaked to allow more than 20 background
processes, and this leads to a catastrophic failure case of not being
able to create new renderers.

The real fix of chrome actively killing LRU child is not ready, so this
is an imperfect workaround, to tell chrome to not create more than 20
renderers.

Note this will not cover all cases since this is a soft limit, and there
are other non-renderer sandboxed child processes beyond renderer. But
this should cover a large chunk of use cases to be address this severe
failure case in the short term.

BUG=693484
==========

to

==========
android: Limit num renderer to service slots

This is a temporary workaround crbug.com/693484. The issue is chrome on
android expects the OS to not be able to maintain more than 20 service
processes. So the current code can only has 20 sandboxed slots, but
tells chrome that it can create as many renderer as it wants. On some
OEM devices, android has been tweaked to allow more than 20 background
processes, and this leads to a catastrophic failure case of not being
able to create new renderers.

The real fix of chrome actively killing LRU child is not ready, so this
is an imperfect workaround, to tell chrome to not create more than 20
renderers.

Note this will not cover all cases since this is a soft limit, and there
are other non-renderer sandboxed child processes beyond renderer. But
this should cover a large chunk of use cases to be address this severe
failure case in the short term.

BUG=693484

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

[commit-bot: I haz the power, 2017-04-03 18:36:24]

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