android: Retry bind if timeout

Unbind and rebind if onServiceConnection does not come after a timeout,
currently set to 10s. Add a unit test, but do not enable this behavior
in production yet.

The current plan is then to add some UMA metrics and evaluate result
to decide whether it's worth turning this on or not.

BUG=736066

Change-Id: Id06b11005c781932810b32e11592727f580ef38c
Reviewed-on: https://chromium-review.googlesource.com/562653
Commit-Queue: Bo Liu <boliu@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485537}

content/public/android/BUILD.gn

@@ -474,6 +474,7 @@ junit_binary("content_junit_tests") {
   java_files = [
     "junit/src/org/chromium/content/browser/BindingManagerImplTest.java",
     "junit/src/org/chromium/content/browser/ChildConnectionAllocatorTest.java",
+    "junit/src/org/chromium/content/browser/ChildProcessConnectionTest.java",
     "junit/src/org/chromium/content/browser/MenuDescriptorTest.java",
     "junit/src/org/chromium/content/browser/SpareChildConnectionTest.java",
     "junit/src/org/chromium/content/browser/TestChildProcessConnection.java",

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

@@ -241,7 +241,7 @@ public class ChildConnectionAllocator {
             listener.onConnectionAllocated(this, connection);
         }
 
-        connection.start(mUseStrongBinding, serviceCallbackWrapper);
+        connection.start(mUseStrongBinding, serviceCallbackWrapper, false /* retryOnTimeout */);
         Log.d(TAG, "Allocator allocated and bound a connection, name: %s, slot: %d",
                 mServiceClassName, slot);
         return connection;

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

@@ -27,6 +27,8 @@ import javax.annotation.Nullable;
 public class ChildProcessConnection {
     private static final String TAG = "ChildProcessConn";
 
+    private static final int BIND_SERVICE_TIMEOUT_IN_MS = 10 * 1000;
+
     /**
      * Used to notify the consumer about the process start. These callbacks will be invoked before
      * the ConnectionCallbacks.
@@ -177,6 +179,11 @@ public class ChildProcessConnection {
     // call.
     private ConnectionCallback mConnectionCallback;
 
+    // Workaround bug on some android versions where bindService does not result in
+    // onServiceConnected for sandboxed services; see crbug.com/736066 for details.
+    // This is a delayed callback that will retry bindService with a delay.
+    private Runnable mOnServiceConnectedWatchDog;
+
     private IChildProcessService mService;
 
     // Set to true when the service connection callback runs. This differs from
@@ -290,7 +297,8 @@ public class ChildProcessConnection {
      * @param serviceCallback (optional) callbacks invoked when the child process starts or fails to
      * start and when the service stops.
      */
-    public void start(boolean useStrongBinding, ServiceCallback serviceCallback) {
+    public void start(
+            boolean useStrongBinding, ServiceCallback serviceCallback, boolean retryOnTimeout) {
         assert LauncherThread.runningOnLauncherThread();
         try {
             TraceEvent.begin("ChildProcessConnection.start");
@@ -300,8 +308,10 @@ public class ChildProcessConnection {
 
             mServiceCallback = serviceCallback;
 
+            resetWatchdog(useStrongBinding, serviceCallback, retryOnTimeout);
             if (!bind(useStrongBinding)) {
                 Log.e(TAG, "Failed to establish the service connection.");
+                cancelWatchDog();
                 // We have to notify the caller so that they can free-up associated resources.
                 // TODO(ppi): Can we hard-fail here?
                 notifyChildProcessDied();
@@ -349,14 +359,17 @@ public class ChildProcessConnection {
      */
     public void stop() {
         assert LauncherThread.runningOnLauncherThread();
+        cancelWatchDog();
         unbind();
         mService = null;
         mConnectionParams = null;
         notifyChildProcessDied();
     }
 
-    private void onServiceConnectedOnLauncherThread(IBinder service) {
+    @VisibleForTesting
+    public void onServiceConnectedOnLauncherThread(IBinder service) {
         assert LauncherThread.runningOnLauncherThread();
+        cancelWatchDog();
         // A flag from the parent class ensures we run the post-connection logic only once
         // (instead of once per each ChildServiceConnection).
         if (mDidOnServiceConnected) {
@@ -403,7 +416,8 @@ public class ChildProcessConnection {
         }
     }
 
-    private void onServiceDisconnectedOnLauncherThread() {
+    @VisibleForTesting
+    public void onServiceDisconnectedOnLauncherThread() {
         assert LauncherThread.runningOnLauncherThread();
         // Ensure that the disconnection logic runs only once (instead of once per each
         // ChildServiceConnection).
@@ -482,12 +496,16 @@ public class ChildProcessConnection {
     protected void unbind() {
         assert LauncherThread.runningOnLauncherThread();
         mUnbound = true;
+        unbindAll();
+        // Note that we don't update the waived bound only state here as to preserve the state when
+        // disconnected.
+    }
+
+    private void unbindAll() {
         mStrongBinding.unbind();
         mWaivedBinding.unbind();
         mModerateBinding.unbind();
         mInitialBinding.unbind();
-        // Note that we don't update the waived bound only state here as to preserve the state when
-        // disconnected.
     }
 
     public boolean isInitialBindingBound() {
@@ -603,6 +621,34 @@ public class ChildProcessConnection {
         }
     }
 
+    private void resetWatchdog(final boolean useStrongBinding,
+            final ServiceCallback serviceCallback, final boolean retryOnTimeout) {
+        assert LauncherThread.runningOnLauncherThread();
+        cancelWatchDog();
+        assert mOnServiceConnectedWatchDog == null;
+        mOnServiceConnectedWatchDog = new Runnable() {
+            @Override
+            public void run() {
+                assert mOnServiceConnectedWatchDog == this;
+                assert !mDidOnServiceConnected;
+                assert mServiceCallback == null;
+                mOnServiceConnectedWatchDog = null;
+                // TODO(boliu): Add a UMA here.
+                if (!retryOnTimeout) return;
+                unbindAll();
+                start(useStrongBinding, serviceCallback, retryOnTimeout);
+            }
+        };
+        LauncherThread.postDelayed(mOnServiceConnectedWatchDog, BIND_SERVICE_TIMEOUT_IN_MS);
+    }
+
+    private void cancelWatchDog() {
+        assert LauncherThread.runningOnLauncherThread();
+        if (mOnServiceConnectedWatchDog == null) return;
+        LauncherThread.removeCallbacks(mOnServiceConnectedWatchDog);
+        mOnServiceConnectedWatchDog = null;
+    }
+
     @VisibleForTesting
     protected ChildServiceConnection createServiceConnection(int bindFlags) {
         assert LauncherThread.runningOnLauncherThread();
@@ -622,4 +668,9 @@ public class ChildProcessConnection {
         return new ChildProcessConnection(context, serviceName, bindAsExternalService,
                 serviceBundle, creationParams, false /* doBind */);
     }
+
+    @VisibleForTesting
+    public boolean didOnServiceConnectedForTesting() {
+        return mDidOnServiceConnected;
+    }
 }

content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java

@@ -357,7 +357,8 @@ public class ChildProcessLauncherTest {
                                                     connection);
                                         }
                                     }
-                                });
+                                },
+                                false /* retryOnTimeout */);
                         return connection;
                     }
                 });

content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java

@@ -49,7 +49,8 @@ public class BindingManagerImplTest {
                 new ComponentName(packageName, "TestService"), false /* bindAsExternalService */,
                 null /* serviceBundle */, creationParams);
         connection.setPid(pid);
-        connection.start(false /* useStrongBinding */, null /* serviceCallback */);
+        connection.start(false /* useStrongBinding */, null /* serviceCallback */,
+                false /* retryOnTimeout */);
         if (manager != null) {
             manager.addNewConnection(pid, connection);
         }

content/public/android/junit/src/org/chromium/content/browser/ChildConnectionAllocatorTest.java

@@ -74,7 +74,8 @@ public class ChildConnectionAllocatorTest {
                     }
                 })
                         .when(mConnection)
-                        .start(anyBoolean(), any(ChildProcessConnection.ServiceCallback.class));
+                        .start(anyBoolean(), any(ChildProcessConnection.ServiceCallback.class),
+                                anyBoolean());
             }
             return mConnection;
         }
@@ -109,7 +110,8 @@ public class ChildConnectionAllocatorTest {
                 }
             })
                     .when(mConnection)
