Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2869103002:
[Android WebView] Propagate Java exceptions thrown in OnReceivedSslError (Closed)
Description[Android WebView] Propagate Java exceptions thrown in OnReceivedSslError
The OnReceivedSslError callback is accessed through a JNI call. By
default, JNI calls in Clank and WebView are handled so that when a Java
exception is thrown inside a JNI call we will print the stack trace for
that exception to the logcat when the JNI call returns to native. We
will then intentionally crash in native to avoid any undefined behaviour
caused by the pending Java exception.
So when an app throws a Java exception inside OnReceivedSslError, that
exception is not correctly propagated to Android's feedback mechanism.
With this CL we fix this by posting the callback call to the
current Java handler - so that when an exception is thrown it won't have
to propagate back through JNI and a native stack, instead it propagates
directly through Java to Android's UncaughtExceptionHandler, and from
there on to Android's Feedback mechanism.
BUG=719395
Review-Url: https://codereview.chromium.org/2869103002
Cr-Commit-Position: refs/heads/master@{#472756}
Committed: https://chromium.googlesource.com/chromium/src/+/a98edc8510b39155ec0e0efa1db29b195d487d7e
Patch Set 1 #
Total comments: 6
Patch Set 2 : Bail out before calling callback in AllowCertificateError, remove TODOs. #Patch Set 3 : Add comments to ssl_manager.h as well. #
Total comments: 3
Patch Set 4 : Post the java-side onReceivedSslError callback back to current thread to avoid native stack. #Messages
Total messages: 26 (6 generated)
gsennton@chromium.org changed reviewers: + estark@chromium.org, sgurun@chromium.org, torne@chromium.org
Torne: adding you as general reviewer since you reviewed the corresponding shouldOverrideUrlLoading CL: https://codereview.chromium.org/2169553002/ Selim: in case you have any insights on the couple of TODOs regarding whether to call/remove a callback before bailing after a Java exception. estark@ you seem to have touched the code in ssl_manager.cc before. The objective of the comments I'm adding is to ensure we won't call back into Java when we have already thrown a Java Exception (which WebView can do, with this patch, in AllowCertificateError()) because doing so is unsafe according to the JNI (Java Native Interface) spec. PTAL :)
https://codereview.chromium.org/2869103002/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/2869103002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_content_browser_client.cc:397: // TODO bail here on JNI exception ? Will we want to run the callback first? No you don't need to run. https://codereview.chromium.org/2869103002/diff/1/android_webview/native/aw_c... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/2869103002/diff/1/android_webview/native/aw_c... android_webview/native/aw_contents_client_bridge.cc:114: // TODO bail here, or remove the calback first? immediately after that, the message loop will abort, execution will go to Java and we will crash right? I don't see the need to remove the callback.
Yeah, this LGTM as-is.
On 2017/05/10 18:54:40, Torne wrote: > Yeah, this LGTM as-is. just remove the //TODO bail here and lgtm from me too.
https://codereview.chromium.org/2869103002/diff/1/content/browser/ssl/ssl_man... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2869103002/diff/1/content/browser/ssl/ssl_man... content/browser/ssl/ssl_manager.cc:318: // any additional code here, make sure it cannot call into Java. Hmm, I don't think I understand this. Is it a problem if a caller of OnCertError() calls back into Java?
estark@ apologies for the wall of text - hopefully it explains what we're trying to do at least :) (let me know if anything doesn't make sense). https://codereview.chromium.org/2869103002/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/2869103002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_content_browser_client.cc:397: // TODO bail here on JNI exception ? Will we want to run the callback first? On 2017/05/09 20:29:59, sgurun wrote: > No you don't need to run. Actually, I think we should explicitly avoid calling this callback when there is a pending Java exception - it is difficult to keep track of whether the callback will or will not call into Java at some point. https://codereview.chromium.org/2869103002/diff/1/android_webview/native/aw_c... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/2869103002/diff/1/android_webview/native/aw_c... android_webview/native/aw_contents_client_bridge.cc:114: // TODO bail here, or remove the calback first? On 2017/05/09 20:29:59, sgurun wrote: > immediately after that, the message loop will abort, execution will go to Java > and we will crash right? I don't see the need to remove the callback. Done. https://codereview.chromium.org/2869103002/diff/1/content/browser/ssl/ssl_man... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2869103002/diff/1/content/browser/ssl/ssl_man... content/browser/ssl/ssl_manager.cc:318: // any additional code here, make sure it cannot call into Java. On 2017/05/11 00:50:56, estark wrote: > Hmm, I don't think I understand this. Is it a problem if a caller of > OnCertError() calls back into Java? 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.
ping for estark@ :)
Thanks for the explanation. Seems like the comment should maybe go in ssl_manager.h then as well? I'm worried that someone else will come along and add another callsite that calls into SSLManager and then calls into Java after that.
Thanks estark@, I added some comments to ssl_manager.h, I don't know if there's anything else we can do to ensure no one breaks this :/ ptal
Thanks, ssl_manager lgtm
Thanks estark@, actually, we are currently contemplating having the Java-side of allowCertificateError just post its task to the current Java-handler - so that if we crash within that task it won't have any native stack to back out of. So, as long as calling allowCertificateError isn't performance critical, and as long as the callback sent to allowCertificateError survives across task postings, we should be fine. https://codereview.chromium.org/2869103002/diff/40001/android_webview/browser... File android_webview/browser/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/2869103002/diff/40001/android_webview/browser... android_webview/browser/aw_contents_client_bridge.cc:126: if (*cancel_request) { Selim: I guess if we post the Java-side of allowCertificateError as a new Java task, we will have to explicitly remove this pending callback at the end of that Java-task? (since we won't know here whether we should cancel or not).
https://codereview.chromium.org/2869103002/diff/40001/android_webview/browser... File android_webview/browser/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/2869103002/diff/40001/android_webview/browser... android_webview/browser/aw_contents_client_bridge.cc:126: if (*cancel_request) { On 2017/05/17 13:19:18, gsennton wrote: > Selim: I guess if we post the Java-side of allowCertificateError as a new Java > task, we will have to explicitly remove this pending callback at the end of that > Java-task? (since we won't know here whether we should cancel or not). Aha, I see what's going on here now. We remove this callback here and call the callback directly from aw_content_browser_client.cc instead (passing CERTIFICATE_REQUEST_RESULT_TYPE_DENY) if the java-side allowCertificateError() call returns false. So, what we could do is run the parts of the java-side allowCertificateError() method that can return false (i.e. deny the ssl-error-ignoring) directly, and then post only the onReceivedSslError() callback as a separate task. In that way we shouldn't need to change any of the native code :)
https://codereview.chromium.org/2869103002/diff/40001/android_webview/browser... File android_webview/browser/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/2869103002/diff/40001/android_webview/browser... android_webview/browser/aw_contents_client_bridge.cc:126: if (*cancel_request) { On 2017/05/17 14:45:07, gsennton wrote: > On 2017/05/17 13:19:18, gsennton wrote: > > Selim: I guess if we post the Java-side of allowCertificateError as a new Java > > task, we will have to explicitly remove this pending callback at the end of > that > > Java-task? (since we won't know here whether we should cancel or not). > > Aha, I see what's going on here now. > We remove this callback here and call the callback directly from > aw_content_browser_client.cc instead (passing > CERTIFICATE_REQUEST_RESULT_TYPE_DENY) if the java-side allowCertificateError() > call returns false. > > So, what we could do is run the parts of the java-side allowCertificateError() > method that can return false (i.e. deny the ssl-error-ignoring) directly, and > then post only the onReceivedSslError() callback as a separate task. > > In that way we shouldn't need to change any of the native code :) sounds reasonable and simpler.
Alright! This is much more straight-forward now :) Torne and Selim: ptal again
On 2017/05/17 16:19:01, gsennton wrote: > Alright! This is much more straight-forward now :) > > Torne and Selim: ptal again still lgtm
lgtm
Actually, can you update the CL description before landing? It still describes the old approach.
Description was changed from ========== [Android WebView] Propagate Java exceptions thrown in OnReceivedSslError The OnReceivedSslError callback is accessed through a JNI call. By default, JNI calls in Clank and WebView are handled so that when a Java exception is thrown inside a JNI call we will print the stack trace for that exception to the logcat when the JNI call returns to native. We will then intentionally crash in native to avoid any undefined behaviour caused by the pending Java exception. So when an app throws a Java exception inside some WebView-callback, that exception is not correctly propagated to Android's feedback mechanism. With this CL we fix this for the OnReceivedSslError callback by making sure that when we throw in OnReceivedSslError 1. we don't crash in native and, 2. we bail directly back out into Java without making any additional JNI calls with the pending Java exception. In this way the Java exception is caught by Android's UncaughtExceptionHandler, and correctly propagated to Android's Feedback mechanism. BUG=719395 ========== to ========== [Android WebView] Propagate Java exceptions thrown in OnReceivedSslError The OnReceivedSslError callback is accessed through a JNI call. By default, JNI calls in Clank and WebView are handled so that when a Java exception is thrown inside a JNI call we will print the stack trace for that exception to the logcat when the JNI call returns to native. We will then intentionally crash in native to avoid any undefined behaviour caused by the pending Java exception. So when an app throws a Java exception inside OnReceivedSslError, that exception is not correctly propagated to Android's feedback mechanism. With this CL we fix this by posting the callback call to the current Java handler - so that when an exception is thrown it won't have to propagate back through JNI and a native stack, instead it propagates directly through Java to Android's UncaughtExceptionHandler, and from there on to Android's Feedback mechanism. BUG=719395 ==========
On 2017/05/18 00:47:56, Torne wrote: > Actually, can you update the CL description before landing? It still describes > the old approach. Thanks for catching that, I did update the commit locally but not here in the web interface. Done!
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2869103002/#ps60001 (title: "Post the java-side onReceivedSslError callback back to current thread to avoid native stack.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495099621833720,
"parent_rev": "36e0b6779687fc64be7cef113c0dd9c7ccb3cdd4", "commit_rev":
"a98edc8510b39155ec0e0efa1db29b195d487d7e"}
Message was sent while issue was closed.
Description was changed from ========== [Android WebView] Propagate Java exceptions thrown in OnReceivedSslError The OnReceivedSslError callback is accessed through a JNI call. By default, JNI calls in Clank and WebView are handled so that when a Java exception is thrown inside a JNI call we will print the stack trace for that exception to the logcat when the JNI call returns to native. We will then intentionally crash in native to avoid any undefined behaviour caused by the pending Java exception. So when an app throws a Java exception inside OnReceivedSslError, that exception is not correctly propagated to Android's feedback mechanism. With this CL we fix this by posting the callback call to the current Java handler - so that when an exception is thrown it won't have to propagate back through JNI and a native stack, instead it propagates directly through Java to Android's UncaughtExceptionHandler, and from there on to Android's Feedback mechanism. BUG=719395 ========== to ========== [Android WebView] Propagate Java exceptions thrown in OnReceivedSslError The OnReceivedSslError callback is accessed through a JNI call. By default, JNI calls in Clank and WebView are handled so that when a Java exception is thrown inside a JNI call we will print the stack trace for that exception to the logcat when the JNI call returns to native. We will then intentionally crash in native to avoid any undefined behaviour caused by the pending Java exception. So when an app throws a Java exception inside OnReceivedSslError, that exception is not correctly propagated to Android's feedback mechanism. With this CL we fix this by posting the callback call to the current Java handler - so that when an exception is thrown it won't have to propagate back through JNI and a native stack, instead it propagates directly through Java to Android's UncaughtExceptionHandler, and from there on to Android's Feedback mechanism. BUG=719395 Review-Url: https://codereview.chromium.org/2869103002 Cr-Commit-Position: refs/heads/master@{#472756} Committed: https://chromium.googlesource.com/chromium/src/+/a98edc8510b39155ec0e0efa1db2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a98edc8510b39155ec0e0efa1db2... |
