Don't share renderers between unrelated tabs on Android.

Don't share renderers between unrelated tabs on Android.

On Android we explicitly allow the OS to kill Chrome's background
renderers when under memory pressure and we don't try to control the
number of renderers ourselves.

The process limit logic in content causes process sharing
between unrelated tabs when the number of renderer process hosts
(not the number of actual live processes) is too high. Because on
Android the system adjusts the number of actual live processes for us,
we don't want to limit the number of process hosts or to ever share
renderers between unrelated tabs.

This patch:
- disables the renderer process host limit on Android. If not
  overridden, ShouldTryToUseExistingProcessHost() will always return
  false.
- drops the logic that sets the renderer limit based on the number of
  declared renderer services

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

[ppi, 2014-06-25 15:15:05]

Hello Grace,

Could you take a look?

Suggested looking order:
- content/browser/renderer_host/render_process_host_impl + unittests
- content/browser/site_instance_impl_unittest
-
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController

[klobag.chromium, 2014-06-27 16:25:42]

https://codereview.chromium.org/356453003/diff/40001/components/gcm_driver/an...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/356453003/diff/40001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:252:
BrowserStartupController.get(context).startBrowserProcessesSync(false);
Hmm, we should check why they limit this to one. Do they want to track the
render pid?

https://codereview.chromium.org/356453003/diff/40001/content/browser/android/...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/356453003/diff/40001/content/browser/android/...
content/browser/android/content_startup_flags.cc:47: if (in_process) {
hmm what happen is command_line_renderer_limit > 0 and in_process is true?

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

https://codereview.chromium.org/356453003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:401: #if
defined(OS_ANDROID)
hmm, I don't think this is a good way. It makes this api very subtle.

https://codereview.chromium.org/356453003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1798: if
(render_host_limit && g_all_hosts.Get().size() >= render_host_limit)
Should we check the live process count?

https://codereview.chromium.org/356453003/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/356453003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:249:
Log.i(TAG, "Initializing chromium process.");
log inProcess.

[ppi, 2014-06-27 16:40:32]

Thanks! Please find replies below.

https://codereview.chromium.org/356453003/diff/40001/components/gcm_driver/an...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/356453003/diff/40001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:252:
BrowserStartupController.get(context).startBrowserProcessesSync(false);
On 2014/06/27 16:25:42, klobag.chromium wrote:
> Hmm, we should check why they limit this to one. Do they want to track the
> render pid?

I spoke with johnme@ about this, we concluded we should be fine dropping the
limit here, they didn't rely on this limit afaiu.

https://codereview.chromium.org/356453003/diff/40001/content/browser/android/...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/356453003/diff/40001/content/browser/android/...
content/browser/android/content_startup_flags.cc:47: if (in_process) {
On 2014/06/27 16:25:42, klobag.chromium wrote:
> hmm what happen is command_line_renderer_limit > 0 and in_process is true?

Nothing in particular. It already can happen - the single process flag doesn't
depend on the process limit having any particular value afaiu.

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

https://codereview.chromium.org/356453003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:401: #if
defined(OS_ANDROID)
On 2014/06/27 16:25:42, klobag.chromium wrote:
> hmm, I don't think this is a good way. It makes this api very subtle.

But it doesn't diverge too much from the codepaths of the other platforms. We
still have all of the code working the same way, just returning infinity (-> no
limit) here (unless overriden) and a finite number on other platforms.

Alternative would be to make the change in
"ShouldTryToUseExistingProcessHost()", making it always return false on Android.
But this breaks the connection between GetMaxRendererProcessCount() and
ShouldTryToUseExistingProcessHos() and takes away the useful ability of
overriding the limit to have the old behavior back (e.g. for local
experimentation).

Wdyt?

https://codereview.chromium.org/356453003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1798: if
(render_host_limit && g_all_hosts.Get().size() >= render_host_limit)
On 2014/06/27 16:25:42, klobag.chromium wrote:
> Should we check the live process count?

Dead process hosts can come back (re-spawn their processes) without going
through this path. So for other platforms checking the number of process hosts
can make sense I think.

[ppi, 2014-06-27 16:44:33]

One idea below.

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

https://codereview.chromium.org/356453003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:401: #if
defined(OS_ANDROID)
On 2014/06/27 16:40:32, ppi wrote:
> On 2014/06/27 16:25:42, klobag.chromium wrote:
> > hmm, I don't think this is a good way. It makes this api very subtle.
> 
> But it doesn't diverge too much from the codepaths of the other platforms. We
> still have all of the code working the same way, just returning infinity (->
no
> limit) here (unless overriden) and a finite number on other platforms.
> 
> Alternative would be to make the change in
> "ShouldTryToUseExistingProcessHost()", making it always return false on
Android.
> But this breaks the connection between GetMaxRendererProcessCount() and
> ShouldTryToUseExistingProcessHos() and takes away the useful ability of
> overriding the limit to have the old behavior back (e.g. for local
> experimentation).
> 
> Wdyt?

Should we add a separate boolean function to the API like "bool
IsRendererProcessCountLimited()" to make this explicit / less tricky?

[ppi, 2014-07-03 13:56:46]

+John (Grace is ooo)

John, could you take a look at render_process_host and site_instance bits and
comment on the approach taken?

[jam, 2014-07-14 22:45:56]

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

https://codereview.chromium.org/356453003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:401: #if
defined(OS_ANDROID)
I agree with Grace that it's confusing to return 0 to mean no limit. Why not
return SIZE_MAX?

[ppi, 2014-07-16 13:26:54]

Thanks, ptal.

https://codereview.chromium.org/356453003/diff/40001/content/browser/android/...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/356453003/diff/40001/content/browser/android/...
content/browser/android/content_startup_flags.cc:47: if (in_process) {
On 2014/06/27 16:40:32, ppi wrote:
> On 2014/06/27 16:25:42, klobag.chromium wrote:
> > hmm what happen is command_line_renderer_limit > 0 and in_process is true?
> 
> Nothing in particular. It already can happen - the single process flag doesn't
> depend on the process limit having any particular value afaiu. 

Oh, I see there is a change in behavior. If both command_line_renderer_limit > 0
is set *and* the embedder requests single process mode, then:

- before: single process mode is ignored, renderer process limit override is set
- after: single process mode is honored (renderer process limit is also set but
it doesn't affect single process)

It feels reasonable to me, wdyt?

Also, in patch set 4 I restored the option to set singleProcess by passing
command_line_renderer_limit = 0, matching previous behavior.

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

https://codereview.chromium.org/356453003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:401: #if
defined(OS_ANDROID)
On 2014/07/14 22:45:56, jam wrote:
> I agree with Grace that it's confusing to return 0 to mean no limit. Why not
> return SIZE_MAX?

Done.

https://codereview.chromium.org/356453003/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/356453003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:249:
Log.i(TAG, "Initializing chromium process.");
On 2014/06/27 16:25:42, klobag.chromium wrote:
> log inProcess.

Done.

[jam, 2014-07-16 20:22:07]

lgtm

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

https://codereview.chromium.org/356453003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:397: // On Android we
don't maintain a limit of renderer process hosts - we are
nit: put the comment inside the ifdef to make it clear that this is for android

[ppi, 2014-07-17 10:14:45]

Thanks!

+mkosiba for webview
+johnme, fgorski for GCM

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

https://codereview.chromium.org/356453003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:397: // On Android we
don't maintain a limit of renderer process hosts - we are
On 2014/07/16 20:22:07, jam wrote:
> nit: put the comment inside the ifdef to make it clear that this is for
android

Done.

[johnme, 2014-07-17 11:28:20]

GCMDriver.java lgtm

https://codereview.chromium.org/356453003/diff/140001/content/browser/android...
File content/browser/android/content_startup_flags.h (right):

https://codereview.chromium.org/356453003/diff/140001/content/browser/android...
content/browser/android/content_startup_flags.h:15: void
SetContentCommandLineFlags(bool single_process,
In the .cc this is called "in_process", whereas here it's called
"single_process"?

In general I find the name in_process quite confusing - it's not obvious what
"starting a browser process in process" means; why not just always call it
single_process, since single process mode has a well-known and fairly intuitive
meaning?

[mkosiba (inactive), 2014-07-17 16:21:30]

android_webview/ LGTM

[fgorski, 2014-07-17 17:55:47]

components/gcm_driver/ lgtm

[ppi, 2014-07-18 11:13:17]

Thanks!

https://codereview.chromium.org/356453003/diff/140001/content/browser/android...
File content/browser/android/content_startup_flags.h (right):

https://codereview.chromium.org/356453003/diff/140001/content/browser/android...
content/browser/android/content_startup_flags.h:15: void
SetContentCommandLineFlags(bool single_process,
On 2014/07/17 11:28:20, johnme wrote:
> In the .cc this is called "in_process", whereas here it's called
> "single_process"?
> 
> In general I find the name in_process quite confusing - it's not obvious what
> "starting a browser process in process" means; why not just always call it
> single_process, since single process mode has a well-known and fairly
intuitive
> meaning?

Done.

[ppi, 2014-07-18 11:13:22]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-07-18 11:13:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ppi@chromium.org/356453003/180001

[commit-bot: I haz the power, 2014-07-18 15:03:05]

Change committed as 284095