[Cronet] Do not call into native adapter after it is destroyed in ChromiumUrlRequest.java

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;
@@ skipping 32 matching lines @@
     private String mMethod;
     private byte[] mUploadData;
     private ReadableByteChannel mUploadChannel;
     private boolean mChunkedUpload;
     private IOException mSinkException;
     private volatile boolean mStarted;
     private volatile boolean mCanceled;
     private volatile boolean mRecycled;
     private volatile boolean mFinished;
     private boolean mHeadersAvailable;
-    private String mContentType;
     private long mUploadContentLength;
     private final HttpUrlRequestListener mListener;
     private boolean mBufferFullResponse;
     private long mOffset;
-    private long mContentLength;
     private long mContentLengthLimit;
     private boolean mCancelIfContentLengthOverLimit;
     private boolean mContentLengthOverLimit;
     private boolean mSkippingToOffset;
     private long mSize;
+
     // Indicates whether redirects have been disabled.
     private boolean mDisableRedirects;
+
+    // Http status code. Default to 0. Populated in onResponseStarted().
+    private int mHttpStatusCode = 0;
+
+    // Http status text. Default to null. Populated in onResponseStarted().
+    private String mHttpStatusText;
+
+    // Content type. Default to null. Populated in onResponseStarted().
+    private String mContentType;
+
+    // Compressed content length as reported by the server. Populated in onResponseStarted().
+    private long mContentLength;
+
+    // Native error code. Default to no error. Populated in onRequestComplete().
+    private int mErrorCode = ChromiumUrlRequestError.SUCCESS;
+
+    // Native error string. Default to null. Populated in onRequestComplete().
+    private String mErrorString;
+
     private final Object mLock = new Object();
 
     public ChromiumUrlRequest(ChromiumUrlRequestContext requestContext,
             String url, int priority, Map<String, String> headers,
             HttpUrlRequestListener listener) {
         this(requestContext, url, priority, headers,
                 new ChunkedWritableByteChannel(), listener);
         mBufferFullResponse = true;
     }
 
@@ skipping 49 matching lines @@
     }
 
     @Override
     public void setContentLengthLimit(long limit, boolean cancelEarly) {
         mContentLengthLimit = limit;
         mCancelIfContentLengthOverLimit = cancelEarly;
     }
 
     @Override
     public int getHttpStatusCode() {
-        int httpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter);
-
         // TODO(mef): Investigate the following:
         // If we have been able to successfully resume a previously interrupted
         // download, the status code will be 206, not 200. Since the rest of the
         // application is expecting 200 to indicate success, we need to fake it.
-        if (httpStatusCode == 206) {
-            httpStatusCode = 200;
+        if (mHttpStatusCode == 206) {
+            return 200;
         }
-        return httpStatusCode;
+        return mHttpStatusCode;
     }
 
     @Override
     public String getHttpStatusText() {
-        return nativeGetHttpStatusText(mUrlRequestAdapter);
+        return mHttpStatusText;
     }
 
     /**
-     * Returns an exception if any, or null if the request was completed
-     * successfully.
+     * Returns an exception if any, or null if the request has not completed or
+     * completed successfully.
      */
     @Override
     public IOException getException() {
         if (mSinkException != null) {
             return mSinkException;
         }
 
-        validateNotRecycled();
-
-        int errorCode = nativeGetErrorCode(mUrlRequestAdapter);
-        switch (errorCode) {
+        switch (mErrorCode) {
             case ChromiumUrlRequestError.SUCCESS:
                 if (mContentLengthOverLimit) {
                     return new ResponseTooLargeException();
                 }
                 return null;
             case ChromiumUrlRequestError.UNKNOWN:
-                return new IOException(
-                        nativeGetErrorString(mUrlRequestAdapter));
+                return new IOException(mErrorString);
             case ChromiumUrlRequestError.MALFORMED_URL:
                 return new MalformedURLException("Malformed URL: " + mUrl);
             case ChromiumUrlRequestError.CONNECTION_TIMED_OUT:
                 return new ConnectTimeoutException("Connection timed out");
             case ChromiumUrlRequestError.UNKNOWN_HOST:
                 String host;
                 try {
                     host = new URL(mUrl).getHost();
                 } catch (MalformedURLException e) {
                     host = mUrl;
                 }
                 return new UnknownHostException("Unknown host: " + host);
             case ChromiumUrlRequestError.TOO_MANY_REDIRECTS:
                 return new IOException("Request failed because there were too "
                         + "many redirects or redirects have been disabled");
             default:
                 throw new IllegalStateException(
-                        "Unrecognized error code: " + errorCode);
+                        "Unrecognized error code: " + mErrorCode);
         }
     }
 
     @Override
     public ByteBuffer getByteBuffer() {
         return ((ChunkedWritableByteChannel) getSink()).getByteBuffer();
     }
 
     @Override
     public byte[] getResponseAsBytes() {
@@ skipping 254 matching lines @@
 
     private void onContentLengthOverLimit() {
         mContentLengthOverLimit = true;
         cancel();
     }
 
     /**
      * A callback invoked when the response has been fully consumed.
      */
     private void onRequestComplete() {
+        mErrorCode = nativeGetErrorCode(mUrlRequestAdapter);
+        mErrorString = nativeGetErrorString(mUrlRequestAdapter);
+        // When there is an error or redirects have been disabled,
+        // onResponseStarted is often not invoked.

[mmenke, 2015/03/02 19:22:50]

Worth noting that other than the redirect case, these may be set on the request
for AUTH and CERT requests?  Otherwise, they're just generally the defaults (-1
/ the empty string).

[xunjieli, 2015/03/02 22:33:57]

Done.

+        // Populate status code and status text if that's the case.
+        if (mErrorCode != ChromiumUrlRequestError.SUCCESS) {
+            mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter);
+            mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter);

