Fix android crash reporting for child processes that exit cleanly

Some services that run in a utility process such as the Data Decoder
Service will shut themselves down cleanly if they do not have requests
for a period of time. These were being treated as OOM kills previously.

This uses the approach Bo outlined in http://crbug.com/872343.

Bug: 872343
Change-Id: Iaf371520893076981d44429a7ec1c444b637c93d
Reviewed-on: https://chromium-review.googlesource.com/c/1298024Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605768}

base/BUILD.gn

@@ -2239,6 +2239,7 @@ test("base_unittests") {
     "allocator/tcmalloc_unittest.cc",
     "android/android_image_reader_compat_unittest.cc",
     "android/application_status_listener_unittest.cc",
+    "android/child_process_unittest.cc",
     "android/content_uri_utils_unittest.cc",
     "android/jni_android_unittest.cc",
     "android/jni_array_unittest.cc",
@@ -3022,8 +3023,8 @@ if (is_android) {
   android_aidl("base_java_aidl") {
     import_include = [ "android/java/src" ]
     sources = [
-      "android/java/src/org/chromium/base/process_launcher/ICallbackInt.aidl",
       "android/java/src/org/chromium/base/process_launcher/IChildProcessService.aidl",
+      "android/java/src/org/chromium/base/process_launcher/IParentProcess.aidl",
     ]
   }
 

base/android/child_process_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/android/jni_android.h"
+#include "base/run_loop.h"
+#include "base/test/multiprocess_test.h"
+#include "base/test/test_timeouts.h"
+#include "testing/multiprocess_func_list.h"
+
+namespace base {
+namespace android {
+
+MULTIPROCESS_TEST_MAIN(BasicMain) {
+  return 0;
+}
+
+MULTIPROCESS_TEST_MAIN(WaitingMain) {
+  base::RunLoop().Run();
+  return 0;
+}
+
+class ChildProcessTest : public MultiProcessTest {};
+
+TEST_F(ChildProcessTest, ChildHasCleanExit) {
+  Process process = SpawnChild("BasicMain");
+  int exit_code = 0;
+  EXPECT_TRUE(WaitForMultiprocessTestChildExit(
+      process, TestTimeouts::action_timeout(), &exit_code));
+  EXPECT_EQ(exit_code, 0);
+  EXPECT_TRUE(MultiProcessTestChildHasCleanExit(process));
+}
+
+TEST_F(ChildProcessTest, ChildTerminated) {
+  Process process = SpawnChild("WaitingMain");
+  EXPECT_TRUE(TerminateMultiProcessTestChild(process, 0, true));
+  EXPECT_FALSE(MultiProcessTestChildHasCleanExit(process));
+}
+
+}  // namespace android
+}  // namespace base

base/android/java/src/org/chromium/base/process_launcher/ChildProcessConnection.java

@@ -255,6 +255,10 @@ public class ChildProcessConnection {
 
     private MemoryPressureCallback mMemoryPressureCallback;
 
+    // Whether the process exited cleanly or not.
+    @GuardedBy("sBindingStateLock")
+    private boolean mCleanExit;
+
     public ChildProcessConnection(Context context, ComponentName serviceName, boolean bindToCaller,
             boolean bindAsExternalService, Bundle serviceBundle) {
         this(context, serviceName, bindToCaller, bindAsExternalService, serviceBundle,
@@ -502,6 +506,10 @@ public class ChildProcessConnection {
     }
 
     private void onSetupConnectionResult(int pid) {
+        if (mPid != 0) {
+            Log.e(TAG, "sendPid was called more than once: pid=%d", mPid);
+            return;
+        }
         mPid = pid;
         assert mPid != 0 : "Child service claims to be run by a process of pid=0.";
 
@@ -522,9 +530,9 @@ public class ChildProcessConnection {
             assert mServiceConnectComplete && mService != null;
             assert mConnectionParams != null;
 
-            ICallbackInt pidCallback = new ICallbackInt.Stub() {
+            IParentProcess parentProcess = new IParentProcess.Stub() {
                 @Override
-                public void call(final int pid) {
+                public void sendPid(final int pid) {
                     mLauncherHandler.post(new Runnable() {
                         @Override
                         public void run() {
@@ -532,9 +540,22 @@ public class ChildProcessConnection {
                         }
                     });
                 }
+
+                @Override
+                public void reportCleanExit() {
+                    synchronized (sBindingStateLock) {
+                        mCleanExit = true;
+                    }
+                    mLauncherHandler.post(new Runnable() {
+                        @Override
+                        public void run() {
+                            unbind();
+                        }
+                    });
+                }
             };
             try {
-                mService.setupConnection(mConnectionParams.mConnectionBundle, pidCallback,
+                mService.setupConnection(mConnectionParams.mConnectionBundle, parentProcess,
                         mConnectionParams.mClientInterfaces);
             } catch (RemoteException re) {
                 Log.e(TAG, "Failed to setup connection.", re);
@@ -675,6 +696,15 @@ public class ChildProcessConnection {
         }
     }
 
+    /**
+     * @return true if the process exited cleanly.
+     */
+    public boolean hasCleanExit() {
+        synchronized (sBindingStateLock) {
+            return mCleanExit;
+        }
+    }
+
     /**
      * Returns the binding state of remaining processes, excluding the current connection.
      *

base/android/java/src/org/chromium/base/process_launcher/ChildProcessService.java

@@ -89,6 +89,9 @@ public abstract class ChildProcessService extends Service {
 
     private final Semaphore mActivitySemaphore = new Semaphore(1);
 
+    // Interface to send notifications to the parent process.
+    private IParentProcess mParentProcess;
+
     // These values are persisted to logs. Entries should not be renumbered and numeric values
     // should never be reused.
     @IntDef({SplitApkWorkaroundResult.NOT_RUN, SplitApkWorkaroundResult.NO_ENTRIES,
@@ -137,18 +140,19 @@ public abstract class ChildProcessService extends Service {
         }
 
         @Override
-        public void setupConnection(Bundle args, ICallbackInt pidCallback, List<IBinder> callbacks)
-                throws RemoteException {
+        public void setupConnection(Bundle args, IParentProcess parentProcess,
+                List<IBinder> callbacks) throws RemoteException {
             assert mServiceBound;
             synchronized (mBinderLock) {
                 if (mBindToCallerCheck && mBoundCallingPid == 0) {
                     Log.e(TAG, "Service has not been bound with bindToCaller()");
-                    pidCallback.call(-1);
+                    parentProcess.sendPid(-1);
                     return;
                 }
             }
 
-            pidCallback.call(Process.myPid());
+            parentProcess.sendPid(Process.myPid());
+            mParentProcess = parentProcess;
             processConnectionBundle(args, callbacks);
         }
 
@@ -272,6 +276,11 @@ public abstract class ChildProcessService extends Service {
                     }
                     if (mActivitySemaphore.tryAcquire()) {
                         mDelegate.runMain();
+                        try {
+                            mParentProcess.reportCleanExit();
+                        } catch (RemoteException e) {
+                            Log.e(TAG, "Failed to call clean exit callback.", e);
+                        }
                         nativeExitChildProcess();
                     }
                 } catch (InterruptedException e) {

base/android/java/src/org/chromium/base/process_launcher/ICallbackInt.aidl

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-package org.chromium.base.process_launcher;
-
-oneway interface ICallbackInt {
-    void call(int value);
-}

base/android/java/src/org/chromium/base/process_launcher/IChildProcessService.aidl

@@ -6,7 +6,7 @@ package org.chromium.base.process_launcher;
 
 import android.os.Bundle;
 
-import org.chromium.base.process_launcher.ICallbackInt;
+import org.chromium.base.process_launcher.IParentProcess;
 
 interface IChildProcessService {
   // On the first call to this method, the service will record the calling PID
@@ -15,8 +15,8 @@ interface IChildProcessService {
   boolean bindToCaller();
 
   // Sets up the initial IPC channel.
-  oneway void setupConnection(in Bundle args, ICallbackInt pidCallback,
-          in List<IBinder>  clientInterfaces);
+  oneway void setupConnection(in Bundle args, IParentProcess parentProcess,
+          in List<IBinder> clientInterfaces);
 
   // Forcefully kills the child process.
   oneway void forceKill();

base/android/java/src/org/chromium/base/process_launcher/IParentProcess.aidl

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base.process_launcher;
+
+interface IParentProcess {
+    // Sends the child pid to the parent process. This will be called before any
+    // third-party code is loaded, and will be a no-op after the first call.
+    oneway void sendPid(int pid);
+
+    // Tells the parent proces the child exited cleanly. Not oneway to ensure
+    // the browser receives the message before child exits.
+    void reportCleanExit();
+}

base/android/junit/src/org/chromium/base/process_launcher/ChildProcessConnectionTest.java

@@ -116,7 +116,7 @@ public class ChildProcessConnectionTest {
 
     // Parameters captured from the IChildProcessService.setupConnection() call
     private Bundle mConnectionBundle;
-    private ICallbackInt mConnectionPidCallback;
+    private IParentProcess mConnectionParentProcess;
     private IBinder mConnectionIBinderCallback;
 
     @Before
@@ -129,7 +129,7 @@ public class ChildProcessConnectionTest {
             @Override
             public Void answer(InvocationOnMock invocation) {
                 mConnectionBundle = (Bundle) invocation.getArgument(0);
-                mConnectionPidCallback = (ICallbackInt) invocation.getArgument(1);
+                mConnectionParentProcess = (IParentProcess) invocation.getArgument(1);
                 mConnectionIBinderCallback = (IBinder) invocation.getArgument(2);
                 return null;
             }
@@ -305,11 +305,29 @@ public class ChildProcessConnectionTest {
         verify(mConnectionCallback, never()).onConnected(any());
         mFirstServiceConnection.notifyServiceConnected(mChildProcessServiceBinder);
         ShadowLooper.runUiThreadTasks();
-        assertNotNull(mConnectionPidCallback);
-        mConnectionPidCallback.call(34 /* pid */);
+        assertNotNull(mConnectionParentProcess);
+        mConnectionParentProcess.sendPid(34);
         verify(mConnectionCallback, times(1)).onConnected(connection);
     }
 
+    @Test
+    public void testSendPidOnlyWorksOnce() throws RemoteException {
+        ChildProcessConnection connection = createDefaultTestConnection();
+        assertNotNull(mFirstServiceConnection);
+        connection.start(false /* useStrongBinding */, null /* serviceCallback */);
+        connection.setupConnection(
+                null /* connectionBundle */, null /* callback */, mConnectionCallback);
+        verify(mConnectionCallback, never()).onConnected(any());
+        mFirstServiceConnection.notifyServiceConnected(mChildProcessServiceBinder);
+        ShadowLooper.runUiThreadTasks();
+        assertNotNull(mConnectionParentProcess);
+
+        mConnectionParentProcess.sendPid(34);
+        assertEquals(34, connection.getPid());
+        mConnectionParentProcess.sendPid(543);
+        assertEquals(34, connection.getPid());
+    }
+
     @Test
     public void testSetupConnectionAfterServiceConnected() throws RemoteException {
         ChildProcessConnection connection = createDefaultTestConnection();
@@ -320,8 +338,8 @@ public class ChildProcessConnectionTest {
                 null /* connectionBundle */, null /* callback */, mConnectionCallback);
         verify(mConnectionCallback, never()).onConnected(any());
         ShadowLooper.runUiThreadTasks();
-        assertNotNull(mConnectionPidCallback);
-        mConnectionPidCallback.call(34 /* pid */);
+        assertNotNull(mConnectionParentProcess);
+        mConnectionParentProcess.sendPid(34);
         verify(mConnectionCallback, times(1)).onConnected(connection);
     }
 
@@ -335,8 +353,8 @@ public class ChildProcessConnectionTest {
                 null /* connectionBundle */, null /* callback */, mConnectionCallback);
         verify(mConnectionCallback, never()).onConnected(any());
         ShadowLooper.runUiThreadTasks();
-        assertNotNull(mConnectionPidCallback);
-        mConnectionPidCallback.call(34 /* pid */);
+        assertNotNull(mConnectionParentProcess);
+        mConnectionParentProcess.sendPid(34);
         verify(mConnectionCallback, times(1)).onConnected(connection);
 
         // Add strong binding so that connection is oom protected.

base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java

@@ -321,6 +321,26 @@ public final class MultiprocessTestClientLauncher {
         }
     }
 
+    @CalledByNative
+    private static boolean hasCleanExit(final int pid) {
+        return runOnLauncherAndGetResult(new Callable<Boolean>() {
+            @Override
+            public Boolean call() {
+                return hasCleanExitOnLauncherThread(pid);
+            }
+        });
+    }
+
+    private static boolean hasCleanExitOnLauncherThread(int pid) {
+        assert isRunningOnLauncherThread();
+
+        MultiprocessTestClientLauncher launcher = sPidToLauncher.get(pid);
+        if (launcher == null) {
+            return false;
+        }
+        return launcher.mLauncher.getConnection().hasCleanExit();
+    }
+
     /** Does not take ownership of of fds. */
     @CalledByNative
     private static FileDescriptorInfo[] makeFdInfoArray(int[] keys, int[] fds) {

base/test/multiprocess_test.h

@@ -81,6 +81,11 @@ bool TerminateMultiProcessTestChild(const Process& process,
                                     int exit_code,
                                     bool wait);
 
+#if defined(OS_ANDROID)
+// Returns whether the child process exited cleanly from the main runloop.
+bool MultiProcessTestChildHasCleanExit(const Process& process);
+#endif
+
 // MultiProcessTest ------------------------------------------------------------
 
 // A MultiProcessTest is a test class which makes it easier to

base/test/multiprocess_test_android.cc

@@ -83,4 +83,12 @@ bool TerminateMultiProcessTestChild(const Process& process,
       env, process.Pid(), exit_code, wait);
 }
 
+bool MultiProcessTestChildHasCleanExit(const Process& process) {
+  JNIEnv* env = android::AttachCurrentThread();
+  DCHECK(env);
+
+  return android::Java_MultiprocessTestClientLauncher_hasCleanExit(
+      env, process.Pid());
+}
+
 }  // namespace base

components/crash/content/browser/child_exit_observer_android.cc

@@ -171,6 +171,7 @@ void ChildExitObserver::BrowserChildProcessKilled(
   info.pid = data.GetProcess().Pid();
   info.process_type = static_cast<content::ProcessType>(data.process_type);
   info.app_state = base::android::ApplicationStatusListener::GetState();
+  info.normal_termination = content_info.clean_exit;
   PopulateTerminationInfo(content_info, &info);
   browser_child_process_info_.emplace(data.id, info);
   // Subsequent BrowserChildProcessHostDisconnected will call OnChildExit.

components/crash/content/browser/crash_metrics_reporter_android_unittest.cc

@@ -157,6 +157,22 @@ TEST_F(CrashMetricsReporterTest, UtilityProcessOOM) {
       nullptr);
 }
 
+TEST_F(CrashMetricsReporterTest, NormalTerminationIsNotOOMUtilityProcess) {
+  ChildExitObserver::TerminationInfo termination_info;
+  termination_info.process_host_id = 1;
+  termination_info.pid = base::kNullProcessHandle;
+  termination_info.process_type = content::PROCESS_TYPE_UTILITY;
+  termination_info.app_state =
+      base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES;
+  termination_info.normal_termination = true;
+  termination_info.binding_state = base::android::ChildBindingState::STRONG;
+  termination_info.was_killed_intentionally_by_browser = false;
+  termination_info.was_oom_protected_status = true;
+  termination_info.renderer_has_visible_clients = true;
+
+  TestOomCrashProcessing(termination_info, {}, nullptr);
+}
+
 TEST_F(CrashMetricsReporterTest, UtilityProcessAll) {
   ChildExitObserver::TerminationInfo termination_info;
   termination_info.process_host_id = 1;

content/browser/child_process_launcher_helper_android.cc

@@ -185,6 +185,7 @@ static void JNI_ChildProcessLauncherHelperImpl_SetTerminationInfo(
     jlong termination_info_ptr,
     jint binding_state,
     jboolean killed_by_us,
+    jboolean clean_exit,
     jint remaining_process_with_strong_binding,
     jint remaining_process_with_moderate_binding,
     jint remaining_process_with_waived_binding) {
@@ -193,6 +194,7 @@ static void JNI_ChildProcessLauncherHelperImpl_SetTerminationInfo(
   info->binding_state =
       static_cast<base::android::ChildBindingState>(binding_state);
   info->was_killed_intentionally_by_browser = killed_by_us;
+  info->clean_exit = clean_exit;
   info->remaining_process_with_strong_binding =
       remaining_process_with_strong_binding;
   info->remaining_process_with_moderate_binding =

content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelperImpl.java

@@ -413,8 +413,9 @@ public final class ChildProcessLauncherHelperImpl {
 
         int bindingCounts[] = connection.remainingBindingStateCountsCurrentOrWhenDied();
         nativeSetTerminationInfo(terminationInfoPtr, connection.bindingStateCurrentOrWhenDied(),
-                connection.isKilledByUs(), bindingCounts[ChildBindingState.STRONG],
-                bindingCounts[ChildBindingState.MODERATE], bindingCounts[ChildBindingState.WAIVED]);
+                connection.isKilledByUs(), connection.hasCleanExit(),
+                bindingCounts[ChildBindingState.STRONG], bindingCounts[ChildBindingState.MODERATE],
+                bindingCounts[ChildBindingState.WAIVED]);
     }
 
     @CalledByNative
@@ -644,6 +645,6 @@ public final class ChildProcessLauncherHelperImpl {
     }
 
     private static native void nativeSetTerminationInfo(long termiantionInfoPtr,
-            @ChildBindingState int bindingState, boolean killedByUs, int remainingStrong,
-            int remainingModerate, int remainingWaived);
+            @ChildBindingState int bindingState, boolean killedByUs, boolean cleanExit,
+            int remainingStrong, int remainingModerate, int remainingWaived);
 }

content/public/browser/child_process_termination_info.h

@@ -37,6 +37,9 @@ struct ChildProcessTerminationInfo {
   // True if child service was explicitly killed by browser.
   bool was_killed_intentionally_by_browser = false;
 
+  // True if the child shut itself down cleanly by quitting the main runloop.
+  bool clean_exit = false;
+
   // Counts of remaining child processes with corresponding binding.
   int remaining_process_with_strong_binding = 0;
   int remaining_process_with_moderate_binding = 0;