Toby and I discussed this some and we think this is likely contributing to background apps getting killed more, and we should probably change it.

The idea we had was to add an API to let the app indicate the importance of a particular WebView, probably with three states:

1) Important. This would be the default and would cause us to bind strongly all the time, keeping the behaviour pretty much the same as in single-process.
2) Important if visible/not-paused/whatever. Just let us handle binding/unbinding in the same way that Chrome does.
3) Not important. Just don't bind strongly at all. This could be used either for things like ads where you'd rather the ad die then your app even when in the foreground, or could be used by an app that has its own scheme for toggling between important and not important because it wants to base it on a different signal than the one we pick for 2).

For each renderer we'd just take the maximum importance of all the webviews associated with that renderer and use that to decide whether to bind strongly or not.

Obviously, apps would only want to use this API in the case where they have also implemented the handle-renderer-crash API, so that they can survive the death of the renderer.

Does something like that sound reasonable? This has the advantage of continuing to work sensibly if we ever have more than one renderer process.

That suggestion sounds great to me! I might add in a 4th though:
4) Less important - use BIND_VISIBLE / BIND_PERCEPTABLE when in foreground (or maybe even better - using BIND_ABOVE_CLIENT to other renderer)

Reason being is that 3) will likely cause webviews to be killed prematurely.

Possibly I've got the terms mixed up here and what you're describing is actually what I meant by 3). If we like the general idea we should figure out exactly what BIND_* flags we actually mean by the different states. :)

sgtm

So I looked at the current behaviour and as far as I can tell right now we *do* bind strongly all the time; the code Bo linked to doesn't seem to ever be called in WebView; it all looks like it's driven by ChromeActivitySessionTracker which we don't use.

"adb shell dumpsys activity processes" lists all the processes and no matter what state webview apps are in, it seems to list two bindings there, one with the "IMP" flag.

Hmm, then it really is just anecdote that apps appear more likely to get killed in the background? Meh :/

RE #1, For WebView,

In which case, "1) Important" is needed?

As to "2) Important if visible/not-paused/whatever", can we detect WebView app's status, and set binding flag accordingly.

Unfortunately it does look like that's the case, Bo :/


The main need for "Important" is to be the default behaviour for apps that haven't been updated to deal with renderer death. If we give them any other behaviour, then the app is more likely to crash with multiprocess enabled, and that's not really okay. I can see some apps wanting to leave it in this mode, though, if they don't have a reliable way to restore the state of the webview after a renderer crash (e.g. if the user is likely to be editing something inside the web content that they don't have a way to preserve).

That's exactly what 2) means - we will use one of the existing signals (probably visibility or webview paused state) to automatically do the binding for the app, instead of the app having to control it manually.

I wonder if we can actually measure my anecdote.. Or honestly, it would probably be easier/better if someone from android team can tell us all the algorithm/heuristics android uses to kill processes. Maybe there's a doc somewhere..

The importance API is probably lower priority imo.

It isn't totally vital to ship, but it would be nice, because without this control some apps would have a very hard time if we tried to move to multiple renderers. If we have this API from the beginning it's more viable to enable multiple renderers later.

So from the meeting today, about AGSA's use case. I'm not really convinced the importance api will be better than just destroying the webview. I do think the importance API is a good idea in general though.

Let's assume for now that when all webviews are destroyed, we drop all bindings to the renderer. If this is not the case, we probably should just make it happen.

Without the importance API, backgrounded AGSA could listen to onTrimMemory, and destroy the webview in that callback.

With the importance API, AGSA can lower the importance when in background, but then has to listen to the crash API and recreate the webview if the renderer is killed.

I don't really see a meaningful difference between these two

So right now we can't drop all bindings to the renderer when all webviews are destroyed, because currently the code treats the renderer exiting as an OOM, always, and immediately exit(0)'s the browser process - this needs fixing obviously but it's nontrivial (there are also other cases that are weird/broken, like when the browser explicitly kills the renderer for sending a bad message, which also is detected as this case and results in the browser exit(0)'ing).

We also probably wouldn't want to drop the renderer instantly when the last webview is destroyed as that seems likely to harm performance for apps that are going to create another webview very soon afterwards (the service is disposed of immediately when the last connection unbinds, it doesn't just go to LRU background list for the OOM killer to deal with).

With the importance API, AGSA would listen to the crash API, and then destroy the webview *without* recreating it if the renderer was killed while in the "unimportant" state. It would only recreate it later when AGSA came into the foreground and needed to use WebView again. Responding to onTrimMemory seems like a poor approximation of this, because onTrimMemory is a pretty vague signal - the OOM killer deciding the renderer wasn't used recently enough is pretty clear - "you aren't important enough, so I freed memory for you".

There isn't a huge difference, but I suspect the behaviour will be better for the user with the importance API.

> So right now we can't drop all bindings to the renderer when all webviews are destroyed, because currently the code treats the renderer exiting as an OOM, always, and immediately exit(0)'s the browser process

Oh gosh. Fix that now! Well, I guess michael is doing that with the crash api.

