New issue
Advanced search Search tips

Issue 718949 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 592556



Sign in to add a comment

[JNI crashes] Properly throw Java Exceptions from android_webview::InputStreamImpl::~InputStreamImpl()

Project Member Reported by gsennton@chromium.org, May 5 2017

Issue description

This crash occurs on the IO thread.
Status: Available (was: Untriaged)
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).

Comment 4 by torne@chromium.org, 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 :)
Owner: gsennton@chromium.org
Alright, will upload a CL.

I agree, I like this approach as well ;)
Ah, darn it, this thread isn't backed by a Java Looper :/
I guess we could post to a thread which does.

Comment 8 by torne@chromium.org, 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)
Yeah the read also shows up: crbug.com/718936

The thread pool we use is BrowserThread.GetBlockingPool()

Comment 10 by torne@chromium.org, 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..
Yeah, good point ;)
Status: Assigned (was: Available)
bulk editing to assign, rather than available. 
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?)

Comment 14 by torne@chromium.org, 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..
Owner: ----
Status: Available (was: Assigned)
Project Member

Comment 16 by PranavkRobot, Oct 7

Labels: crash-BugNoRepro
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@
Project Member

Comment 17 by PranavkRobot, Oct 15

Labels: crash-BugNoRepro-Closed
Status: WontFix (was: Available)
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@
Labels: -crash-BugNoRepro -crash-BugNoRepro-Closed
Status: Available (was: WontFix)
We should deal with these exceptions; any app might throw here.

Sign in to add a comment