Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
474573003:
Catch and report exceptions in CalledByNative java methods. (Closed)
|
Created:
6 years, 4 months ago by mef Modified:
6 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionCatch and report exceptions in CalledByNative java methods.
BUG=403515
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289971
Patch Set 1 #Patch Set 2 : Add test asserts. #
Total comments: 13
Patch Set 3 : Address review comments. #Patch Set 4 : Add Bad Listener to throw exception and signal completion. #
Total comments: 4
Patch Set 5 : Remove redundant exception handling. #
Total comments: 5
Patch Set 6 : Close mUploadChannel in finish, and don't cancel again if exception is caught in finish. #
Total comments: 1
Patch Set 7 : Close upload channel even if mSink.close throws an exception. #Patch Set 8 : Fix long lines. #
Total comments: 2
Patch Set 9 : Move && #
Messages
Total messages: 16 (0 generated)
Hi, please take a look. I've added catching and reporting of all exceptions in @CalledByNative methods of ChromiumUrlRequest to avoid native crashes in CheckException after JNI call. Diffs look a bit worse than they are due to indentation.
https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:414: "CalledByNative method has thrown an exception", e); Should these continuation lines all use 8-space indent? https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:418: cancel(); Do we tell the consumer when this happens?
https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:421: "Exception trying to cancel request", cancel_exception); I'm not super comfortable with swallowing all exceptions. How will we know our code has a bug? https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:23: public static final String LOG_TAG = "ChromiumNetwork"; No need for public, leaving off the access marker puts it as package-private, which is what you want. https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:109: Thread.sleep(5000); This seems flaky.
Thanks! https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:414: "CalledByNative method has thrown an exception", e); On 2014/08/13 21:15:27, mmenke wrote: > Should these continuation lines all use 8-space indent? Done. https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:418: cancel(); On 2014/08/13 21:15:27, mmenke wrote: > Do we tell the consumer when this happens? Consumer can check whether request isCanceled() and getException is not null. https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:421: "Exception trying to cancel request", cancel_exception); On 2014/08/13 21:17:09, clm wrote: > I'm not super comfortable with swallowing all exceptions. How will we know our > code has a bug? Um, you do check results of request, right? Would it be better if crashed here? I want to preserve / log / communicate java exception information as opposed to native crash without much info. https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:23: public static final String LOG_TAG = "ChromiumNetwork"; On 2014/08/13 21:17:09, clm wrote: > No need for public, leaving off the access marker puts it as package-private, > which is what you want. Done. https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:109: Thread.sleep(5000); On 2014/08/13 21:17:09, clm wrote: > This seems flaky. Yeah, what's a good way to make it deterministic? Add custom HttpUrlRequestListener listener that signals from onRequestComplete?
https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:418: cancel(); On 2014/08/13 21:34:40, mef wrote: > On 2014/08/13 21:15:27, mmenke wrote: > > Do we tell the consumer when this happens? > Consumer can check whether request isCanceled() and getException is not null. Right, but the consumer isn't notified there was an error in any way...So it just has to randomly decide the request is taking a while, and poll the request, right? That seems rather bad.
On 2014/08/13 22:18:10, mmenke wrote: > https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... > File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java > (right): > > https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... > components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:418: > cancel(); > On 2014/08/13 21:34:40, mef wrote: > > On 2014/08/13 21:15:27, mmenke wrote: > > > Do we tell the consumer when this happens? > > Consumer can check whether request isCanceled() and getException is not null. > > Right, but the consumer isn't notified there was an error in any way...So it > just has to randomly decide the request is taking a while, and poll the request, > right? That seems rather bad. Listener.OnRequestCompleted() is called when request is cancelled (it is not obvious, but it is done via nativeCancel).
PTAL https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:418: cancel(); On 2014/08/13 22:18:09, mmenke wrote: > On 2014/08/13 21:34:40, mef wrote: > > On 2014/08/13 21:15:27, mmenke wrote: > > > Do we tell the consumer when this happens? > > Consumer can check whether request isCanceled() and getException is not null. > > Right, but the consumer isn't notified there was an error in any way...So it > just has to randomly decide the request is taking a while, and poll the request, > right? That seems rather bad. Listener's onRequestComplete is getting called when request is canceled. https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... File components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java (right): https://codereview.chromium.org/474573003/diff/20001/components/cronet/androi... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:109: Thread.sleep(5000); On 2014/08/13 21:34:40, mef wrote: > On 2014/08/13 21:17:09, clm wrote: > > This seems flaky. > Yeah, what's a good way to make it deterministic? > Add custom HttpUrlRequestListener listener that signals from onRequestComplete? Done. Bad Listener also verifies that onRequestComplete is triggered by cancellation and thrown exception is preserved in the request.
https://codereview.chromium.org/474573003/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:501: mSinkException = e; I don't think we need to handle these exceptions differently. https://codereview.chromium.org/474573003/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:576: } catch (IOException e) { We should merge this exception logic, right?
PTAL. https://codereview.chromium.org/474573003/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:501: mSinkException = e; On 2014/08/15 15:23:28, mmenke wrote: > I don't think we need to handle these exceptions differently. Done. https://codereview.chromium.org/474573003/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:576: } catch (IOException e) { On 2014/08/15 15:23:28, mmenke wrote: > We should merge this exception logic, right? Done. https://codereview.chromium.org/474573003/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:500: onContentLengthOverLimit(); onContentLengthOverLimit is no longer called if exception is thrown, but I think that's ok.
https://codereview.chromium.org/474573003/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:500: onContentLengthOverLimit(); On 2014/08/15 16:03:35, mef wrote: > onContentLengthOverLimit is no longer called if exception is thrown, but I think > that's ok. I agree https://codereview.chromium.org/474573003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:573: mUploadChannel.close(); Question: Why does this matter, anyways? We aren't closing the channel in normal cancellation cases or network error cases, so things have to be able to handle those cases, anyways.
PTAL. https://codereview.chromium.org/474573003/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:566: mUploadChannel.close(); Per clm it is better to close the channel as soon as possible. https://codereview.chromium.org/474573003/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:573: mUploadChannel.close(); On 2014/08/15 16:12:22, mmenke wrote: > Question: Why does this matter, anyways? We aren't closing the channel in > normal cancellation cases or network error cases, so things have to be able to > handle those cases, anyways. I've discussed with clm@, and we have to close upload channel in all scenarios, so I've moved this to finish(). https://codereview.chromium.org/474573003/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:534: mSinkException = new IOException("Exception in finish", e); Don't call onCalledByNativeException, as request is already canceled.
LGTM https://codereview.chromium.org/474573003/diff/140001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:440: && !mContentLengthOverLimit) { && should go on previous line (Know it was like this before)
Thanks! https://codereview.chromium.org/474573003/diff/140001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java (right): https://codereview.chromium.org/474573003/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:440: && !mContentLengthOverLimit) { On 2014/08/15 17:42:02, mmenke wrote: > && should go on previous line (Know it was like this before) Done.
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/474573003/160001
Message was sent while issue was closed.
Committed patchset #9 (160001) as 289971 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
