[Cronet] Fix race in UploadDataStream.attachToRequest.

components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java

Index: components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java
index c03ba84455fb4b56f11d3bf71fcf755d6016ab9a..506b452440985e2bdf72765f8d5dfe5cdf3ab662 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java
@@ -4,6 +4,7 @@
 
 package org.chromium.net;
 
+import android.os.ConditionVariable;
 import android.os.ParcelFileDescriptor;
 import android.os.StrictMode;
 import android.test.suitebuilder.annotation.SmallTest;
@@ -13,6 +14,8 @@ import org.chromium.base.test.util.Feature;
 
 import java.io.File;
 import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
 
 /** Test the default provided implementations of {@link UploadDataProvider} */
 public class UploadDataProvidersTest extends CronetTestBase {
@@ -137,4 +140,38 @@ public class UploadDataProvidersTest extends CronetTestBase {
         assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
         assertEquals(LOREM, callback.mResponseAsString);
     }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testNoErrorWhenCanceledDuringStart() throws Exception {
+        TestUrlRequestCallback callback = new TestUrlRequestCallback();
+        UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getEchoBodyURL(),
+                callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+        final ConditionVariable first = new ConditionVariable();
+        final ConditionVariable second = new ConditionVariable();
+        builder.addHeader("Content-Type", "useless/string");
+        builder.setUploadDataProvider(new UploadDataProvider() {
+            @Override
+            public long getLength() throws IOException {
+                first.open();
+                // This causes deadlock.
+                // second.block();

[xunjieli, 2016/03/10 17:29:53]

Why is this line commented out?

We can have a simpler test where we use TestUploadDataProvider with a dummy
read, and do

urlRequest.start();
urlRequest.cancel();

This will crash in before this CL.

[mef, 2016/03/10 17:44:05]

because getLength() is now called under urlRequest.mUrlRequestAdapterLock, so
blocking here causes urlRequest.cancel() to wait forever.

Could you elaborate on your test suggestion?

[xunjieli, 2016/03/10 18:14:19]

I see. But this isn't a regression test right? Since this test will pass with
and without this change (given that this line is commented out).

An alternative is to do a test where we immediately cancel after start(). Since
start() is posted asynchronously, cancel() will be guaranteed to destroy the
adapter before startInternalLocked() gets run (at least most of the time). This
is a regression test because before this change the test will crash, but with
this change the test won't.

[mef, 2016/03/10 19:00:54]

Yes, I've split getLength() from attach, so it is done without the lock.

+                return 0;
+            }
+
+            @Override
+            public void read(UploadDataSink uploadDataSink, ByteBuffer byteBuffer)
+                    throws IOException {}
+
+            @Override
+            public void rewind(UploadDataSink uploadDataSink) throws IOException {}
+        }, callback.getExecutor());
+        UrlRequest urlRequest = builder.build();
+        urlRequest.start();
+        first.block();
+        urlRequest.cancel();
+        second.open();
+        callback.blockForDone();
+        assertTrue(urlRequest.isCanceled());
+    }
 }