-                    .start(anyBoolean(), any(ChildProcessConnection.ServiceCallback.class));
+                    .start(anyBoolean(), any(ChildProcessConnection.ServiceCallback.class),
+                            anyBoolean());
         }
 
         public void simulateServiceProcessDying() {
@@ -161,7 +163,7 @@ public class ChildConnectionAllocatorTest {
 
         verify(connection, times(1))
                 .start(eq(false) /* useStrongBinding */,
-                        any(ChildProcessConnection.ServiceCallback.class));
+                        any(ChildProcessConnection.ServiceCallback.class), anyBoolean());
         verify(listener, times(1)).onConnectionAllocated(mAllocator, connection);
         assertTrue(mAllocator.anyConnectionAllocated());
     }
@@ -199,7 +201,8 @@ public class ChildConnectionAllocatorTest {
             allocator.setConnectionFactoryForTesting(mTestConnectionFactory);
             ChildProcessConnection connection = allocator.allocate(
                     null /* context */, null /* serviceBundle */, mServiceCallback);
-            verify(connection, times(0)).start(useStrongBinding, mServiceCallback);
+            verify(connection, times(0))
+                    .start(useStrongBinding, mServiceCallback, false /* retryOnTimeout */);
         }
     }
 
@@ -262,7 +265,7 @@ public class ChildConnectionAllocatorTest {
         assertNotNull(connection);
         verify(connection, times(1))
                 .start(eq(false) /* useStrongBinding */,
-                        any(ChildProcessConnection.ServiceCallback.class));
+                        any(ChildProcessConnection.ServiceCallback.class), anyBoolean());
         assertTrue(mAllocator.anyConnectionAllocated());
 
         mTestConnectionFactory.simulateServiceProcessDying();

