Add test to ensure shouldOverrideUrlLoading throws Java exception

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

Index: android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java
diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java
new file mode 100644
index 0000000000000000000000000000000000000000..996165c8c8f1d0ef18811f4f8c59144dbb2d2768
--- /dev/null
+++ b/android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java
@@ -0,0 +1,116 @@
+// 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.content.ComponentName;
+import android.content.Intent;
+import android.content.ServiceConnection;
+import android.os.Handler;
+import android.os.IBinder;
+import android.os.Message;
+import android.os.Messenger;
+import android.os.RemoteException;
+import android.test.suitebuilder.annotation.SmallTest;
+import android.util.AndroidRuntimeException;
+
+import org.chromium.android_webview.AwContents;
+import org.chromium.base.ThreadUtils;
+import org.chromium.base.test.util.Feature;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Test class ensuring we handle Java exceptions thrown in WebView callbacks correctly.
+ */
+public class JniCrashTest extends AwTestBase {
+    private static final String DATA_URL = "data:text/html,<div/>";
+    private static final String TAG = JniCrashTest.class.getSimpleName();
+
+    private AwTestContainerView mTestContainerView;
+    private TestAwContentsClient mContentsClient;
+    private AwContents mAwContents;
+    private CountDownLatch mUnhandledExceptionLatch;
+
+    private static class MySpecialException extends RuntimeException {
+        MySpecialException() {
+            super();
+        }
+    }
+
+    /**
+     * Ensure that we do not start WebView in this process - otherwise we will crash when trying to
+     * start WebView in other process (they share the same ... directory)
+     */
+    @Override
+    protected boolean needsBrowserProcessStarted() {
+        return false;
+    }
+
+    private class MyServiceConnection implements ServiceConnection {
+        private Messenger mMessenger;
+
+        public MyServiceConnection(Messenger messenger) {
+            mMessenger = messenger;
+        }
+
+        public void onServiceConnected(ComponentName className, IBinder service) {
+            Messenger serviceMessenger = new Messenger(service);
+            sendMessengerToService(serviceMessenger, mMessenger);
+        }
+
+        public void onServiceDisconnected(ComponentName className) {}
+    };
+
+    private void sendMessengerToService(Messenger serviceMessenger, Messenger messenger) {

[boliu, 2016/08/02 20:15:59]

can this be part of MyServiceConnection? then MyServiceConnection can be static?

[gsennton, 2016/08/02 20:33:57]

True, will fix.

[gsennton, 2016/09/06 15:26:08]

Done.

+        Message msg = Message.obtain(null, SeparateProcessWebViewService.RECEIVER_MESSAGE_ID);
+        msg.replyTo = messenger;
+        try {
+            serviceMessenger.send(msg);
+        } catch (RemoteException e) {
+            throw new AndroidRuntimeException(e);
+        }
+    }
+
+    @SmallTest
+    @Feature({"AndroidWebView", "Navigation"}) // TODO what does this Feature stuff mean?
+    public void testShouldOverrideUrlLoadingCrash() throws Exception {
+        final CountDownLatch messageReceivedLatch = new CountDownLatch(1);
+        Messenger messenger = new Messenger(new Handler(
+                // Don't use the current thread for this Handler as we will block the current
+                // thread waiting for a result from the separate process.
+                ThreadUtils.getUiThreadLooper(), new Handler.Callback() {
+                    @Override
+                    public boolean handleMessage(Message msg) {
+                        switch (msg.what) {
+                            case SeparateProcessWebViewService.RETURN_MESSAGE_ID:
+                                messageReceivedLatch.countDown();
+                                return true;
+                            default:
+                                return false;
+                        }
+                    }
+                }));
+        Intent intent = new Intent(getActivity(), SeparateProcessWebViewService.class);
+
+        MyServiceConnection serviceConnection = new MyServiceConnection(messenger);
+
+        // We start the service explicitly to ensure it keeps on running asynchronous tasks.
+        assertNotNull(getActivity().startService(intent));

[boliu, 2016/08/02 20:15:59]

not super familiar with services, is there something we listen to for service
crash? if the reply does not arrive first, that would be an explicit failure
case instead of waiting for this to timeout?

[gsennton, 2016/08/02 20:33:57]

There is the onServiceDisconnected callback above we could use to notify the
test when we lose the connection to the service.

[gsennton, 2016/09/06 15:26:08]

Though I think the onServiceDisconnected callback would just mean that the
service got killed - it might still be restarted again to complete the test.

Now I'm returning a message from the service telling us that we failed. But that
only covers the case when the service throws a java exception - not the other
cases where the service can crash.

[boliu, 2016/09/07 18:32:40]

Sounds like should definitely listen to onServiceDisconnected in this test then,
to avoid timeouts.

[gsennton, 2016/09/14 13:51:51]

Wouldn't that be a possible cause for flakiness? (if the service gets killed for
whatever reason, instead of crashing). I'm not sure here - atm I've added a
check that crashes here if we disconnect Before we receive a response from the
Service.

+        try {
+            assertTrue(getActivity().bindService(intent, serviceConnection, 0));
+            try {
+                // Wait until we receive a message from the service, if we never receive a message
+                // we should fail (since the service then didn't observe the correct exception).
+                assertTrue("Timed out waiting for separate service to report thrown Exception",
+                        messageReceivedLatch.await(4, TimeUnit.SECONDS)); // TODO time-out
+            } finally {
+                getActivity().unbindService(serviceConnection);
+            }
+        } finally {
+            getActivity().stopService(intent);
+        }
+    }
+}