[JNI crashes] Properly throw Java Exceptions from android_webview::InputStreamImpl::~InputStreamImpl() |
||||||||
Issue descriptionandroid_webview::InputStreamImpl::~InputStreamImpl() calls into Java through Java_InputStreamUtil_close which just calls inputStream.close() on the given InputStream. The call stack for this dtor looks kinda ridiculous - the number of method calls between MessageLoop.RunTask and the failing JNI call are about 30. So I'm not entirely sure whether propagating this exception is feasible. Crash.corp URL: https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%20CONTAINS%20%27%5BAndroid%20Java%20Exception%5D%27%20AND%20product.name%3D%27AndroidWebView%27%20AND%20NOT%20product.Version%20CONTAINS%20%2754.0.2%27%20AND%20NOT%20product.Version%20CONTAINS%20%2753.0.2%27%20AND%20NOT%20product.Version%20CONTAINS%20%2752.0.2%27%20AND%20NOT%20product.Version%20CONTAINS%20%2751.0.2%27%20AND%20special_protos.user_feedback.mobile_data.crash_data.exception_class_name%3D%27Native%20crash%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%5BAndroid%20Java%20Exception%5D%20android_webview%3A%3AInputStreamImpl%3A%3A~InputStreamImpl%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D
,
May 9 2017
,
May 18 2017
So, I don't see any reason why we can't just post the inputStream.close() call to a task to be executed later (we shouldn't need to close the stream synchronously - all we need to ensure is that the stream is closed at some point).
,
May 18 2017
Yeah; the caller must already be able to deal with there being various streams hanging around that WebView is still reading from; having them hang around for one more Looper iteration before they get closed seems unlikely to cause problems. I'm liking this new approach: it's way easier to reason about :)
,
May 18 2017
Alright, will upload a CL. I agree, I like this approach as well ;)
,
May 19 2017
Ah, darn it, this thread isn't backed by a Java Looper :/
,
May 19 2017
I guess we could post to a thread which does.
,
May 19 2017
Hm. Not sure that's the best option. Aren't there also other InputStreamImpl JNI calls in the same situation? e.g. couldn't we also get an exception when reading from the stream? It might not show up as a common crash cause, but it seems logical to try to handle all of them together if possible. Maybe we should just change how the InputStream reading works to run on a java thread pool isntead of a native thread pool? It's all entirely "our" threads isn't it? (i might be wrong)
,
May 19 2017
Yeah the read also shows up: crbug.com/718936 The thread pool we use is BrowserThread.GetBlockingPool()
,
May 19 2017
Well, it probably makes sense to deal with those two together, in any case. Posting the reads to another java thread would defeat the point of using this pool in the first place, which is there to parallelise reading for performance..
,
May 19 2017
Yeah, good point ;)
,
Jun 20 2017
bulk editing to assign, rather than available.
,
Jun 20 2017
Do we have any kind of tests to ensure we don't regress performance if we go from a native thread pool to a Java one? (or is just any kind of threading enough performance-wise?)
,
Jun 20 2017
I wouldn't expect it to make any difference to performance really - this isn't about CPU usage, but blocking IO. This is done in parallel because the reads are blocking operations, and might in turn be doing blocking IO (to disk or network) in the app's implementation. We don't want to do them one at a time because then lots of streams can get stuck "behind" one that happens to be going slowly for some reason. I don't think we have any explicit performance tests for this. Using a java thread pool instead of the native one seems like it doesn't add any additional thread hops and just one extra JNI call per stream: we'd be moving from "each stream posts a task to the blocking pool in C++, each task makes repeated JNI calls to get chunks of data" to instead "each stream makes a JNI call to post a task to a Java thread pool, each task repeatedly gets chunks of data and makes a JNI call to pass it to native". The one issue with doing it that way is that it's going to add several additional threads, since we wouldn't want to use the AsyncTask pool for this (it's already full of stuff and causes problems sometimes), and would have to create a new Java thread pool for it. Probably the right thing to do is just to look at how difficult it would be to invert the stream reading code such that it runs in Java and just calls native to hand the data over: if that's going to be really fiddly as well then that might be enough of a downside to consider something else..
,
May 31 2018
,
Oct 7
Crash analysis has not encountered any reports for this bug for the past 90 days. We have added the label 'crash-BugNoRepro' Crash analysis will be automatically closing the bug in 6 days. If you do not want Crash analysis to automatically close the bug, please remove the label 'crash-BugNoRepro'. If you have any feedback on this feature, please contact pranavk@
,
Oct 15
Crash analysis has not encountered any reports for this bug for the past 90 days. Hence as per the comment 8 days ago, we are closing the bug and setting the status to WontFix. If you have any feedback on this feature, please contact pranavk@
,
Oct 16
We should deal with these exceptions; any app might throw here. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by gsennton@chromium.org
, May 8 2017