[Cronet] Fix two races/leaks in JavaUrlRequest

components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java

Index: components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
index f47541d7dc9a97cef24a3fc164697fe461fdeeda..afaa35e157f6e8ea3bc6e88a5d38e829048f33d2 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
@@ -8,7 +8,6 @@ import android.annotation.SuppressLint;
 import android.annotation.TargetApi;
 import android.net.TrafficStats;
 import android.os.Build;
-
 import android.util.Log;
 
 import org.chromium.net.CronetException;
@@ -17,7 +16,6 @@ import org.chromium.net.UploadDataProvider;
 import org.chromium.net.UploadDataSink;
 import org.chromium.net.UrlResponseInfo;
 
-import java.io.Closeable;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.net.HttpURLConnection;
@@ -98,17 +96,10 @@ final class JavaUrlRequest extends UrlRequestBase {
 
     /* These change with redirects. */
     private String mCurrentUrl;
-    private ReadableByteChannel mResponseChannel;
+    private ReadableByteChannel mResponseChannel; // Only accessed on mExecutor.
     private UrlResponseInfoImpl mUrlResponseInfo;
     private String mPendingRedirectUrl;
-    /**
-     * The happens-before edges created by the executor submission and AtomicReference setting are
-     * sufficient to guarantee the correct behavior of this field; however, this is an
-     * AtomicReference so that we can cleanly dispose of a new connection if we're cancelled during
-     * a redirect, which requires get-and-set semantics.
-     * */
-    private final AtomicReference<HttpURLConnection> mCurrentUrlConnection =
-            new AtomicReference<>();
+    private HttpURLConnection mCurrentUrlConnection; // Only accessed on mExecutor.
 
     /**
      *             /- AWAITING_FOLLOW_REDIRECT <-  REDIRECT_RECEIVED <-\     /- READING <--\
@@ -585,29 +576,30 @@ final class JavaUrlRequest extends UrlRequestBase {
         mExecutor.execute(errorSetting(new CheckedRunnable() {
             @Override
             public void run() throws Exception {
-                HttpURLConnection connection = mCurrentUrlConnection.get();
-                if (connection == null) {
+                if (mCurrentUrlConnection == null) {
                     return; // We've been cancelled
                 }
                 final List<Map.Entry<String, String>> headerList = new ArrayList<>();
                 String selectedTransport = "http/1.1";
                 String headerKey;
-                for (int i = 0; (headerKey = connection.getHeaderFieldKey(i)) != null; i++) {
+                for (int i = 0; (headerKey = mCurrentUrlConnection.getHeaderFieldKey(i)) != null;
+                        i++) {
                     if (X_ANDROID_SELECTED_TRANSPORT.equalsIgnoreCase(headerKey)) {
-                        selectedTransport = connection.getHeaderField(i);
+                        selectedTransport = mCurrentUrlConnection.getHeaderField(i);
                     }
                     if (!headerKey.startsWith(X_ANDROID)) {
-                        headerList.add(new SimpleEntry<>(headerKey, connection.getHeaderField(i)));
+                        headerList.add(new SimpleEntry<>(
+                                headerKey, mCurrentUrlConnection.getHeaderField(i)));
                     }
                 }
 
-                int responseCode = connection.getResponseCode();
+                int responseCode = mCurrentUrlConnection.getResponseCode();
                 // Important to copy the list here, because although we never concurrently modify
                 // the list ourselves, user code might iterate over it while we're redirecting, and
                 // that would throw ConcurrentModificationException.
                 mUrlResponseInfo = new UrlResponseInfoImpl(new ArrayList<>(mUrlChain), responseCode,
-                        connection.getResponseMessage(), Collections.unmodifiableList(headerList),
-                        false, selectedTransport, "");
+                        mCurrentUrlConnection.getResponseMessage(),
+                        Collections.unmodifiableList(headerList), false, selectedTransport, "");
                 // TODO(clm) actual redirect handling? post -> get and whatnot?
                 if (responseCode >= 300 && responseCode < 400) {
                     fireRedirectReceived(mUrlResponseInfo.getAllHeaders());
@@ -615,10 +607,12 @@ final class JavaUrlRequest extends UrlRequestBase {
                 }
                 fireCloseUploadDataProvider();
                 if (responseCode >= 400) {
-                    mResponseChannel = InputStreamChannel.wrap(connection.getErrorStream());
+                    mResponseChannel =
+                            InputStreamChannel.wrap(mCurrentUrlConnection.getErrorStream());
                     mCallbackAsync.onResponseStarted(mUrlResponseInfo);
                 } else {
-                    mResponseChannel = InputStreamChannel.wrap(connection.getInputStream());
+                    mResponseChannel =
+                            InputStreamChannel.wrap(mCurrentUrlConnection.getInputStream());
                     mCallbackAsync.onResponseStarted(mUrlResponseInfo);
                 }
             }
@@ -671,29 +665,29 @@ final class JavaUrlRequest extends UrlRequestBase {
                 }
 
                 final URL url = new URL(mCurrentUrl);
-                HttpURLConnection newConnection = (HttpURLConnection) url.openConnection();
-                HttpURLConnection oldConnection = mCurrentUrlConnection.getAndSet(newConnection);
-                if (oldConnection != null) {
-                    oldConnection.disconnect();
+                if (mCurrentUrlConnection != null) {
+                    mCurrentUrlConnection.disconnect();
+                    mCurrentUrlConnection = null;
                 }
-                newConnection.setInstanceFollowRedirects(false);
+                mCurrentUrlConnection = (HttpURLConnection) url.openConnection();
+                mCurrentUrlConnection.setInstanceFollowRedirects(false);
                 if (!mRequestHeaders.containsKey(USER_AGENT)) {
                     mRequestHeaders.put(USER_AGENT, mUserAgent);
                 }
                 for (Map.Entry<String, String> entry : mRequestHeaders.entrySet()) {
-                    newConnection.setRequestProperty(entry.getKey(), entry.getValue());
+                    mCurrentUrlConnection.setRequestProperty(entry.getKey(), entry.getValue());
                 }
                 if (mInitialMethod == null) {
                     mInitialMethod = "GET";
                 }
-                newConnection.setRequestMethod(mInitialMethod);
+                mCurrentUrlConnection.setRequestMethod(mInitialMethod);
                 if (mUploadDataProvider != null) {
                     OutputStreamDataSink dataSink = new OutputStreamDataSink(
-                            mUploadExecutor, mExecutor, newConnection, mUploadDataProvider);
+                            mUploadExecutor, mExecutor, mCurrentUrlConnection, mUploadDataProvider);
                     dataSink.start(mUrlChain.size() == 1);
                 } else {
                     mAdditionalStatusDetails = Status.CONNECTING;
-                    newConnection.connect();
+                    mCurrentUrlConnection.connect();
                     fireGetHeaders();
                 }
             }
@@ -772,15 +766,15 @@ final class JavaUrlRequest extends UrlRequestBase {
     }
 
     private void fireDisconnect() {
-        final HttpURLConnection connection = mCurrentUrlConnection.getAndSet(null);
-        if (connection != null) {
-            mExecutor.execute(new Runnable() {
-                @Override
-                public void run() {
-                    connection.disconnect();
+        mExecutor.execute(new Runnable() {
+            @Override
+            public void run() {
+                if (mCurrentUrlConnection != null) {
+                    mCurrentUrlConnection.disconnect();
+                    mCurrentUrlConnection = null;
                 }
-            });
-        }
+            }
+        });
     }
 
     @Override
@@ -965,18 +959,16 @@ final class JavaUrlRequest extends UrlRequestBase {
     }
 
     private void closeResponseChannel() {
-        final Closeable closeable = mResponseChannel;
-        if (closeable == null) {
-            return;
-        }
-        mResponseChannel = null;
         mExecutor.execute(new Runnable() {
             @Override
             public void run() {
-                try {
-                    closeable.close();
-                } catch (IOException e) {
-                    e.printStackTrace();
+                if (mResponseChannel != null) {
+                    try {
+                        mResponseChannel.close();
+                    } catch (IOException e) {
+                        e.printStackTrace();
+                    }
+                    mResponseChannel = null;
                 }
             }
         });