[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..f32acc6d718112cec34d08df466f6bca336eff49 100644
--- a/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
@@ -61,8 +61,22 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
     private boolean mContentLengthOverLimit;
     private boolean mSkippingToOffset;
     private long mSize;
+
     // Indicates whether redirects have been disabled.
     private boolean mDisableRedirects;
+
+    // Caches the http status code. Default to 0.
+    private int mHttpStatusCode = 0;
+
+    // Caches the http status text. Default to null.
+    private String mHttpStatusText;
+
+    // Caches the native error code. Default to no error.
+    private int mErrorCode = ChromiumUrlRequestError.SUCCESS;
+
+    // Caches the native error string. Default to null.
+    private String mErrorString;
+
     private final Object mLock = new Object();
 
     public ChromiumUrlRequest(ChromiumUrlRequestContext requestContext,
@@ -132,21 +146,26 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
 
     @Override
     public int getHttpStatusCode() {
-        int httpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter);
+        if (mUrlRequestAdapter != 0) {
+            mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter);

[mmenke, 2015/02/25 23:43:57]

Can we populate this in onResponseStarted unconditionally instead?

[miloslav, 2015/02/26 15:56:49]

+1 for setting in onResponseStarted.

[xunjieli, 2015/02/26 17:54:00]

Done. Good idea! thanks!

[xunjieli, 2015/02/26 17:54:00]

Done.

+        }
 
         // 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) {
+            mHttpStatusCode = 200;
         }
-        return httpStatusCode;
+        return mHttpStatusCode;
     }
 
     @Override
     public String getHttpStatusText() {

[miloslav, 2015/02/26 15:56:49]

As above. Also, it would be nice if you change the javadoc of
HttpUrlRequest#getHttpStatusCode to mention that it may return null the request
has not started.

[xunjieli, 2015/02/26 17:54:00]

Done.

-        return nativeGetHttpStatusText(mUrlRequestAdapter);
+        if (mUrlRequestAdapter != 0) {
+            mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter);
+        }
+        return mHttpStatusText;
     }
 
     /**
@@ -161,16 +180,21 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
 
         validateNotRecycled();
 
-        int errorCode = nativeGetErrorCode(mUrlRequestAdapter);
-        switch (errorCode) {
+        if (mUrlRequestAdapter != 0) {
+            mErrorCode = nativeGetErrorCode(mUrlRequestAdapter);

[mmenke, 2015/02/25 23:43:57]

Can we populate this and mErrorString in whatever callback from net/ we normally
get on failures?  (OnRequestFinished?).  Then we don't need the
validateNotRecycled, either...  Also, currently, the validateNotRecycled means
we'll actually throw an exception if the adapter is 0 before we get to this
line.

[mmenke, 2015/02/25 23:45:37]

Also, if we care about this case, seems a problem that it wasn't caught by tests
(Which I haven't looked at / thought about yet)

[xunjieli, 2015/02/26 17:54:00]

Acknowledged.

[xunjieli, 2015/02/26 17:54:00]

Done. Thanks for clarifying about validateNotRecycled.

+        }
+
+        switch (mErrorCode) {
             case ChromiumUrlRequestError.SUCCESS:
                 if (mContentLengthOverLimit) {
                     return new ResponseTooLargeException();
                 }
                 return null;
             case ChromiumUrlRequestError.UNKNOWN:
-                return new IOException(
-                        nativeGetErrorString(mUrlRequestAdapter));
+                if (mUrlRequestAdapter != 0) {
+                    mErrorString = nativeGetErrorString(mUrlRequestAdapter);
+                }
+                return new IOException(mErrorString);
             case ChromiumUrlRequestError.MALFORMED_URL:
                 return new MalformedURLException("Malformed URL: " + mUrl);
             case ChromiumUrlRequestError.CONNECTION_TIMED_OUT:
@@ -188,7 +212,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);
         }
     }