> We also probably wouldn't want to drop the renderer instantly when the last webview is destroyed as that seems likely to harm performance for apps that are going to create another webview very soon afterwards (the service is disposed of immediately when the last connection unbinds, it doesn't just go to LRU background list for the OOM killer to deal with).

Good point

> With the importance API, AGSA would listen to the crash API, and then destroy the webview *without* recreating it if the renderer was killed while in the "unimportant" state. It would only recreate it later when AGSA came into the foreground and needed to use WebView again.

You can do that with trim as well. Don't recreate the webview until it's needed again.

For the case that all webview are destroyed, it isn't related to the importance API, right? we can fix this issue by keeping the host application alive if there is no webview instance.

yes

Calling exit(0) in the OOM case is also bad. Anecdotally, it appears to cause the browser process to not actually exit, if it happens early enough.

And additionally the current scheme means we don't differentiate between cases where the browser receives a bad message from the renderer and unbinds it, from renderer sigkills.

Yeah, we should probably treat "the browser unbound from the renderer on purpose so that the system would kill it" as a crash, not a kill, because this happens when the renderer is broken (and potentially compromised), and we only do it this way because we don't have any more direct method to kill() it because it doesn't run as the same UID as us.

(relatedly, in this case we also do DumpWithoutCrashing in the browser process to generate a dump to act as a report that we had to do this in the first place, even though we can't get a dump from the renderer in this scenario, and we should make sure that silent crash uploading handles those dumps correctly and uploads them).

RE #16 why it cause browser process to not actually exit?

We don't know, and we're not sure exactly how we get into this bad state, it might not really be caused by that (there's a number of stacked problems with multiprocess right now which makes it difficult to unpick which problem is which)

We do call SetInForground (via RenderProcessHostImpl::UpdateProcessPriority) but as part of webview setup, we do the following:

    cl->AppendSwitch(switches::kDisableRendererBackgrounding);

which is what explains the renderer process priority is currently never lowered.

I think we should decided which binding flag should be used for each importance level, though I probably still not fully understand those flags, here is my understanding

- Important, the default value, since we want this matches single process mode as much as possible,  BIND_ABOVE_CLIENT seems suitable for this, app will have few chance to see render process is killed by OOM, though it isn't guaranteed, this flag hasn't been used in chrome.

- Important when visible, this is pretty much same as chrome's behavior. We need to call APIs if WebView is in background or foreground.

- not important,  we could use BIND_WAIVE_PRIORITY, this somehow is implemented in Chrome, and is the binding state that other binding connections are unbound.

I would propose that from a framework API perspective, we provide four priority settings:

HIGH - renderer priority is at least as high as app priority (default; hold a strong binding)
LOW - renderer priority is lower than app priority (hold a moderate binding)
WAIVED - keep only the waived priority binding
MANAGED - renderer priority is HIGH if visible, LOW if not.

I don't think we would ever want to do BIND_ABOVE_CLIENT, it doesn't make sense to insist on the app being killed before the renderer.

I'm not sure if there are reasonable use cases for apps to want a low priority there. I'm not sure what the actual impact on OOM score/state is, but I was assuming that apps, if they use this at all, are generally going to be doing it in a similar way to chrome, which doesn't use this level does it?

It matches single process behavior to make app being killed before the renderer, and should be default value, so app (instead of render process) will be judged whether it should be killed for OOM; otherwise the apps that doesn't implement crash API might be easy to be killed.

I don't understand what you mean; it doesn't match the single process behaviour, there is no separate killing there, so it can't. I don't see how there is any benefit to using BIND_ABOVE_CLIENT for apps that don't implement the crash API: if either process is killed they will die, so it doesn't matter which one crashes first.

The difference is how the crash appears to app, if BIND_ABOVE_CLIENT is used, the crash could happen in anywhere in app, this matches single process mode behavior, right? otherwise, the crash is mostly coming from render process's crash.

These are OOM kills, so neither will look like reportable crashes.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/401a3c143672daee741c309a213aa690673cf1bd

commit 401a3c143672daee741c309a213aa690673cf1bd
Author: tobiasjs <tobiasjs@chromium.org>
Date: Fri Jan 27 10:35:31 2017

Implement renderer importance API for WebView

The renderer importance API allows a WebView to declare which priority
from the set {HIGH, LOW, WAIVED} its associated renderer process
should be bound with. HIGH corresponds to binding with
Context.BIND_IMPORTANT, LOW corresponds to keeping a normal binding
to the service. WAIVED corresponds to binding with BIND_WAIVE_PRIORITY.

Additionally, the priority can be automatically lowered to WAIVE when
the WebView instance is not visible.

When multiple WebViews are assocated with the same renderer, the
renderer will receive a priority equal to the maximum over the set
of associated WebViews.

The default is HIGH, without visibility management, for backwards
compatibility with single-process.

BUG= 667531 

Review-Url: https://codereview.chromium.org/2650753002
Cr-Commit-Position: refs/heads/master@{#446636}

[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/BUILD.gn
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/java/src/org/chromium/android_webview/AwRenderProcessGoneDetail.java
[add] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/java/src/org/chromium/android_webview/AwRendererPriorityManager.java
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/lib/main/aw_main_delegate.cc
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/native/BUILD.gn
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/native/aw_contents.cc
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/native/aw_contents.h
[add] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/native/aw_renderer_priority_manager.cc
[add] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/android_webview/native/aw_renderer_priority_manager.h
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/content/public/common/content_switches.cc
[modify] https://crrev.com/401a3c143672daee741c309a213aa690673cf1bd/content/public/common/content_switches.h

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/1ae830016230ac463ac61dbe5853a5c1f29d8d1e

commit 1ae830016230ac463ac61dbe5853a5c1f29d8d1e
Author: Tobias Sargeant <tobiasjs@google.com>
Date: Wed Jan 25 16:44:42 2017

Just noticed webview code directly calls ChildProcessLauncher.setInForeground

That's not ok. And this basically roughly has the same hallmark of  crbug.com/664530 , that an API is being abused, incorrectly.

Directly calling setInForeground is not ok, because that's an internal implementation detail in content. content code will continue to call it based on whether there are foreground widgets in that process. And webview is actively circumventing that process by calling setInForeground directly, and then just hoping it doesn't conflict with what content does.

How webview child process priority works:
* onBroughtToForeground / onSentToBackground do not get called. These calls indicate the entire app is foreground or background, although looks like this isn't an "accident" that these aren't called, since the callers in chrome are actually in chrome/ code, so webview just isn't calling these things.
* Add kDisableRendererPriorityManagement. This disables content behavior that would normally call setInForeground. This is the call that indicates a particular process is foreground or background and normally controlled by whether a webcontents is visible or not.
* Never enable "moderate binding management" thing
* Hook up setInForeground from AwContents. Explicitly iterate over the all AwConents hosted in the AwContents and take the max priority.

So this mostly works, besides the layering violation mentioned above and how crazy that loop looks. The only subtle thing is the initial binding should really be strong rather than moderate, but that's pretty thing.

This would not scale to more than one renderer process though on low ram device, since setInForeground also has the "at most one connection have strong binding" behavior (only enabled on low ram device).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a16b9c0613315c731427493048cab07dd00f7ff6

commit a16b9c0613315c731427493048cab07dd00f7ff6
Author: Bo Liu <boliu@chromium.org>
Date: Wed Aug 16 22:22:55 2017

aw: Call RenderProcessHost::SetMaxRendererProcessCount

instead of adding a command line. These to exactly the same thing, but
now the command line is freed up to be used by tests.

Bug:  667531 
Change-Id: I20b93f7f9c69233db019dd489e3edddefed38617
Reviewed-on: https://chromium-review.googlesource.com/617806
Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494975}
[modify] https://crrev.com/a16b9c0613315c731427493048cab07dd00f7ff6/android_webview/lib/aw_main_delegate.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924

commit 18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924
Author: Bo Liu <boliu@chromium.org>
Date: Tue Aug 29 16:00:18 2017

aw: Use ChildProcessImportance

Remove all the complexity in android webview to loop over all AwContents
and aggregate over the priority, and just use the newly added
ChildProcessImportance instead.

This means webview can no longer set kDisableRendererPriorityManagement.
However webview needs binding importance to be completely independent
from visibility, so add a new CreationParam to ignore implicit signals
for importance.

Changes in non-webview areas:
* Webview needs ChildProcessImportance from RenderProcessHost, so needed
to move child_process_importance from content/browser to
content/public/browser
* Add another parameter to ChildProcessCreationParams which needed all
call sites of its constructor to be updated.
* Removed kDisableRendererPriorityManagement which was only added for
webview to skip visibility prioritization. This is now replaced with
the new ChildProcessCreationParams.

Bug:  667531 
Change-Id: I3573fc64953db95321f68065b494c834bf943f2c
Reviewed-on: https://chromium-review.googlesource.com/617264
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498124}
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/android_webview/BUILD.gn
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/android_webview/browser/aw_contents.cc
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/android_webview/browser/aw_contents.h
[add] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/android_webview/browser/aw_renderer_priority.h
[delete] https://crrev.com/0579ed631fb37de5704b54ed2ee466bf29630ad0/android_webview/browser/aw_renderer_priority_manager.cc
[delete] https://crrev.com/0579ed631fb37de5704b54ed2ee466bf29630ad0/android_webview/browser/aw_renderer_priority_manager.h
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/android_webview/java/src/org/chromium/android_webview/AwContents.java
[delete] https://crrev.com/0579ed631fb37de5704b54ed2ee466bf29630ad0/android_webview/java/src/org/chromium/android_webview/AwRendererPriorityManager.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/android_webview/lib/aw_main_delegate.cc
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/chrome/android/java/src/org/chromium/chrome/browser/MonochromeApplication.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/browser/child_process_launcher_helper.h
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherHelperTest.java
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/public/browser/render_process_host.h
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/public/common/content_switches.cc
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/public/common/content_switches.h
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/18c4a5ab5a4ff34f36fe687bb6d1d28d01e21924/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java

I guess the minor issue left is the initial binding. I wouldn't mind having some systematic way to solve that one as well

Changing status to assigned since there is an owner.

Bo, I think it's reasonable to transfer ownership of this to you.

Initial binding is relatively minor.