Implement Crash Handle API

android_webview/browser/aw_browser_terminator.cc

Index: android_webview/browser/aw_browser_terminator.cc
diff --git a/android_webview/browser/aw_browser_terminator.cc b/android_webview/browser/aw_browser_terminator.cc
index 362d070dc7c91050be096f97d171321be6ea963c..7265299fe40a287bd2d748b00a85442bd13ba6a3 100644
--- a/android_webview/browser/aw_browser_terminator.cc
+++ b/android_webview/browser/aw_browser_terminator.cc
@@ -6,6 +6,7 @@
 
 #include <unistd.h>
 
+#include "android_webview/browser/aw_render_process_gone_delegate.h"
 #include "android_webview/common/aw_descriptors.h"
 #include "base/bind.h"
 #include "base/logging.h"
@@ -16,11 +17,66 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_widget_host.h"
+#include "content/public/browser/render_widget_host_iterator.h"
+#include "content/public/browser/web_contents.h"
 
 using content::BrowserThread;
 
 namespace android_webview {
 
+namespace {
+
+void GetAwRenderProcessGoneDelegatesForRenderProcess(
+    int render_process_id,
+    std::vector<AwRenderProcessGoneDelegate*>* delegates) {
+  content::RenderProcessHost* rph =
+      content::RenderProcessHost::FromID(render_process_id);
+  if (!rph)
+    return;
+
+  std::unique_ptr<content::RenderWidgetHostIterator> widgets(
+      content::RenderWidgetHost::GetRenderWidgetHosts());
+  while (content::RenderWidgetHost* widget = widgets->GetNextHost()) {
+    content::RenderViewHost* view = content::RenderViewHost::From(widget);
+    if (view && rph == view->GetProcess()) {
+      content::WebContents* wc = content::WebContents::FromRenderViewHost(view);
+      if (wc) {
+        AwRenderProcessGoneDelegate* delegate =
+            AwRenderProcessGoneDelegate::FromWebContents(wc);
+        if (delegate)
+          delegates->push_back(delegate);
+      }
+    }
+  }
+}
+
+void OnRenderProcessGone(int child_process_id) {

[Tobias Sargeant, 2017/01/13 10:48:38]

I think it would be good to name this and the below in a less similar manner to
make it clear that this is providing a notification that the delegates will soon
receive the termination reason. Otherwise (to me, at least) it looks like these
are either/or methods.

Also, what guarantee do we have that the list of delegates that we get at this
point is identical to the list below? What if an AwContents is destroyed in the
meantime (or, more confusingly, created. will the new AwContents cause the
creation of a new renderer)?

[michaelbai, 2017/01/13 21:01:38]

Changed to OnRenderProcessGoneDetail(), and rewrote the comments.
No, there is no guarantee, change the description of Delegate class, basically,
AwContents is destroyed, then delegate is gone, there is no meaning to notify it
with detail information, new AwContents has new renderer,  it seems there is no
confusing here.

+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  std::vector<AwRenderProcessGoneDelegate*> delegates;
+  GetAwRenderProcessGoneDelegatesForRenderProcess(child_process_id, &delegates);
+  for (auto delegate : delegates)
+    delegate->OnRenderProcessGone(child_process_id);
+}
+
+void OnRenderProcessGoneWithDetail(int child_process_id, bool crashed) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  std::vector<AwRenderProcessGoneDelegate*> delegates;
+  GetAwRenderProcessGoneDelegatesForRenderProcess(child_process_id, &delegates);
+  for (auto delegate : delegates) {
+    if (!delegate->OnRenderProcessGoneWithDetail(child_process_id, crashed)) {
+      // Keeps this log unchanged, CTS test uses it to detect crash.
+      LOG(ERROR) << "Render process's abnormal termination wasn't handled by"
+                 << " all associated webviews, triggering application crash";
+      // For some reason, FATAL message didn't show up in adb logcat.

[Torne, 2017/01/13 13:24:08]

This normally works, I think; we should probably look into why, rather than just
doing this?

[michaelbai, 2017/01/13 21:01:38]

We shouldn't try to fix LOG issue in this patch, I filed a bug to track it.

[Torne, 2017/01/16 12:20:32]

We use LOG(FATAL) in other places and I'm pretty sure it works in general; I
think you should investigate this specific case and work out what's up rather
than just assuming this is broken.

[michaelbai, 2017/01/17 17:09:42]

Would you like point me where it still work? it will help me to figure out how
it is broken for this specific case. Thanks!

[michaelbai, 2017/01/18 03:01:26]

It seems Android O issue, the message showed up now.

+      LOG(FATAL) << "Crash";
+    }
+  }
+}
+
+}  // namespace
+
 AwBrowserTerminator::AwBrowserTerminator() {}
 
 AwBrowserTerminator::~AwBrowserTerminator() {}
@@ -42,18 +98,23 @@ void AwBrowserTerminator::OnChildStart(int child_process_id,
 }
 
 void AwBrowserTerminator::ProcessTerminationStatus(
+    int child_process_id,
     std::unique_ptr<base::SyncSocket> pipe) {
+  bool crashed = false;
+
+  // If the child process hasn't written anything into the pipe. This implies
+  // that it was terminated via SIGKILL by the low memory killer.
   if (pipe->Peek() >= sizeof(int)) {
     int exit_code;
     pipe->Receive(&exit_code, sizeof(exit_code));
-    LOG(FATAL) << "Renderer process crash detected (code " << exit_code
+    LOG(ERROR) << "Renderer process crash detected (code " << exit_code
                << "). Terminating browser.";

[Torne, 2017/01/13 13:24:08]

We're not terminating the browser necessarily any more, can we update this log
message?

[michaelbai, 2017/01/13 21:01:38]

Done.

-  } else {
-    // The child process hasn't written anything into the pipe. This implies
-    // that it was terminated via SIGKILL by the low memory killer, and thus we
-    // need to perform a clean exit.
-    exit(0);
+    crashed = true;
   }
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&OnRenderProcessGoneWithDetail, child_process_id, crashed));
 }
 
 void AwBrowserTerminator::OnChildExit(
@@ -62,6 +123,7 @@ void AwBrowserTerminator::OnChildExit(
     content::ProcessType process_type,
     base::TerminationStatus termination_status,
     base::android::ApplicationState app_state) {
+  OnRenderProcessGone(child_process_id);

[Tobias Sargeant, 2017/01/13 10:48:38]

Don't do this here; OnChildExit can be called more than once per child
(unavoidable because of the way the chrome child process notification system
works) so you need to move it to after the early-out when we don't find a pipe.

[michaelbai, 2017/01/13 21:01:38]

Thanks, it was added for experiment, I didn't think through it afterwards. 

   std::unique_ptr<base::SyncSocket> pipe;
 
   {
@@ -81,6 +143,7 @@ void AwBrowserTerminator::OnChildExit(
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&AwBrowserTerminator::ProcessTerminationStatus,
+                 child_process_id,
                  base::Passed(std::move(pipe))));
 }