content/public/android/junit/src/org/chromium/content/browser/ChildProcessConnectionTest.java

+// 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.content.browser;
+
+import android.content.ComponentName;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.annotation.Config;
+import org.robolectric.shadows.ShadowLooper;
+
+import org.chromium.testing.local.LocalRobolectricTestRunner;
+
+/** Unit tests for ChildProcessConnection. */
+@RunWith(LocalRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class ChildProcessConnectionTest {
+    @Before
+    public void setUp() {
+        // The tests run on only one thread. Pretend that is the launcher thread so LauncherThread
+        // asserts are not triggered.
+        LauncherThread.setCurrentThreadAsLauncherThread();
+    }
+
+    @After
+    public void tearDown() {
+        LauncherThread.setLauncherThreadAsLauncherThread();
+    }
+
+    private TestChildProcessConnection createTestConnection() {
+        String packageName = "org.chromium.test";
+        String serviceName = "TestService";
+        return new TestChildProcessConnection(new ComponentName(packageName, serviceName),
+                false /* bindAsExternalService */, null /* serviceBundle */,
+                null /* creationParams */);
+    }
+
+    @Test
+    public void testWatchdog() {
+        TestChildProcessConnection connection = createTestConnection();
+        connection.setPostOnServiceConnected(false);
+
+        connection.start(false /* useStrongBinding */, null /* serviceCallback */,
+                true /* retryOnTimeout */);
+        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        Assert.assertTrue(connection.isInitialBindingBound());
+        Assert.assertFalse(connection.didOnServiceConnectedForTesting());
+
+        connection.setPostOnServiceConnected(true);
+        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        Assert.assertTrue(connection.isInitialBindingBound());
+        Assert.assertTrue(connection.didOnServiceConnectedForTesting());
+    }
+
+    @Test
+    public void testWatchdogDisabled() {
+        TestChildProcessConnection connection = createTestConnection();
+        connection.setPostOnServiceConnected(false);
+
+        connection.start(false /* useStrongBinding */, null /* serviceCallback */,
+                false /* retryOnTimeout */);
+        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        Assert.assertTrue(connection.isInitialBindingBound());
+        Assert.assertFalse(connection.didOnServiceConnectedForTesting());
+
+        connection.setPostOnServiceConnected(true);
+        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        Assert.assertTrue(connection.isInitialBindingBound());
+        // No retry, so service is still not connected.
+        Assert.assertFalse(connection.didOnServiceConnectedForTesting());
+    }
+
+    @Test
+    public void testWatchdogCancelled() {
+        TestChildProcessConnection connection = createTestConnection();
+        connection.start(false /* useStrongBinding */, null /* serviceCallback */,
+                true /* retryOnTimeout */);
+        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        Assert.assertTrue(connection.isInitialBindingBound());
+        Assert.assertTrue(connection.didOnServiceConnectedForTesting());
+
+        connection.setPostOnServiceConnected(false);
+        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        // Did not attempt to rebind.
+        Assert.assertTrue(connection.isInitialBindingBound());
+    }
+}

content/public/android/junit/src/org/chromium/content/browser/SpareChildConnectionTest.java

@@ -57,6 +57,7 @@ public class SpareChildConnectionTest {
             assert mConnection == null;
             mConnection = new TestChildProcessConnection(
                     serviceName, bindAsExternalService, serviceBundle, creationParams);
+            mConnection.setPostOnServiceConnected(false);
             return mConnection;
         }
 
@@ -132,7 +133,7 @@ public class SpareChildConnectionTest {
         ChildProcessConnection connection =
                 mSpareConnection.getConnection(mWrongConnectionAllocator, mServiceCallback);
         assertNull(connection);
-        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        ShadowLooper.runUiThreadTasks();
         verify(mServiceCallback, times(0)).onChildStarted();
         verify(mServiceCallback, times(0)).onChildStartFailed();
         verify(mServiceCallback, times(0)).onChildProcessDied(any());
@@ -153,7 +154,7 @@ public class SpareChildConnectionTest {
         // No more connections are available.
         assertTrue(mSpareConnection.isEmpty());
 
-        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        ShadowLooper.runUiThreadTasks();
         verify(mServiceCallback, times(1)).onChildStarted();
         verify(mServiceCallback, times(0)).onChildStartFailed();
     }
@@ -167,7 +168,7 @@ public class SpareChildConnectionTest {
         ChildProcessConnection connection =
                 mSpareConnection.getConnection(mConnectionAllocator, mServiceCallback);
         assertNotNull(connection);
-        ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
+        ShadowLooper.runUiThreadTasks();
         // No callbacks are called.
         verify(mServiceCallback, times(0)).onChildStarted();
         verify(mServiceCallback, times(0)).onChildStartFailed();

content/public/android/junit/src/org/chromium/content/browser/TestChildProcessConnection.java

@@ -6,17 +6,49 @@ package org.chromium.content.browser;
 
 import android.content.ComponentName;
 import android.os.Bundle;
+import android.os.IBinder;
 
 import org.chromium.base.process_launcher.ChildProcessCreationParams;
+import org.chromium.base.process_launcher.ICallbackInt;
+import org.chromium.base.process_launcher.IChildProcessService;
 
 /** An implementation of ChildProcessConnection that does not connect to a real service. */
 class TestChildProcessConnection extends ChildProcessConnection {
-    private static class MockChildServiceConnection
+    private static class MockServiceBinder extends IChildProcessService.Stub {
+        @Override
+        public boolean bindToCaller() {
+            return true;
+        }
+
+        @Override
+        public void setupConnection(Bundle args, ICallbackInt pidCallback, IBinder gpuCallback) {}
+
+        @Override
+        public void crashIntentionallyForTesting() {
+            throw new RuntimeException("crashIntentionallyForTesting");
+        }
+    }
+
+    private class MockChildServiceConnection
             implements ChildProcessConnection.ChildServiceConnection {
+        private final ChildProcessConnection mConnection;
         private boolean mBound;
 
+        MockChildServiceConnection(ChildProcessConnection connection) {
+            mConnection = connection;
+        }
+
         @Override
         public boolean bind() {
+            if (TestChildProcessConnection.this.mPostOnServiceConnected) {
+                LauncherThread.post(new Runnable() {
+                    @Override
+                    public void run() {
+                        // TODO(boliu): implement a dummy service.
+                        mConnection.onServiceConnectedOnLauncherThread(new MockServiceBinder());
+                    }
+                });
+            }
             mBound = true;
             return true;
         }
@@ -35,6 +67,7 @@ class TestChildProcessConnection extends ChildProcessConnection {
     private int mPid;
     private boolean mConnected;
     private ServiceCallback mServiceCallback;
+    private boolean mPostOnServiceConnected;
 
     /**
      * Creates a mock binding corresponding to real ManagedChildProcessConnection after the
@@ -44,6 +77,7 @@ class TestChildProcessConnection extends ChildProcessConnection {
             Bundle serviceBundle, ChildProcessCreationParams creationParams) {
         super(null /* context */, serviceName, bindAsExternalService, serviceBundle,
                 creationParams);
+        mPostOnServiceConnected = true;
     }
 
     public void setPid(int pid) {
@@ -57,13 +91,14 @@ class TestChildProcessConnection extends ChildProcessConnection {
 
     @Override
     protected ChildServiceConnection createServiceConnection(int bindFlags) {
-        return new MockChildServiceConnection();
+        return new MockChildServiceConnection(this);
     }
 
     // We don't have a real service so we have to mock the connection status.
     @Override
-    public void start(boolean useStrongBinding, ServiceCallback serviceCallback) {
-        super.start(useStrongBinding, serviceCallback);
+    public void start(
+            boolean useStrongBinding, ServiceCallback serviceCallback, boolean retryOnTimeout) {
+        super.start(useStrongBinding, serviceCallback, retryOnTimeout);
         mConnected = true;
         mServiceCallback = serviceCallback;
     }
@@ -82,4 +117,8 @@ class TestChildProcessConnection extends ChildProcessConnection {
     public ServiceCallback getServiceCallback() {
         return mServiceCallback;
     }
+
+    public void setPostOnServiceConnected(boolean post) {
+        mPostOnServiceConnected = post;
+    }
 }