Properly throw java exceptions from shouldOverrideUrlLoading

android_webview/native/aw_contents_client_bridge.cc

 // Copyright (c) 2013 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.
 
 #include "android_webview/native/aw_contents_client_bridge.h"
 
 #include "android_webview/common/devtools_instrumentation.h"
 #include "android_webview/native/aw_contents.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
@@ skipping 13 matching lines @@
 #include "net/ssl/openssl_client_key_store.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_client_cert_type.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF16;
 using base::android::ConvertUTF8ToJavaString;
 using base::android::ConvertUTF16ToJavaString;
+using base::android::HasException;
 using base::android::JavaRef;
 using base::android::ScopedJavaLocalRef;
 using content::BrowserThread;
 
 namespace android_webview {
 
 namespace {
 
 // Must be called on the I/O thread to record a client certificate
 // and its private key in the OpenSSLClientKeyStore.
@@ skipping 305 matching lines @@
                                                       bool has_user_gesture,
                                                       bool is_redirect,
                                                       bool is_main_frame) {
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
   if (obj.is_null())
     return false;
   ScopedJavaLocalRef<jstring> jurl = ConvertUTF16ToJavaString(env, url);
   devtools_instrumentation::ScopedEmbedderCallbackTask(
       "shouldOverrideUrlLoading");
-  return Java_AwContentsClientBridge_shouldOverrideUrlLoading(
+  bool did_override = Java_AwContentsClientBridge_shouldOverrideUrlLoading(
       env, obj.obj(), jurl.obj(), has_user_gesture, is_redirect, is_main_frame);
+  if (HasException(env)) {
+    // Tell the chromium message loop to not perform any tasks after the current
+    // one - we want to make sure we return to Java cleanly without first making
+    // any new jni calls.
+    static_cast<base::MessageLoopForUI*>(base::MessageLoop::current())->Abort();

[Torne, 2016/07/21 13:38:25]

This stops later stages of DoRunLoopOnce from executing, but it's not obvious to
me that it prevents any other native chromium tasks from running - am I missing
something? What if there's already another queued task after this current one
for immediate execution?

[gsennton, 2016/07/26 14:42:39]

The current implementation of this (MessageLoop::DoWork() -
https://cs.chromium.org/chromium/src/base/message_loop/message_loop.cc?cl=GRO...)
runs one task and then returns immediately.

danakj@, is MessageLoop.DoWork / DoDelayedWork guaranteed to only run one task?
I.e. is this something we want to uphold in the future as well, and if so, could
we add a comment to the MessagePump::Delegate::DoWork/DoDelayedWork/DoIdleWork
declarations to show that these functions should indeed only run one task each?

+    // If we crashed we don't want to continue the navigation.
+    return true;
+  }
+  return did_override;
 }
 
 void AwContentsClientBridge::ConfirmJsResult(JNIEnv* env,
                                              const JavaRef<jobject>&,
                                              int id,
                                              const JavaRef<jstring>& prompt) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   content::JavaScriptDialogManager::DialogClosedCallback* callback =
       pending_js_dialog_callbacks_.Lookup(id);
   if (!callback) {
@@ skipping 30 matching lines @@
   pending_client_cert_request_delegates_.Remove(request_id);
 
   delete delegate;
 }
 
 bool RegisterAwContentsClientBridge(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace android_webview