Catch and report exceptions in CalledByNative java methods.

components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net;
 
+import android.util.Log;
+
 import org.apache.http.conn.ConnectTimeoutException;
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
 
 import java.io.IOException;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.UnknownHostException;
 import java.nio.ByteBuffer;
 import java.nio.channels.ReadableByteChannel;
@@ skipping 380 matching lines @@
 
     private void validateHeadersAvailable() {
         if (!mHeadersAvailable) {
             throw new IllegalStateException("Response headers not available");
         }
     }
 
     // Private methods called by native library.
 
     /**
+     * If @CalledByNative method throws an exception, request gets cancelled
+     * and exception could be retrieved from using getException().
+     */
+    private void onCalledByNativeException(Exception e) {
+        mSinkException = new IOException(
+            "CalledByNative method has thrown an exception", e);

[mmenke, 2014/08/13 21:15:27]

Should these continuation lines all use 8-space indent?

[mef, 2014/08/13 21:34:40]

Done.

+        Log.e(ChromiumUrlRequestContext.LOG_TAG,
+            "Exception in CalledByNative method", e);
+        try {
+            cancel();

[mmenke, 2014/08/13 21:15:27]

Do we tell the consumer when this happens?

[mef, 2014/08/13 21:34:40]

Consumer can check whether request isCanceled() and getException is not null.

[mmenke, 2014/08/13 22:18:09]

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.

[mef, 2014/08/15 14:58:19]

Listener's onRequestComplete is getting called when request is canceled.

+        } catch (Exception cancel_exception) {
+            Log.e(ChromiumUrlRequestContext.LOG_TAG,
+                "Exception trying to cancel request", cancel_exception);

[Charles, 2014/08/13 21:17:09]

I'm not super comfortable with swallowing all exceptions. How will we know our
code has a bug?

[mef, 2014/08/13 21:34:40]

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.

+        }
+    }
+
+    /**
      * A callback invoked when the first chunk of the response has arrived.
      */
     @CalledByNative
     private void onResponseStarted() {
-        mContentType = nativeGetContentType(mUrlRequestAdapter);
-        mContentLength = nativeGetContentLength(mUrlRequestAdapter);
-        mHeadersAvailable = true;
+        try {
+            mContentType = nativeGetContentType(mUrlRequestAdapter);
+            mContentLength = nativeGetContentLength(mUrlRequestAdapter);
+            mHeadersAvailable = true;
 
-        if (mContentLengthLimit > 0 && mContentLength > mContentLengthLimit
-                && mCancelIfContentLengthOverLimit) {
-            onContentLengthOverLimit();
-            return;
+            if (mContentLengthLimit > 0 &&
+                    mContentLength > mContentLengthLimit &&
+                    mCancelIfContentLengthOverLimit) {
+                onContentLengthOverLimit();
+                    return;
+            }
+
+            if (mBufferFullResponse && mContentLength != -1
+                    && !mContentLengthOverLimit) {
+                ((ChunkedWritableByteChannel)getSink()).setCapacity(
+                        (int)mContentLength);
+            }
+
+            if (mOffset != 0) {
+                // The server may ignore the request for a byte range.
+                if (getHttpStatusCode() == 200) {
+                    // TODO(mef): Revisit this logic.
+                    if (mContentLength != -1) {
+                        mContentLength -= mOffset;
+                    }
+                    mSkippingToOffset = true;
+                } else {
+                    mSize = mOffset;
+                }
+            }
+            mListener.onResponseStarted(this);
+        } catch (Exception e) {
+            onCalledByNativeException(e);
         }
-
-        if (mBufferFullResponse && mContentLength != -1
-                && !mContentLengthOverLimit) {
-            ((ChunkedWritableByteChannel)getSink()).setCapacity(
-                    (int)mContentLength);
-        }
-
-        if (mOffset != 0) {
-            // The server may ignore the request for a byte range.
-            if (getHttpStatusCode() == 200) {
-                // TODO(mef): Revisit this logic.
-                if (mContentLength != -1) {
-                    mContentLength -= mOffset;
-                }
-                mSkippingToOffset = true;
-            } else {
-                mSize = mOffset;
-            }
-        }
-        mListener.onResponseStarted(this);
     }
 
     /**
      * Consumes a portion of the response.
      *
      * @param byteBuffer The ByteBuffer to append. Must be a direct buffer, and
      *            no references to it may be retained after the method ends, as
      *            it wraps code managed on the native heap.
      */
     @CalledByNative
     private void onBytesRead(ByteBuffer buffer) {
-        if (mContentLengthOverLimit) {
-            return;
-        }
+        try {
+            if (mContentLengthOverLimit) {
+                return;
+            }
 
-        int size = buffer.remaining();
-        mSize += size;
-        if (mSkippingToOffset) {
-            if (mSize <= mOffset) {
-                return;
-            } else {
-                mSkippingToOffset = false;
-                buffer.position((int)(mOffset - (mSize - size)));
+            int size = buffer.remaining();
+            mSize += size;
+            if (mSkippingToOffset) {
+                if (mSize <= mOffset) {
+                    return;
+                } else {
+                    mSkippingToOffset = false;
+                    buffer.position((int)(mOffset - (mSize - size)));
+                }
             }
-        }
 
-        boolean contentLengthOverLimit =
-                (mContentLengthLimit != 0 && mSize > mContentLengthLimit);
-        if (contentLengthOverLimit) {
-            buffer.limit(size - (int)(mSize - mContentLengthLimit));
-        }
+            boolean contentLengthOverLimit =
+                    (mContentLengthLimit != 0 && mSize > mContentLengthLimit);
+            if (contentLengthOverLimit) {
+                buffer.limit(size - (int)(mSize - mContentLengthLimit));
+            }
 
-        try {
-            while (buffer.hasRemaining()) {
-                mSink.write(buffer);
+            try {
+                while (buffer.hasRemaining()) {
+                    mSink.write(buffer);
+                }
+            } catch (IOException e) {
+                mSinkException = e;
+                cancel();
             }
-        } catch (IOException e) {
-            mSinkException = e;
-            cancel();
-        }
-        if (contentLengthOverLimit) {
-            onContentLengthOverLimit();
+            if (contentLengthOverLimit) {
+                onContentLengthOverLimit();
+            }
+        } catch (Exception e) {
+            onCalledByNativeException(e);
         }
     }
 
     /**
      * Notifies the listener, releases native data structures.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void finish() {
-        synchronized (mLock) {
-            mFinished = true;
+        try {
+            synchronized (mLock) {
+                mFinished = true;
 
-            if (mRecycled) {
-                return;
+                if (mRecycled) {
+                    return;
+                }
+                try {
+                    mSink.close();
+                } catch (IOException e) {
+                    // Ignore
+                }
+                onRequestComplete();
+                nativeDestroyRequestAdapter(mUrlRequestAdapter);
+                mUrlRequestAdapter = 0;
+                mRecycled = true;
             }
-            try {
-                mSink.close();
-            } catch (IOException e) {
-                // Ignore
-            }
-            onRequestComplete();
-            nativeDestroyRequestAdapter(mUrlRequestAdapter);
-            mUrlRequestAdapter = 0;
-            mRecycled = true;
+        } catch (Exception e) {
+            onCalledByNativeException(e);
         }
     }
 
     /**
      * Appends header |name| with value |value| to |headersMap|.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void onAppendResponseHeader(ResponseHeadersMap headersMap,
             String name, String value) {
-        if (!headersMap.containsKey(name)) {
-            headersMap.put(name, new ArrayList<String>());
+        try {
+            if (!headersMap.containsKey(name)) {
+                headersMap.put(name, new ArrayList<String>());
+            }
+            headersMap.get(name).add(value);
+        } catch (Exception e) {
+            onCalledByNativeException(e);
         }
-        headersMap.get(name).add(value);
     }
 
     /**
      * Reads a sequence of bytes from upload channel into the given buffer.
      * @param dest The buffer into which bytes are to be transferred.
      * @return Returns number of bytes read (could be 0) or -1 and closes
      * the channel if error occured.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private int readFromUploadChannel(ByteBuffer dest) {
-        if (mUploadChannel == null || !mUploadChannel.isOpen())
-            return -1;
         try {
-            int result = mUploadChannel.read(dest);
-            if (result < 0) {
-                mUploadChannel.close();
-                return 0;
+            if (mUploadChannel == null || !mUploadChannel.isOpen())
+                return -1;
+            try {
+                int result = mUploadChannel.read(dest);
+                if (result < 0) {
+                    mUploadChannel.close();
+                    return 0;
+                }
+                return result;
+            } catch (IOException e) {
+                mSinkException = e;
+                try {
+                    mUploadChannel.close();
+                } catch (IOException ignored) {
+                    // Ignore this exception.
+                }
+                cancel();
             }
-            return result;
-        } catch (IOException e) {
-            mSinkException = e;
-            try {
-                mUploadChannel.close();
-            } catch (IOException ignored) {
-                // Ignore this exception.
-            }
-            cancel();
-            return -1;
+        } catch (Exception e) {
+            onCalledByNativeException(e);
         }
+        return -1;
     }
 
     // Native methods are implemented in chromium_url_request.cc.
 
     private native long nativeCreateRequestAdapter(
             long ChromiumUrlRequestContextAdapter, String url, int priority);
 
     private native void nativeAddHeader(long urlRequestAdapter, String name,
             String value);
 
@@ skipping 23 matching lines @@
 
     private native String nativeGetHeader(long urlRequestAdapter, String name);
 
     private native void nativeGetAllHeaders(long urlRequestAdapter,
             ResponseHeadersMap headers);
 
     // Explicit class to work around JNI-generator generics confusion.
     private class ResponseHeadersMap extends HashMap<String, List<String>> {
     }
 }