android: Delay removing binding

The effective process importance is computed from many inputs that can
change independently. An undesired case happens when one change causes a
binding to be dropped, but a closely-followed subsequent change adds the
binding back. This causes the process to temporarily drop the binding
which then can cause the OS to kill the process.

Use a herustic to delay calls to drop bindings to avoid situations like
this. To be conservative, only use apply this delay on the initial one
second of the process to avoid wider impact.

Bug: 1045059
Change-Id: I73462cf3c43140416871ef7abef3d77a419ca5a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031305Reviewed-by: ssid <ssid@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738324}

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

@@ -750,7 +750,6 @@ public class ChildProcessConnection {
     public void removeStrongBinding() {
         assert isRunningOnLauncherThread();
         if (!isConnected()) {
-            Log.w(TAG, "The connection is not bound for %d", getPid());
             return;
         }
         assert mStrongBindingCount > 0;
@@ -782,7 +781,6 @@ public class ChildProcessConnection {
     public void removeModerateBinding() {
         assert isRunningOnLauncherThread();
         if (!isConnected()) {
-            Log.w(TAG, "The connection is not bound for %d", getPid());
             return;
         }
         assert mModerateBindingCount > 0;

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

@@ -61,6 +61,14 @@ public final class ChildProcessLauncherHelperImpl {
     private static final String NUM_PRIVILEGED_SERVICES_KEY =
             "org.chromium.content.browser.NUM_PRIVILEGED_SERVICES";
 
+    // When decrementing the refcount on bindings, delay the decrement by this amount of time in
+    // case a new ref count is added in the mean time. This is a heuristic to avoid temporarily
+    // dropping bindings when inputs to calculating importance change independently.
+    private static final int REMOVE_BINDING_DELAY_MS = 500;
+
+    // To be conservative, only delay removing binding in the initial second of the process.
+    private static final int TIMEOUT_FOR_DELAY_BINDING_REMOVE_MS = 1000;
+
     // Flag to check features after native is initialized.
     private static boolean sCheckedFeatures;
 
@@ -183,6 +191,8 @@ public final class ChildProcessLauncherHelperImpl {
 
     private long mNativeChildProcessLauncherHelper;
 
+    private long mStartTimeMs;
+
     // This is the current computed importance from all the inputs from setPriority.
     // The initial value is MODERATE since a newly created connection has moderate bindings.
     private @ChildProcessImportance int mEffectiveImportance = ChildProcessImportance.MODERATE;
@@ -448,6 +458,7 @@ public final class ChildProcessLauncherHelperImpl {
 
     private void start() {
         mLauncher.start(true /* doSetupConnection */, true /* queueIfNoFreeConnection */);
+        mStartTimeMs = System.currentTimeMillis();
     }
 
     /**
@@ -552,19 +563,28 @@ public final class ChildProcessLauncherHelperImpl {
             if (mBindingManager != null) mBindingManager.rankingChanged();
         }
 
-        if (mEffectiveImportance != newEffectiveImportance) {
-            switch (mEffectiveImportance) {
-                case ChildProcessImportance.NORMAL:
-                    // Nothing to remove.
-                    break;
-                case ChildProcessImportance.MODERATE:
-                    connection.removeModerateBinding();
-                    break;
-                case ChildProcessImportance.IMPORTANT:
-                    connection.removeStrongBinding();
-                    break;
-                default:
-                    assert false;
+        if (mEffectiveImportance != newEffectiveImportance
+                && mEffectiveImportance != ChildProcessImportance.NORMAL) {
+            final int existingEffectiveImportance = mEffectiveImportance;
+            Runnable removeBindingRunnable = () -> {
+                switch (existingEffectiveImportance) {
+                    case ChildProcessImportance.NORMAL:
+                        // Nothing to remove.
+                        break;
+                    case ChildProcessImportance.MODERATE:
+                        connection.removeModerateBinding();
+                        break;
+                    case ChildProcessImportance.IMPORTANT:
+                        connection.removeStrongBinding();
+                        break;
+                    default:
+                        assert false;
+                }
+            };
+            if (System.currentTimeMillis() - mStartTimeMs < TIMEOUT_FOR_DELAY_BINDING_REMOVE_MS) {
+                LauncherThread.postDelayed(removeBindingRunnable, REMOVE_BINDING_DELAY_MS);
+            } else {
+                removeBindingRunnable.run();
             }
         }
 

content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java

@@ -25,6 +25,8 @@ import org.chromium.content_public.browser.RenderFrameHost;
 import org.chromium.content_public.browser.UiThreadTaskTraits;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.WebContentsStatics;
+import org.chromium.content_public.browser.test.util.Criteria;
+import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.content_shell.Shell;
 import org.chromium.content_shell_apk.ChildProcessLauncherTestUtils;
@@ -385,8 +387,14 @@ public class WebContentsTest {
         TestThreadUtils.runOnUiThreadBlocking(() -> webContents.onHide());
 
         final ChildProcessConnection connection = getSandboxedChildProcessConnection();
-        ChildProcessLauncherTestUtils.runOnLauncherThreadBlocking(
-                () -> Assert.assertFalse(connection.isModerateBindingBound()));
+        // Need to poll here because there is an intentional delay for removing binding.
+        CriteriaHelper.pollInstrumentationThread(new Criteria("Failed to remove moderate binding") {
+            @Override
+            public boolean isSatisfied() {
+                return ChildProcessLauncherTestUtils.runOnLauncherAndGetResult(
+                        () -> !connection.isModerateBindingBound());
+            }
+        });
 
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> webContents.setImportance(ChildProcessImportance.MODERATE));
@@ -407,8 +415,13 @@ public class WebContentsTest {
                 () -> Assert.assertTrue(connection.isStrongBindingBound()));
 
         TestThreadUtils.runOnUiThreadBlocking(() -> webContents.onHide());
-        ChildProcessLauncherTestUtils.runOnLauncherThreadBlocking(
-                () -> Assert.assertFalse(connection.isStrongBindingBound()));
+        CriteriaHelper.pollInstrumentationThread(new Criteria("Failed to remove strong binding") {
+            @Override
+            public boolean isSatisfied() {
+                return ChildProcessLauncherTestUtils.runOnLauncherAndGetResult(
+                        () -> !connection.isStrongBindingBound());
+            }
+        });
 
         TestThreadUtils.runOnUiThreadBlocking(() -> webContents.onShow());
         ChildProcessLauncherTestUtils.runOnLauncherThreadBlocking(