Add test to ensure shouldOverrideUrlLoading throws Java exception

android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java

Index: android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java
diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java
index d6366358b075700f7067328bb71bfea30c0b4e31..009fe2d8e490f153c2e8e04ea2c307fd52dbfddb 100644
--- a/android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java
+++ b/android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java
@@ -5,35 +5,23 @@
 package org.chromium.android_webview.test;
 
 import android.app.ActivityManager;
-import android.content.ComponentName;
 import android.content.Context;
-import android.content.Intent;
-import android.content.ServiceConnection;
-import android.os.IBinder;
-import android.os.Parcel;
+import android.os.Handler;
+import android.os.Message;
 import android.os.Process;
-import android.os.RemoteException;
 
 import org.chromium.android_webview.AwBrowserProcess;
 import org.chromium.base.test.util.DisabledTest;
 
 import java.util.concurrent.Callable;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
 
 /**
  * Tests to ensure that it is impossible to launch two browser processes within
  * the same application. Chromium is not designed for that, and attempting to do that
  * can cause data files corruption.
  */
-public class AwSecondBrowserProcessTest extends AwTestBase {
-    private CountDownLatch mSecondBrowserProcessLatch;
-    private int mSecondBrowserServicePid;
-
-    @Override
-    protected boolean needsBrowserProcessStarted() {
-        return false;
-    }
+public class AwSecondBrowserProcessTest extends SeparateServiceTestBase {
+    private static final String TAG = AwSecondBrowserProcessTest.class.getSimpleName();

[boliu, 2016/09/15 04:57:52]

this is not used

 
     @Override
     protected void tearDown() throws Exception {
@@ -48,7 +36,7 @@ public class AwSecondBrowserProcessTest extends AwTestBase {
      * process succeeds either, because in debug it will crash due to an assert
      * in the SQL DB code.
      */
-    @DisabledTest(message = "crbug.com/582146")
+    @DisabledTest

[boliu, 2016/09/15 04:57:53]

don't drop the message

also can you make sure these two tests still works ok? I think this one is
disabled because we actually have to make the crash a warning instead

[gsennton, 2016/09/15 10:02:04]

Both of these fail in the assertFalse(tryStartingBrowserProcess()) statement
since (as you said) the crash we are looking for is just a log message now. This
is true before and after my change.

[boliu, 2016/09/15 18:04:04]

Can you turn on the crash and make sure the tests pass? and repeat like 20 times
to make sure they are not flaky? (I guess do that when everything else in this
CL is ready, since things are still changing it looks like)

     public void testCreatingSecondBrowserProcessFails() throws Throwable {
         startSecondBrowserProcess();
         assertFalse(tryStartingBrowserProcess());
@@ -66,45 +54,32 @@ public class AwSecondBrowserProcessTest extends AwTestBase {
         assertTrue(tryStartingBrowserProcess());
     }
 
-    private final ServiceConnection mConnection = new ServiceConnection() {
-        @Override
-        public void onServiceConnected(ComponentName className, IBinder service) {
-            Parcel result = Parcel.obtain();
-            try {
-                assertTrue(service.transact(
-                                SecondBrowserProcess.CODE_START, Parcel.obtain(), result, 0));
-            } catch (RemoteException e) {
-                fail("RemoteException: " + e);
-            }
-            result.readException();
-            mSecondBrowserServicePid = result.readInt();
-            assertTrue(mSecondBrowserServicePid > 0);
-            mSecondBrowserProcessLatch.countDown();
-        }
-
-        @Override
-        public void onServiceDisconnected(ComponentName className) {
-        }
-    };
-
     private void startSecondBrowserProcess() throws Exception {
-        Context context = getActivity();
-        Intent intent = new Intent(context, SecondBrowserProcess.class);
-        mSecondBrowserProcessLatch = new CountDownLatch(1);
-        assertNotNull(context.startService(intent));
-        assertTrue(context.bindService(intent, mConnection, 0));
-        assertTrue(mSecondBrowserProcessLatch.await(
-                        AwTestBase.WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS));
-        mSecondBrowserProcessLatch = null;
+        runServiceTestBlocking(new SecondBrowserProcessTestRunner(), new Handler.Callback() {
+            @Override
+            public boolean handleMessage(Message message) {
+                switch (message.what) {
+                    case SecondBrowserProcessTestRunner.RETURN_TEST_SUCCESS:
+                        return true;
+                    case SecondBrowserProcessTestRunner.RETURN_TEST_FAILURE:
+                        fail("Something went wrong in separate Service, see logs for info");
+                        return true;
+                    default:
+                        fail("Received an unexpected service response " + message);
+                        return false;
+                }
+            }
+        }, false /* unBindAndStopService */);

[boliu, 2016/09/15 04:57:52]

hmmmm, this unBindAndStopService thing kinda smells bad, since spreading clean
up code is generally not a good idea

you can refactor this test to execute code while the service is up by doing
adding code to handler above when a message is received

     }
 
     private void stopSecondBrowserProcess(boolean sync) throws Exception {
-        if (mSecondBrowserServicePid <= 0) return;
+        int separateProcessPid = getSeparateProcessPid();
+        if (separateProcessPid <= 0) return;
         assertTrue(isSecondBrowserServiceRunning());
         // Note that using killProcess ensures that the service record gets removed
         // from ActivityManager after the process has actually died. While using
         // Context.stopService would result in the opposite outcome.
-        Process.killProcess(mSecondBrowserServicePid);
+        Process.killProcess(separateProcessPid);
         if (sync) {
             pollInstrumentationThread(new Callable<Boolean>() {
                 @Override
@@ -113,7 +88,7 @@ public class AwSecondBrowserProcessTest extends AwTestBase {
                 }
             });
         }
-        mSecondBrowserServicePid = 0;
+        resetSeparateProcessPid();
     }
 
     private boolean tryStartingBrowserProcess() {
@@ -142,8 +117,9 @@ public class AwSecondBrowserProcessTest extends AwTestBase {
     private boolean isSecondBrowserServiceRunning() {
         ActivityManager activityManager =
                 (ActivityManager) getActivity().getSystemService(Context.ACTIVITY_SERVICE);
+        int separateProcessPid = getSeparateProcessPid();
         for (ActivityManager.RunningServiceInfo si : activityManager.getRunningServices(65536)) {
-            if (si.pid == mSecondBrowserServicePid) return true;
+            if (si.pid == separateProcessPid) return true;
         }
         return false;
     }