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

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

Index: components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
index 05f933fed38ff8f74fd9942190623386aaed1ca9..78db18c2fdc4071f2a30a0327aaae3e94267afa3 100644
--- a/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
@@ -50,19 +50,37 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
     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,
@@ -132,26 +150,24 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
 
     @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() {
@@ -159,18 +175,14 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
             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:
@@ -188,7 +200,7 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
                         + "many redirects or redirects have been disabled");
             default:
                 throw new IllegalStateException(
-                        "Unrecognized error code: " + errorCode);
+                        "Unrecognized error code: " + mErrorCode);
         }
     }
 
@@ -463,6 +475,15 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
      * 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);
     }
 
@@ -515,6 +536,8 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
     @CalledByNative
     private void onResponseStarted() {
         try {
+            mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter);
+            mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter);
             mContentType = nativeGetContentType(mUrlRequestAdapter);
             mContentLength = nativeGetContentLength(mUrlRequestAdapter);
             mHeadersAvailable = true;
@@ -536,7 +559,7 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
                 // 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;