Allow direct executors in cronet.

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

Index: components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
index d7da13995e5e93778193615b2acd737971a373fa..795a68d26296e8fc918d9f11347489eb4886a6b4 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
@@ -13,6 +13,7 @@ import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNIAdditionalImport;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeClassQualifiedName;
+import org.chromium.net.InlineExecutionProhibitedException;
 import org.chromium.net.Preconditions;
 import org.chromium.net.QuicException;
 import org.chromium.net.RequestFinishedInfo;
@@ -49,6 +50,7 @@ import javax.annotation.concurrent.GuardedBy;
 public final class CronetUrlRequest implements UrlRequest {
     private static final RequestFinishedInfo.Metrics EMPTY_METRICS =
             new RequestFinishedInfo.Metrics(null, null, null, null);
+    private final boolean mAllowDirectExecutor;
 
     /* Native adapter object, owned by UrlRequest. */
     @GuardedBy("mUrlRequestAdapterLock")
@@ -57,6 +59,8 @@ public final class CronetUrlRequest implements UrlRequest {
     @GuardedBy("mUrlRequestAdapterLock")
     private boolean mStarted = false;
     @GuardedBy("mUrlRequestAdapterLock")
+    private boolean mFailed = false;
+    @GuardedBy("mUrlRequestAdapterLock")
     private boolean mWaitingOnRedirect = false;
     @GuardedBy("mUrlRequestAdapterLock")
     private boolean mWaitingOnRead = false;
@@ -109,6 +113,7 @@ public final class CronetUrlRequest implements UrlRequest {
 
         @Override
         public void run() {
+            checkCallingThread();
             // Null out mByteBuffer, to pass buffer ownership to callback or release if done.
             ByteBuffer buffer = mByteBuffer;
             mByteBuffer = null;
@@ -130,7 +135,7 @@ public final class CronetUrlRequest implements UrlRequest {
     CronetUrlRequest(CronetUrlRequestContext requestContext, String url, int priority,
             UrlRequest.Callback callback, Executor executor, Collection<Object> requestAnnotations,
             boolean metricsCollectionEnabled, boolean disableCache,
-            boolean disableConnectionMigration) {
+            boolean disableConnectionMigration, boolean allowDirectExecutor) {
         if (url == null) {
             throw new NullPointerException("URL is required");
         }
@@ -144,6 +149,7 @@ public final class CronetUrlRequest implements UrlRequest {
             throw new NullPointerException("requestAnnotations is required");
         }
 
+        mAllowDirectExecutor = allowDirectExecutor;
         mRequestContext = requestContext;
         mInitialUrl = url;
         mUrlChain.add(url);
@@ -223,10 +229,9 @@ public final class CronetUrlRequest implements UrlRequest {
                                 "Requests with upload data must have a Content-Type.");
                     }
                     mStarted = true;
-                    mUploadDataStream.postTaskToExecutor(new Runnable() {
+                    mUploadDataStream.initializeWithRequest(CronetUrlRequest.this, new Runnable() {
                         @Override
                         public void run() {
-                            mUploadDataStream.initializeWithRequest(CronetUrlRequest.this);
                             synchronized (mUrlRequestAdapterLock) {
                                 if (isDoneLocked()) {
                                     return;
@@ -369,6 +374,8 @@ public final class CronetUrlRequest implements UrlRequest {
                     failException);
             // If posting a task throws an exception, then there is no choice
             // but to destroy the request without invoking the callback.
+            failWithException(

[mef, 2016/08/30 17:55:47]

I think this could cause destroyRequestAdapter() to be called twice - once here,
and once in task posted in failedWithException().

[Charles, 2016/08/30 20:11:06]

That's OK, destroyRequestAdapter() is idempotent.

[mef, 2016/08/31 17:26:52]

Fair point. Could you update the comment above? 
I'm also missing what scenario does failWithException address here, given that
its runnable is just as likely to be rejected as this |task|.

[Charles, 2016/08/31 19:08:36]

Comment updated. I wouldn't say it's "just as likely" - there's circumstances
where it is worth trying.

+                    new UrlRequestException("Exception posting task to executor", failException));
             destroyRequestAdapter(false);
         }
     }
@@ -466,6 +473,13 @@ public final class CronetUrlRequest implements UrlRequest {
      * Fails the request with an exception. Can be called on any thread.
      */
     private void failWithException(final UrlRequestException exception) {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mFailed) {
+                return;
+            } else {
+                mFailed = true;
+            }
+        }
         Runnable task = new Runnable() {
             @Override
             public void run() {
@@ -482,7 +496,11 @@ public final class CronetUrlRequest implements UrlRequest {
                 }
             }
         };
-        postTaskToExecutor(task);
+        try {
+            mExecutor.execute(task);
+        } catch (RejectedExecutionException e) {
+            Log.e(CronetUrlRequestContext.LOG_TAG, "Exception posting task to executor", e);
+        }
     }
 
     ////////////////////////////////////////////////
@@ -517,6 +535,7 @@ public final class CronetUrlRequest implements UrlRequest {
         Runnable task = new Runnable() {
             @Override
             public void run() {
+                checkCallingThread();
                 synchronized (mUrlRequestAdapterLock) {
                     if (isDoneLocked()) {
                         return;
@@ -547,6 +566,7 @@ public final class CronetUrlRequest implements UrlRequest {
         Runnable task = new Runnable() {
             @Override
             public void run() {
+                checkCallingThread();
                 synchronized (mUrlRequestAdapterLock) {
                     if (isDoneLocked()) {
                         return;
@@ -736,6 +756,13 @@ public final class CronetUrlRequest implements UrlRequest {
         }
     }
 
+    /** Enforces prohibition of direct execution. */
+    private void checkCallingThread() {
+        if (!mAllowDirectExecutor && mRequestContext.isNativeThread(Thread.currentThread())) {
+            throw new InlineExecutionProhibitedException();

[mef, 2016/08/30 17:55:47]

I think this will cause native crash - exception will be thrown on the network
thread and jni wrappers will CHECK as it is not handled.

[Charles, 2016/08/30 20:11:06]

The exception is on the network thread, but it'll get caught in
postTaskToExecutor() and so it won't make it into native code.

[mef, 2016/08/31 18:30:11]

Ah, good point.

+        }
+    }
+
     // Native methods are implemented in cronet_url_request_adapter.cc.
 
     private native long nativeCreateRequestAdapter(long urlRequestContextAdapter, String url,