Add test to ensure shouldOverrideUrlLoading throws Java exception

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

Index: android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java
diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java b/android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java
new file mode 100644
index 0000000000000000000000000000000000000000..120be39d43c136a9c6acb68c110d2a29e1447c49
--- /dev/null
+++ b/android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java
@@ -0,0 +1,191 @@
+// Copyright 2016 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.android_webview.test;
+
+import android.annotation.SuppressLint;
+import android.content.ComponentName;
+import android.content.Intent;
+import android.content.ServiceConnection;
+import android.os.Bundle;
+import android.os.Handler;
+import android.os.HandlerThread;
+import android.os.IBinder;
+import android.os.Message;
+import android.os.Messenger;
+import android.os.Process;
+import android.os.RemoteException;
+import android.util.AndroidRuntimeException;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * This class provides functionality that lets you start a service and return some message from the
+ * service to a handler running on a separate thread in the test-process.
+ * NOTE: this class only supports waiting for one message sent from the separate service.
+ */
+public class SeparateServiceTestBase extends AwTestBase {
+    private static final String TAG = SeparateServiceTestBase.class.getSimpleName();

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

unused

+    private AtomicInteger mSeparateProcessPid;
+    // Messenger taking replies from the separate service
+    private Messenger mMessenger;
+    // Thread on which we handle replies from the separate service
+    private HandlerThread mSeparateProcessThread;
+    private CountDownLatch mMessageReceivedLatch;
+    private CountDownLatch mProcessPidMessageReceivedLatch;
+    private Handler.Callback mTestMessageHandler;
+
+    /**
+     * Ensure that we do not start WebView in this process - otherwise we will crash when trying to
+     * start WebView in another process.
+     */
+    @Override
+    protected boolean needsBrowserProcessStarted() {
+        return false;
+    }
+
+    @Override
+    protected void setUp() throws Exception {
+        super.setUp();
+        mSeparateProcessThread = new HandlerThread("SeparateWebViewProcessThread");

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

do you need a new thread? I think you don't actually do any blocking thing on
this thread, so just using the main thread should be fine, right? one less
thread to keep track of

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

Right, this thread just handles messages and unblocks the instrumentation
thread.

+        mSeparateProcessThread.start();
+        mMessenger = new Messenger(new TestHandler());
+        mSeparateProcessPid = new AtomicInteger();
+        mMessageReceivedLatch = new CountDownLatch(1);
+        mProcessPidMessageReceivedLatch = new CountDownLatch(1);
+    }
+
+    @Override
+    protected void tearDown() throws Exception {
+        mSeparateProcessThread.quitSafely();
+        mSeparateProcessThread = null;
+        mMessageReceivedLatch = null;
+        mProcessPidMessageReceivedLatch = null;
+        super.tearDown();
+    }
+
+    /**
+     * Starts a Service and then blocks the current thread until a response from the Service has
+     * been handled in the testCode callback.
+     * The Service will be running the code defined in serviceTestCode
+     * testCode is a callback in which we handle a response from the separate Service - this should
+     * return true if the response is handled and false otherwise.
+     */
+    protected void runServiceTestBlocking(ServiceTestRunner serviceTestCode,
+            Handler.Callback testCode, boolean unBindAndStopService) {
+        assertEquals(0, mSeparateProcessPid.get());
+
+        mTestMessageHandler = testCode;
+        Intent intent = new Intent(getActivity(), SeparateProcessWebViewService.class);
+
+        TestServiceConnection serviceConnection =
+                new TestServiceConnection(mMessenger, serviceTestCode, mMessageReceivedLatch);
+
+        // We start the service explicitly to ensure it keeps on running asynchronous tasks.
+        assertNotNull(getActivity().startService(intent));
+        try {
+            assertTrue(getActivity().bindService(intent, serviceConnection, 0));
+            try {
+                waitForServiceResponse();
+            } finally {
+                if (unBindAndStopService) {
+                    getActivity().unbindService(serviceConnection);
+                }
+            }
+        } finally {
+            if (unBindAndStopService) {
+                getActivity().stopService(intent);
+            }
+        }
+    }
+
+    /**
+     * ServiceConnection that sends a custom message and messenger to the given service when bound.
+     */
+    private static class TestServiceConnection implements ServiceConnection {
+        private Messenger mMessenger;
+        private ServiceTestRunner mServiceTestCode;
+        private CountDownLatch mMessageReceivedLatch;
+
+        public TestServiceConnection(Messenger messenger, ServiceTestRunner serviceTestCode,
+                CountDownLatch messageReceivedLatch) {
+            mMessenger = messenger;
+            mServiceTestCode = serviceTestCode;
+            mMessageReceivedLatch = messageReceivedLatch;
+        }
+
+        public void onServiceConnected(ComponentName className, IBinder service) {
+            Messenger serviceMessenger = new Messenger(service);
+            setupServiceMessaging(serviceMessenger);
+        }
+
+        public void onServiceDisconnected(ComponentName className) {
+            assertEquals("Service disconnected before we received a response", (long) 0,

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

0L

+                    mMessageReceivedLatch.getCount());
+        }
+
+        private void setupServiceMessaging(Messenger serviceMessenger) {
+            Message msg = Message.obtain();
+            msg.replyTo = mMessenger;
+            Bundle data = new Bundle();
+            data.putSerializable(
+                    SeparateProcessWebViewService.TEST_CODE_BUNDLE_TAG, mServiceTestCode);
+            msg.setData(data);
+            try {
+                serviceMessenger.send(msg);
+            } catch (RemoteException e) {
+                throw new AndroidRuntimeException(e);
+            }
+        }
+    };
+
+    // TODO do we care about handler leaks in a test class?

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

no

+    @SuppressLint("HandlerLeak")
+    private class TestHandler extends Handler {
+        public TestHandler() {
+            super(mSeparateProcessThread.getLooper(), new Handler.Callback() {
+                @Override
+                public boolean handleMessage(Message message) {
+                    if (message.what == SeparateProcessWebViewService.PROCESS_PID_MESSAGE_ID) {
+                        mSeparateProcessPid.set(message.arg1);
+                        mProcessPidMessageReceivedLatch.countDown();

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

It's really odd that we are the one starting a service process, and we have to
ask the service process what its pid is. There should be a unspoofable way to
get that without having the service to provide it, surely..

Actually, looking at how ChildProcessConnection in clank works, it uses an aidl
interface over binder, but it basically just asks the service for its pid too.
o_O how's that safe...?!

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

Oooooh, you mean in case the child decides to return the wrong pid?

[Torne, 2016/09/15 10:26:43]

Not really; Android doesn't really think there's any reason to care what the pid
of a service you're bound to is; it's none of  your business, and in the general
case (not your own package) it will be running as a different UID so you can't
use the pid for anything anyway.
It's safe because it happens at startup, before the service has loaded any
content.

[boliu, 2016/09/15 16:47:51]

Yeah that's sort of what I assumed too. Still super brittle though. meh

+                        return true;
+                    }
+                    if (mTestMessageHandler.handleMessage(message)) {
+                        mMessageReceivedLatch.countDown();

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

I'm confused what the return value of mTestMessageHandler.handleMessage means

do you mean for returning true to mean end the test? but then it's not possible
to encode unhandled vs handled but don't mark test as done.

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

Yeah, this implementation is assuming that you either 1. don't handle a message,
or 2. you handle it and then the test finishes.
I guess you could just change the interface to return a value representing more
state than a boolean but the current tests all finish after one message so I
didn't see a reason for making this change currently (especially since, as you
pointed out, it is not clear whether we want to support this test :/).

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

Another option is you can give the latch up to the test cast and let it decide
when to unblock.

+                        return true;
+                    }
+                    return false;
+                }
+            });
+        }
+    }
+
+    protected void waitForServiceResponse() {
+        try {
+            assertTrue(mProcessPidMessageReceivedLatch.await(
+                    AwTestBase.WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+            assertTrue(
+                    mMessageReceivedLatch.await(AwTestBase.WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+        } catch (InterruptedException e) {
+        }
+    }
+
+    protected void killSeparateProcess() {
+        if (mSeparateProcessPid.get() <= 0)
+            throw new AndroidRuntimeException("Separate browser pid less than 0 :(");
+        Process.killProcess(mSeparateProcessPid.get());
+    }
+
+    protected void resetSeparateProcessPid() {
+        mSeparateProcessPid.set(0);
+    }
+
+    protected int getSeparateProcessPid() {
+        assertEquals(0, mProcessPidMessageReceivedLatch.getCount());
+        return mSeparateProcessPid.get();
+    }
+}