[Android WebView] Propagate Java exceptions thrown in OnReceivedSslError

content/browser/ssl/ssl_manager.cc

Index: content/browser/ssl/ssl_manager.cc
diff --git a/content/browser/ssl/ssl_manager.cc b/content/browser/ssl/ssl_manager.cc
index 2a3124acae247b38d959156da59540dc80a6a42e..6e45791b4689e263617f2dc0aa10b0ec43805c5c 100644
--- a/content/browser/ssl/ssl_manager.cc
+++ b/content/browser/ssl/ssl_manager.cc
@@ -125,6 +125,9 @@ void HandleSSLErrorOnUI(
 
   SSLManager* manager = controller->ssl_manager();
   manager->OnCertError(std::move(handler));
+  // On Android, OnCertError can cause a Java exception to be thrown - in such a
+  // case we cannot allow calls back into Java here. If adding any additional
+  // code here, make sure it cannot call into Java.
 }
 
 }  // namespace
@@ -310,6 +313,9 @@ void SSLManager::OnCertError(std::unique_ptr<SSLErrorHandler> handler) {
       if (expired_previous_decision)
         options_mask |= EXPIRED_PREVIOUS_DECISION;
       OnCertErrorInternal(std::move(handler), options_mask);
+      // On Android, OnCertErrorInternal can cause a Java exception to be thrown
+      // - in such a case we cannot allow calls back into Java here. If adding
+      // any additional code here, make sure it cannot call into Java.

[estark, 2017/05/11 00:50:56]

Hmm, I don't think I understand this. Is it a problem if a caller of
OnCertError() calls back into Java?

[gsennton, 2017/05/11 12:17:17]

Yes, so what happens is this:
OnCertErrorInternal() calls into some code in Android WebView which in turn uses
JNI to call into Java. This call into Java ends up calling a callback,
implemented by an app-developer, which happens to throw a run-time exception.
When an exception is thrown (and never caught) on the Java-side of a JNI call
that exception is seen as pending when the call returns back to native.

According to the JNI documentation it is not safe to call back into Java from
native when there exists a pending Java exception.

The way we (Chrome on Android / Android WebView) normally deal with Java
exceptions thrown during JNI calls is to check whether there exists a pending
exception just after returning from Java to native, and if so, log the
exception, and explicitly cause a native crash instead (so that we can't
continue executing in a state where we don't really how to deal with what
happened on the Java side).
However, this way of dealing with Java exceptions is a bit messy in terms of
crash reporting - seeing only the native crash doesn't necessarily help us debug
the original Java exception, since often we don't get the log output containing
the Java crash that was the actual cause.

In Android WebView, because some of our Java exceptions are caused by
app-developers rather than by us (WebView/Chrome developers), the impact of
reporting these native crashes instead of the original java exceptions is even
higher - most app-developers will never see the native crashes because they
aren't forwarded to app developers, and even if they were, the native crash
information wouldn't make much sense to them.

So what we want to do in this code instead is to keep the Java exception
pending, and return out of our native call stack - up until we reach back into
Android (Java). At that point Android will handle the pending Java exception
through an UnhandledExceptionHandler and properly report that exception to the
app-developer.
But the important point here is that, because of the JNI documentation stating
that it's not safe to call back into Java with a pending Java exception, we have
to ensure that when returning out of our native call stack we never call back
into Java.

       break;
     case net::ERR_CERT_NO_REVOCATION_MECHANISM:
       // Ignore this error.
@@ -330,6 +336,9 @@ void SSLManager::OnCertError(std::unique_ptr<SSLErrorHandler> handler) {
       if (expired_previous_decision)
         options_mask |= EXPIRED_PREVIOUS_DECISION;
       OnCertErrorInternal(std::move(handler), options_mask);
+      // On Android, OnCertErrorInternal can cause a Java exception to be thrown
+      // - in such a case we cannot allow calls back into Java here. If adding
+      // any additional code here, make sure it cannot call into Java.
       break;
     default:
       NOTREACHED();
@@ -393,6 +402,9 @@ void SSLManager::OnCertErrorInternal(std::unique_ptr<SSLErrorHandler> handler,
         web_contents, cert_error, ssl_info, request_url, resource_type,
         overridable, strict_enforcement, expired_previous_decision,
         base::Bind(&OnAllowCertificateWithRecordDecision, true, callback));
+    // On Android, AllowCertificateError can cause a Java exception to be thrown
+    // - in such a case we cannot allow calls back into Java here. If adding any
+    // additional code here, make sure it cannot call into Java.
   }
 }