Android: remove ContentViewCore from the renderer crash codepath.

Android: remove ContentViewCore from the renderer crash codepath.

This patch employs base::TerminationStatus to communicate the state of the oom
bindings when a child process dies and exposes renderProcessGone() in
WebContentsObserverAndroid, removing the need for ContentViewCore to be
involved in the renderer crash codepath.

This allows to drop tracking the pid of the renderer process in ContentViewCore
and eliminates the need for the |tab_crashed_| flag in native
ContentViewCoreImpl.

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

[ppi, 2013-11-04 19:16:27]

Hi Yaron,

This is a follow-up on your hint, as promised in
https://codereview.chromium.org/19969003/ :). Could you take a look?

[Yaron, 2013-11-05 01:48:43]

https://codereview.chromium.org/58203002/diff/1/chrome/browser/guestview/webv...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/58203002/diff/1/chrome/browser/guestview/webv...
chrome/browser/guestview/webview/webview_guest.cc:50: case
base::TERMINATION_STATUS_OOM_PROTECTED:
#if defined(OS_ANDROID) 

wouldn't compile otherwise, right? I think this is the other "webview" not
android_webview.

https://codereview.chromium.org/58203002/diff/1/content/browser/android/web_c...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/58203002/diff/1/content/browser/android/web_c...
content/browser/android/web_contents_observer_android.cc:69: jboolean
wasOomProtected =
was_oom_protected

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:397:
public static boolean isOomProtected(int pid) {
private

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:130:
public boolean isReadyForSnapshot() {
Have you verified this part is kosher? I think it probably is it looks like it
has other relevant checks. Additionally, it looks like it's for grabbing a
snapshot for resuming a cold tab? It might actually be dead code.

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
(left):

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:61:
public void onRendererCrash(boolean processWasOomProtected) {
You'll have to update a few more clients (esp. android_webview)

[ppi, 2013-11-05 13:58:18]

Thanks, Yaron! Could you take another look at patch set 3?

https://codereview.chromium.org/58203002/diff/1/chrome/browser/guestview/webv...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/58203002/diff/1/chrome/browser/guestview/webv...
chrome/browser/guestview/webview/webview_guest.cc:50: case
base::TERMINATION_STATUS_OOM_PROTECTED:
On 2013/11/05 01:48:43, Yaron wrote:
> #if defined(OS_ANDROID) 
> 
> wouldn't compile otherwise, right? I think this is the other "webview" not
> android_webview.

Done (yup, this is http://developer.chrome.com/apps/tags/webview.html).

https://codereview.chromium.org/58203002/diff/1/content/browser/android/web_c...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/58203002/diff/1/content/browser/android/web_c...
content/browser/android/web_contents_observer_android.cc:69: jboolean
wasOomProtected =
On 2013/11/05 01:48:43, Yaron wrote:
> was_oom_protected

Done.

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:397:
public static boolean isOomProtected(int pid) {
On 2013/11/05 01:48:43, Yaron wrote:
> private

Done.

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:130:
public boolean isReadyForSnapshot() {
On 2013/11/05 01:48:43, Yaron wrote:
> Have you verified this part is kosher? I think it probably is it looks like it
> has other relevant checks. Additionally, it looks like it's for grabbing a
> snapshot for resuming a cold tab? It might actually be dead code.

I moved this check to the caller, adding an equivalent call to
"tab.canProvideThumbnail()" in StartupSnapshotManager.

Now that I looked into it - yes, StartupSnapshotManager seems to be dead since
January, when we disabled it in Main onCreate(). Let's remove it in a follow-up?

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
(left):

https://codereview.chromium.org/58203002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:61:
public void onRendererCrash(boolean processWasOomProtected) {
On 2013/11/05 01:48:43, Yaron wrote:
> You'll have to update a few more clients (esp. android_webview)

Done.

[Yaron, 2013-11-06 01:08:17]

lgtm

+joth for aw and general sanity

[joth, 2013-11-06 01:51:18]

aw lgtm, and the rest doesn't look insane to me from a quick scan through :)

[ppi, 2013-11-06 11:22:01]

Thanks, Yaron and Jonathan! Adding OWNERs for the missing bits.

Jim, could you please take a look at:
- base/process/kill.h

John, could you please take a look at:
- content/browser/child_process_launcher.cc
- chrome/browser/guestview/webview/webview_guest.cc

[jam, 2013-11-06 17:39:15]

On 2013/11/06 11:22:01, ppi wrote:
> Thanks, Yaron and Jonathan! Adding OWNERs for the missing bits.
> 
> Jim, could you please take a look at:
> - base/process/kill.h
> 
> John, could you please take a look at:
> - content/browser/child_process_launcher.cc
> - chrome/browser/guestview/webview/webview_guest.cc

lgtm

[ppi, 2013-11-06 19:17:00]

Thanks, John!

I see that Jim has an all-day event on calendar today - Brett, could you please
have a look at the remaining file:
- base/process/kill.h
?

[jar (doing other things), 2013-11-06 19:27:25]

LGTM for base    ...but please address the nits

https://codereview.chromium.org/58203002/diff/320001/content/browser/android/...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/58203002/diff/320001/content/browser/android/...
content/browser/android/web_contents_observer_android.cc:64:
base::TerminationStatus termination_status) {
nit: 4 character indent from previous line's "v" in void.

https://codereview.chromium.org/58203002/diff/320001/content/browser/android/...
File content/browser/android/web_contents_observer_android.h (right):

https://codereview.chromium.org/58203002/diff/320001/content/browser/android/...
content/browser/android/web_contents_observer_android.h:36:
base::TerminationStatus termination_status) OVERRIDE;
nit: only use 4 character indent.  See line 40 for example.

https://codereview.chromium.org/58203002/diff/320001/content/browser/child_pr...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/58203002/diff/320001/content/browser/child_pr...
content/browser/child_process_launcher.cc:477: } else
nit: This seems like a consistent style guide violation: Lacking curlies around
the else clause as well as the if clause.  I know it is harder to read in this
current format.

You can possibly make this more consistent (without messing with indents) using
a pattern such as:

if () {
} else if () {
...
} else if () {
etc.

[ppi, 2013-11-06 20:14:06]

Thanks a lot Jim and apologies for the indent nits (one experiment too far with
the vimrc).

I'd appreciate if you could take another look at the
content/browser/child_process_launcher.cc bit - please see my reply below.

https://codereview.chromium.org/58203002/diff/320001/content/browser/android/...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/58203002/diff/320001/content/browser/android/...
content/browser/android/web_contents_observer_android.cc:64:
base::TerminationStatus termination_status) {
On 2013/11/06 19:27:26, jar wrote:
> nit: 4 character indent from previous line's "v" in void.

Done.

https://codereview.chromium.org/58203002/diff/320001/content/browser/android/...
File content/browser/android/web_contents_observer_android.h (right):

https://codereview.chromium.org/58203002/diff/320001/content/browser/android/...
content/browser/android/web_contents_observer_android.h:36:
base::TerminationStatus termination_status) OVERRIDE;
On 2013/11/06 19:27:26, jar wrote:
> nit: only use 4 character indent.  See line 40 for example.

Done.

https://codereview.chromium.org/58203002/diff/320001/content/browser/child_pr...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/58203002/diff/320001/content/browser/child_pr...
content/browser/child_process_launcher.cc:477: } else
On 2013/11/06 19:27:26, jar wrote:
> nit: This seems like a consistent style guide violation: Lacking curlies
around
> the else clause as well as the if clause.  I know it is harder to read in this
> current format.
> 
> You can possibly make this more consistent (without messing with indents)
using
> a pattern such as:
> 
> if () {
> } else if () {
> ...
> } else if () {
> etc.

I'm not sure if I see how this could work. We can't start each platform-specific
bit with "} else if (...) {", because it will be only one (platform-specific
#elif chunk) that will be compiled each time and it needs to start with "if ()
{".

I agree the current way looks confusing. I added braces to "else" clauses in
patch set 5, but I am not sure if it is easier to read that way.

[commit-bot: I haz the power, 2013-11-08 10:51:34]

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

[commit-bot: I haz the power, 2013-11-08 12:21:54]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-08 12:23:31]

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

[commit-bot: I haz the power, 2013-11-08 15:52:57]

Change committed as 233905