[Cronet] Fix two races/leaks in JavaUrlRequest

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

 // Copyright 2015 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.impl;
 
 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;
 import org.chromium.net.InlineExecutionProhibitedException;
 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;
 import java.net.URI;
 import java.net.URL;
 import java.nio.ByteBuffer;
 import java.nio.channels.Channels;
 import java.nio.channels.ReadableByteChannel;
 import java.nio.channels.WritableByteChannel;
 import java.util.AbstractMap.SimpleEntry;
@@ skipping 60 matching lines @@
      *
      * <p>Concurrency notes - this value is not atomically updated with mState, so there is some
      * risk that we'd get an inconsistent snapshot of both - however, it also happens that this
      * value is only used with the STARTED state, so it's inconsequential.
      */
     @StatusValues
     private volatile int mAdditionalStatusDetails = Status.INVALID;
 
     /* 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 <--\
      *             |                                                   |     |             |
      *             \                                                   /     \             /
      * NOT_STARTED --->                   STARTED                       ----> AWAITING_READ --->
      * COMPLETE
      */
     private enum State {
         NOT_STARTED,
@@ skipping 456 matching lines @@
                 fireOpenConnection();
             }
         });
     }
 
     private void fireGetHeaders() {
         mAdditionalStatusDetails = Status.WAITING_FOR_RESPONSE;
         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());
                     return;
                 }
                 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);
                 }
             }
         }));
     }
 
     private void fireCloseUploadDataProvider() {
         if (mUploadDataProvider != null && mUploadProviderClosed.compareAndSet(false, true)) {
             try {
                 mUploadExecutor.execute(uploadErrorSetting(new CheckedRunnable() {
@@ skipping 32 matching lines @@
         mExecutor.execute(errorSetting(new CheckedRunnable() {
             @Override
             public void run() throws Exception {
                 // If we're cancelled, then our old connection will be disconnected for us and
                 // we shouldn't open a new one.
                 if (mState.get() == State.CANCELLED) {
                     return;
                 }
 
                 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();
                 }
             }
         }));
     }
 
     private Runnable errorSetting(final CheckedRunnable delegate) {
         return new Runnable() {
             @Override
             public void run() {
@@ skipping 58 matching lines @@
         } else {
             mResponseChannel.close();
             if (mState.compareAndSet(State.READING, State.COMPLETE)) {
                 fireDisconnect();
                 mCallbackAsync.onSucceeded(mUrlResponseInfo);
             }
         }
     }
 
     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
     public void cancel() {
         State oldState = mState.getAndSet(State.CANCELLED);
         switch (oldState) {
             // We've just scheduled some user code to run. When they perform their next operation,
             // they'll observe it and fail. However, if user code is cancelling in response to one
             // of these callbacks, we'll never actually cancel!
             // TODO(clm) figure out if it's possible to avoid concurrency in user callbacks.
@@ skipping 164 matching lines @@
                 mUserExecutor.execute(runnable);
             } catch (InlineExecutionProhibitedException wasDirect) {
                 if (mFallbackExecutor != null) {
                     mFallbackExecutor.execute(runnable);
                 }
             }
         }
     }
 
     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;
                 }
             }
         });
     }
 
     /**
      * Executor that detects and throws if its mDelegate runs a submitted runnable inline.
      */
     static final class DirectPreventingExecutor implements Executor {
         private final Executor mDelegate;
@@ skipping 35 matching lines @@
                     // Can't throw directly from here, since the delegate executor could catch this
                     // exception.
                     mExecutedInline = new InlineExecutionProhibitedException();
                     return;
                 }
                 mCommand.run();
             }
         }
     }
 }