Terminate process in onDestroy.

content/public/android/java/src/org/chromium/content/app/ChildProcessService.java

Index: content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
diff --git a/content/public/android/java/src/org/chromium/content/app/ChildProcessService.java b/content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
index 2898d423444fad86298329a9fdf2780367cb0972..e551aed667306c3d7ce24e059f0b8ea638d5c35c 100644
--- a/content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
+++ b/content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
@@ -184,23 +184,13 @@ public class ChildProcessService extends Service {
     public void onDestroy() {
         Log.i(TAG, "Destroying ChildProcessService pid=" + Process.myPid());
         super.onDestroy();
-        if (mCommandLineParams == null) {
-            // This process was destroyed before it even started. Nothing more to do.
-            return;
-        }
-        synchronized (mMainThread) {
-            try {
-                while (!mLibraryInitialized) {
-                    // Avoid a potential race in calling through to native code before the library
-                    // has loaded.
-                    mMainThread.wait();
-                }
-            } catch (InterruptedException e) {
-            }
-        }
-        // Try to shutdown the MainThread gracefully, but it might not
-        // have chance to exit normally.
-        nativeShutdownMainThread();
+
+        // The Android OS may resuse a process to start new service instances,
+        // but a renderer expects to start in a clean slate. Thus we need to
+        // reliably and quickly get rid of this process before there's a chance
+        // it get's recycled. See http://crbug.com/338709 as well as
+        // https://codereview.chromium.org/12288017
+        System.exit(0);

[bulach, 2014/02/03 21:57:40]

I can try to dig further from the android side, but System.exit is extremely
frowned upon...
it messes up with the activity life cycle, and in some cases it may trigger a
process restart..
don't quite know all the details, just saw some threads floating around.
I can dig it up if you want?

[ppi, 2014/11/07 14:16:35]

If System.exit(0) is frowned up... would this work with
Process.killProcess(Process.myPid()); ?

[Fredrik Öhrn, 2014/11/07 15:00:43]

I'm not sure exactly what supposedly gets messed up in the activity manager when
a service process exits. It does print a complaint in logcat and a notice about
restarting the process (the same message as for a regular crash) however I've
never seen it actually restart anything, presumably since the browser process
will (or have already) drop all bindings.

Digging into the Android OS code, System.exit will run exit hooks (both at the
Java and C library levels) if there are any registered, while calling
killProcess will send a SIGKILL which can't be caught so no hooks or other
cleanup will run.

I don't know if there are any activity manager related exit hooks, but if there
are I would assume it's friendlier to let them run. On the other hand
killProcess will be indistinguishable from a crash which the manager obviously
must be able to cope with. In short I think in theory System.exit might be
better, but in practice it probably doesn't matter, whatever goes "bad" when
terminating it probably happens inside the activity manager, not in the service
process.

It would be great if you could reach out to the right Android OS people and ask
them if there really is a problem with terminating the service process and if
yes, exactly what that problem is.

(It obviously can't be a big problem since we exit all the time from native code
when the IPC pipe is closed.)

[ppi, 2014/11/14 10:29:58]

Thanks for looking into it! Process.killProcess(Process.myPid()); seems a pretty
safe bet to me, given its equivalence to a process crash.

I relayed the discussion to android framework folks and pointed here, waiting
for a reply.

[ppi, 2014/12/12 19:23:06]

We had a discussion with the framework folks. Please find some notes below:

- processes for isolated services (like our renderers) are *not* reused for
subsequent services after the service is destroyed
- one hypothesis was that the problem we were seeing was caused by calling
unbind and then binding the service again in quick succession; as destroying the
service is asynchronous
- yet it seems that even in this case we shouldn't be able to run into trouble -
the activity manager service should always destroy the isolated process in
response to the last binding being removed and not reuse it
- it's hard to reason about the referenced crash - we were never able to
reproduce the crash locally; we only had crash reports that ceased to come after
we introduced the LRU ordering of services

Finally, the consensus was that given that the system guarantees that processes
for isolated services are not reused, killing the process ourselves would be a
workaround for an issue that we don't understand; and we probably shouldn't do
it.

I feel on the fence on this matter - I think I'd err on keeping the status quo
side until we hit the actual problem again. Wdyt?

Also adding Maria, who is taking over process management on our side.

     }
 
     @Override
@@ -300,6 +290,4 @@ public class ChildProcessService extends Service {
      * Force the child process to exit.
      */
     private static native void nativeExitChildProcess();
-
-    private native void nativeShutdownMainThread();
 }