[Cronet] In JavaUrlRequest avoid thread-unsafe access to HttpURLConnection

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;
@@ skipping 12 matching lines @@
 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;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.concurrent.Executor;
 import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 
 /**
  * Pure java UrlRequest, backed by {@link HttpURLConnection}.
@@ skipping 76 matching lines @@
         STARTED,
         REDIRECT_RECEIVED,
         AWAITING_FOLLOW_REDIRECT,
         AWAITING_READ,
         READING,
         ERROR,
         COMPLETE,
         CANCELLED,
     }
 
+    // Executor that runs one task at a time on an underlying Executor.
+    private class SeriaizingExecutor implements Executor {
+        private final Executor mUnderlyingExecutor;
+        // Queue of tasks to run.  First element is the task currently executing.
+        // Task processing loop is running if queue is not empty.  Synchronized on itself.
+        private final List<Runnable> mTaskQueue = new LinkedList<>();

[Charles, 2017/01/07 03:48:06]

Also @GuardedBy

[Charles, 2017/01/07 03:48:06]

Use an ArrayDeque instead - faster, offers methods named for queues, and
allocates less memory.

[pauljensen, 2017/01/11 15:29:58]

Done.

[pauljensen, 2017/01/11 15:29:58]

Done.

+
+        public SeriaizingExecutor(Executor underlyingExecutor) {
+            mUnderlyingExecutor = underlyingExecutor;
+        }
+
+        @Override
+        public void execute(final Runnable command) {
+            synchronized (mTaskQueue) {
+                mTaskQueue.add(command);
+                // Task processing loop is already running if there are other items in mTaskQueue.
+                if (mTaskQueue.size() > 1) {
+                    return;
+                }
+            }
+            mUnderlyingExecutor.execute(new Runnable() {
+                @Override
+                public void run() {
+                    Runnable task = command;
+                    while (true) {
+                        task.run();

[Charles, 2017/01/07 03:48:06]

If this throws an exception, other submitted tasks won't run until another task
is submitted - you could fix that by changing this code to submit a new runnable
to the underlying executor for each runnable. 

The reason this isn't done in the Guava implementation is that someone could
wrap a direct executor, and that would cause stack overflow. That's not a risk
here, because we control the executor we're wrapping.

[pauljensen, 2017/01/11 15:29:58]

Done, note that the Runnables can only be submitted after each task has run.

+                        synchronized (mTaskQueue) {
+                            mTaskQueue.remove(0);
+                            if (mTaskQueue.isEmpty()) {
+                                return;
+                            }
+                            task = mTaskQueue.get(0);
+                        }
+                    }
+                }
+            });
+        };
+    }
+
     /**
      * @param executor The executor used for reading and writing from sockets
      * @param userExecutor The executor used to dispatch to {@code callback}
      */
     JavaUrlRequest(Callback callback, final Executor executor, Executor userExecutor, String url,
             String userAgent, boolean allowDirectExecutor) {
         if (url == null) {
             throw new NullPointerException("URL is required");
         }
         if (callback == null) {
             throw new NullPointerException("Listener is required");
         }
         if (executor == null) {
             throw new NullPointerException("Executor is required");
         }
         if (userExecutor == null) {
             throw new NullPointerException("userExecutor is required");
         }
 
         this.mAllowDirectExecutor = allowDirectExecutor;
         this.mCallbackAsync = new AsyncUrlRequestCallback(callback, userExecutor);
         this.mTrafficStatsTag = TrafficStats.getThreadStatsTag();
-        this.mExecutor = new Executor() {
+        this.mExecutor = new SeriaizingExecutor(new Executor() {
             @Override
             public void execute(final Runnable command) {
                 executor.execute(new Runnable() {
                     @Override
                     public void run() {
                         int oldTag = TrafficStats.getThreadStatsTag();
                         TrafficStats.setThreadStatsTag(mTrafficStatsTag);
                         try {
                             command.run();
                         } finally {
                             TrafficStats.setThreadStatsTag(oldTag);
                         }
                     }
                 });
             }
-        };
+        });
         this.mCurrentUrl = url;
         this.mUserAgent = userAgent;
     }
 
     @Override
     public void setHttpMethod(String method) {
         checkNotStarted();
         if (method == null) {
             throw new NullPointerException("Method is required.");
         }
@@ skipping 794 matching lines @@
                     // Can't throw directly from here, since the delegate executor could catch this
                     // exception.
                     mExecutedInline = new InlineExecutionProhibitedException();
                     return;
                 }
                 mCommand.run();
             }
         }
     }
 }