[mmenke, 2015/03/02 19:22:50]

Hrm...If we were going to maintain this interface, I'd say we should map the
net/ default values to the HttpURLConnection ones, but I don't think it's worth
the effort, and could break embedders, in theory.

[xunjieli, 2015/03/02 19:36:15]

Not sure what this means. HttpURLConnection isn't doing any default mapping from
http status code to status text. It just parses the status text from the status
line. How does net/ use default values? when the status text part is empty?

[mmenke, 2015/03/02 20:41:58]

mHttpStatusCode has a default value of 0.  If we don't get a status code, net/
uses -1.  Is this currently observable somehow?  I don't think so, but I'm not
confident of that.  Certainly would be for non-HTTP requests, but I suppose we
don't support them.

I was thinking mContentType had a similar behavior (NULL vs ""), but looks like
I was wrong about that.

mErrorString *does* have similar weirdness - it's "" if the request completed
with an error, but NULL beforehand, I believe.

[xunjieli, 2015/03/02 22:33:56]

I am not sure whether it is observable elsewhere, but it is observed in tests.
If we cancel the request, we get back status code of -1, and if we use
URLRequestFailedJob, we get 0 back.

Not sure if we should convert -1 back to 0 in Java, so it's consistent across.
But you've said that it's probably not worth the effort, so I am not sure..
I was aware of this too. We can default it to empty string, but I am not sure if
that gains any tangible benefit.

+        }
         mListener.onRequestComplete(this);
     }
 
     private void validateNotRecycled() {
         if (mRecycled) {
             throw new IllegalStateException("Accessing recycled request");
         }
     }
 
     private void validateNotStarted() {
@@ skipping 32 matching lines @@
                     "Exception trying to cancel request", cancel_exception);
         }
     }
 
     /**
      * A callback invoked when the first chunk of the response has arrived.
      */
     @CalledByNative
     private void onResponseStarted() {
         try {
+            mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter);
+            mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter);
             mContentType = nativeGetContentType(mUrlRequestAdapter);
             mContentLength = nativeGetContentLength(mUrlRequestAdapter);
             mHeadersAvailable = true;
 
             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, in which
                 // case status code will be 200, instead of 206. Note that we
                 // cannot call getHttpStatusCode as it rewrites 206 into 200.
-                if (nativeGetHttpStatusCode(mUrlRequestAdapter) == 200) {
+                if (mHttpStatusCode == 200) {
                     // TODO(mef): Revisit this logic.
                     if (mContentLength != -1) {
                         mContentLength -= mOffset;
                     }
                     mSkippingToOffset = true;
                 } else {
                     mSize = mOffset;
                 }
             }
             mListener.onResponseStarted(this);
@@ skipping 170 matching lines @@
     private native void nativeGetAllHeaders(long urlRequestAdapter,
             ResponseHeadersMap headers);
 
     private native String nativeGetNegotiatedProtocol(long urlRequestAdapter);
 
     // Explicit class to work around JNI-generator generics confusion.
     private static class ResponseHeadersMap extends
             HashMap<String, List<String>> {
     }
 }