Cronet Fix Channel Write after Close when request is canceled after success.

components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java

Index: components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java
index b271c7d81a0827baab755abfdffa08b2f78b7521..32488ed888cc04a7fcf2c61a4687be9639b79cbd 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java
@@ -4,13 +4,18 @@
 
 package org.chromium.cronet_test_apk;
 
+import android.test.suitebuilder.annotation.LargeTest;
 import android.test.suitebuilder.annotation.SmallTest;
 
 import org.chromium.base.test.util.Feature;
+import org.chromium.net.ChunkedWritableByteChannel;
 import org.chromium.net.HttpUrlRequest;
 
+import java.io.IOException;
+import java.nio.ByteBuffer;
 import java.util.HashMap;
 import java.util.List;
+import java.util.concurrent.Executors;
 
 /**
  * Tests that use mock URLRequestJobs to simulate URL requests.
@@ -128,4 +133,120 @@ public class MockUrlRequestJobTest extends CronetTestBase {
                          + "redirects have been disabled",
                      listener.mException.getMessage());
     }
+
+    /**
+     * TestByteChannel is used for making sure write is not called after the
+     * channel has been closed. Can synchronously cancel a request when write is
+     * called.
+     */
+    static class TestByteChannel extends ChunkedWritableByteChannel {
+        boolean mIsOpen = true;
+        HttpUrlRequest mRequestToCancelOnWrite;
+
+        @Override
+        public int write(ByteBuffer byteBuffer) throws IOException {
+            assertTrue(isOpen());

[mmenke, 2014/12/10 20:59:14]

Should we also be asserting mRequestToCancelOnWrite.cancel() hasn't been called
yet?  (i.e. this is the first write)

[mef, 2014/12/10 23:23:35]

Done.

+            if (mRequestToCancelOnWrite != null) {
+                mRequestToCancelOnWrite.cancel();

[mmenke, 2014/12/10 20:59:14]

Should we also have a test where we do this on the current thread's executor?

[mef, 2014/12/10 23:23:35]

Hmm, could you elaborate on that?

[mmenke, 2014/12/11 01:09:19]

Run a that cancels on the current thread, and make sure we never call write
after that - much like testWriteAfterCancel, but on the same thread.  The big
difference is that, with an executor that does stuff on a single thread, we
should never call write after cancel, even if we cancel asynchronously, and we
can check that.  When cancelling on another thread, we could.

+            }
+            return super.write(byteBuffer);
+        }
+
+        @Override
+        public void close() {
+            assertTrue(isOpen());
+            mIsOpen = false;
+            super.close();
+        }
+
+        @Override
+        public boolean isOpen() {

[mmenke, 2014/12/10 20:59:14]

Do we need this, or mIsOpen?  The super class already tracks closed/open state
already.

[mef, 2014/12/10 23:23:34]

Done.

+            return mIsOpen;
+        }
+
+        /**
+         * Set request that will be synchronously canceled when write is called.
+         */
+        public void setRequestToCancelOnWrite(HttpUrlRequest request) {
+            mRequestToCancelOnWrite = request;
+        }
+    }
+
+    @LargeTest
+    @Feature({"Cronet"})
+    public void testWriteAfterCancel() throws Exception {

[mmenke, 2014/12/10 20:59:14]

Think this test name and the next are confusing:  This is named after what
shouldn't happen, the other is named after what the test does.

Maybe "testNoWriteAfterCancelOnAnotherThread" and "testNoWriteAfterSyncCancel"? 

[mef, 2014/12/10 23:23:35]

Done.

+        CronetTestActivity activity = launchCronetTestApp();
+
+        // This test verifies that WritableByteChannel.write is not called after
+        // WritableByteChannel.close if request is canceled from another
+        // thread.
+        for (int i = 0; i < 100; ++i) {
+            HashMap<String, String> headers = new HashMap<String, String>();
+            TestByteChannel channel = new TestByteChannel();
+            TestHttpUrlRequestListener listener =
+                    new TestHttpUrlRequestListener();
+
+            // Create request.
+            final HttpUrlRequest request =
+                    activity.mRequestFactory.createRequest(
+                            MockUrlRequestJobFactory.SUCCESS_URL,
+                            HttpUrlRequest.REQUEST_PRIORITY_LOW, headers,
+                            channel, listener);
+            request.start();
+            listener.blockForStart();
+            Runnable cancelTask = new Runnable() {
+                public void run() {
+                    request.cancel();
+                }
+            };
+            Executors.newCachedThreadPool().execute(cancelTask);
+            listener.blockForComplete();

[mmenke, 2014/12/10 20:59:14]

Assert that the byte channel is closed?  Could do it on the next one, too.

[mef, 2014/12/10 23:23:34]

Done.

+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testCancelDuringWrite() throws Exception {
+        CronetTestActivity activity = launchCronetTestApp();
+
+        HashMap<String, String> headers = new HashMap<String, String>();
+        TestByteChannel channel = new TestByteChannel();
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+
+        String data = "MyBigFunkyData";
+        int dataLength = data.length();
+        int repeatCount = 10000;
+        String mockUrl = mMockUrlRequestJobFactory.getMockUrlForData(data,
+                repeatCount);
+
+        // Create request.
+        final HttpUrlRequest request =
+                activity.mRequestFactory.createRequest(
+                        mockUrl,
+                        HttpUrlRequest.REQUEST_PRIORITY_LOW, headers,
+                        channel, listener);
+        // Channel will cancel the request from the network thread during write.

[mmenke, 2014/12/10 20:59:14]

"during the first write"?

[mef, 2014/12/10 23:23:35]

Done.

+        channel.setRequestToCancelOnWrite(request);
+        request.start();
+        listener.blockForComplete();
+        assertTrue(request.isCanceled());
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testBigDataSyncReadRequest() throws Exception {
+        String data = "MyBigFunkyData";
+        int dataLength = data.length();
+        int repeatCount = 100000;
+        String mockUrl = mMockUrlRequestJobFactory.getMockUrlForData(data,
+                repeatCount);
+        TestHttpUrlRequestListener listener = createRequestAndWaitForComplete(
+                mockUrl, false);
+        assertEquals(mockUrl, listener.mUrl);
+        String responseData = new String(listener.mResponseAsBytes);
+        for (int i = 0; i < repeatCount; ++i) {
+            assertEquals(data, responseData.substring(dataLength * i,
+                    dataLength * (i + 1)));
+        }
+    }
 }