[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 29 matching lines @@
     private final WritableByteChannel mSink;
     private Map<String, String> mAdditionalHeaders;
     private String mUploadContentType;
     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;
+
+    // Protects access of mUrlRequestAdapter, mStarted, mCanceled, and mFinished.
     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 125 matching lines @@
     }
 
     @Override
     public void start() {
         synchronized (mLock) {
             if (mCanceled) {
                 return;
             }
 
             validateNotStarted();
-            validateNotRecycled();
+            validateNativeAdapterNotDestroyed();
 
             mStarted = true;
 
             if (mHeaders != null && !mHeaders.isEmpty()) {
                 for (Entry<String, String> entry : mHeaders.entrySet()) {
                     nativeAddHeader(mUrlRequestAdapter, entry.getKey(),
                             entry.getValue());
                 }
             }
 
@@ skipping 29 matching lines @@
 
     @Override
     public void cancel() {
         synchronized (mLock) {
             if (mCanceled) {
                 return;
             }
 
             mCanceled = true;
 
-            if (!mRecycled) {
+            if (mUrlRequestAdapter != 0) {
                 nativeCancel(mUrlRequestAdapter);
             }
         }
     }
 
     @Override
     public boolean isCanceled() {
         synchronized (mLock) {
             return mCanceled;
         }
     }
 
-    public boolean isRecycled() {
-        synchronized (mLock) {
-            return mRecycled;
-        }
-    }
-
     @Override
     public String getNegotiatedProtocol() {
-        validateNotRecycled();
-        validateHeadersAvailable();
-        return nativeGetNegotiatedProtocol(mUrlRequestAdapter);
+        synchronized (mLock) {

[xunjieli, 2015/03/03 22:53:12]

I realize there can be a race when we call get* methods immediately after
listener.onRequestComplete() finishes, since the adapter can be destroyed after
validateNativeAdapterNotDestroyed() passes but before we actually invoke
nativeGet* methods. Therefore, I have protected the access of mUrlRequesAdapter
using mLock here and below.

+            validateNativeAdapterNotDestroyed();
+            validateHeadersAvailable();
+            return nativeGetNegotiatedProtocol(mUrlRequestAdapter);
+        }
     }
 
     @Override
     public String getContentType() {
         return mContentType;
     }
 
     @Override
     public String getHeader(String name) {
-        validateNotRecycled();
-        validateHeadersAvailable();
-        return nativeGetHeader(mUrlRequestAdapter, name);
+        synchronized (mLock) {
+            validateNativeAdapterNotDestroyed();
+            validateHeadersAvailable();
+            return nativeGetHeader(mUrlRequestAdapter, name);
+        }
     }
 
     // All response headers.
     @Override
     public Map<String, List<String>> getAllHeaders() {
-        validateNotRecycled();
-        validateHeadersAvailable();
-        ResponseHeadersMap result = new ResponseHeadersMap();
-        nativeGetAllHeaders(mUrlRequestAdapter, result);
-        return result;
+        synchronized (mLock) {
+            validateNativeAdapterNotDestroyed();
+            validateHeadersAvailable();
+            ResponseHeadersMap result = new ResponseHeadersMap();
+            nativeGetAllHeaders(mUrlRequestAdapter, result);
+            return result;
+        }
     }
 
     @Override
     public String getUrl() {
         return mUrl;
     }
 
     private static int convertRequestPriority(int priority) {
         switch (priority) {
             case HttpUrlRequest.REQUEST_PRIORITY_IDLE:
@@ skipping 13 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.
+        // Populate status code and status text if that's the case.
+        // Note that besides redirects, these two fields may be set on the
+        // request for AUTH and CERT requests.
+        if (mErrorCode != ChromiumUrlRequestError.SUCCESS) {
+            mHttpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter);
+            mHttpStatusText = nativeGetHttpStatusText(mUrlRequestAdapter);
+        }
         mListener.onRequestComplete(this);
     }
 
-    private void validateNotRecycled() {
-        if (mRecycled) {
-            throw new IllegalStateException("Accessing recycled request");
+    private void validateNativeAdapterNotDestroyed() {
+        if (mUrlRequestAdapter == 0) {
+            throw new IllegalStateException("Adapter has been destroyed");
         }
     }
 
     private void validateNotStarted() {
         if (mStarted) {
             throw new IllegalStateException("Request already started");
         }
     }
 
     private void validateHeadersAvailable() {
@@ skipping 26 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 50 matching lines @@
     @SuppressWarnings("unused")
     @CalledByNative
     private void finish() {
         try {
             synchronized (mLock) {
                 if (mDisableRedirects) {
                     mHeadersAvailable = true;
                 }
                 mFinished = true;
 
-                if (mRecycled) {
+                if (mUrlRequestAdapter == 0) {
                     return;
                 }
                 try {
                     mSink.close();
                 } catch (IOException e) {
                     // Ignore
                 }
                 try {
                     if (mUploadChannel != null && mUploadChannel.isOpen()) {
                         mUploadChannel.close();
                     }
                 } catch (IOException e) {
                     // Ignore
                 }
                 onRequestComplete();
                 nativeDestroyRequestAdapter(mUrlRequestAdapter);
                 mUrlRequestAdapter = 0;
-                mRecycled = true;
             }
         } catch (Exception e) {
             mSinkException = new IOException("Exception in finish", e);
         }
     }
 
     /**
      * Appends header |name| with value |value| to |headersMap|.
      */
     @SuppressWarnings("unused")
@@ skipping 81 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>> {
     }
 }