[Cronet] Do not call into BidirectionalStream::SendData if stream failed

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

Index: components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
index 10557b5cbc65c07290ef90cd294c2c00e141911c..d47f39a6df5e999d0c537e6765d4902f4fdbed9b 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
@@ -10,11 +10,15 @@ import org.chromium.base.test.util.Feature;
 import org.chromium.net.CronetTestBase.OnlyRunNativeCronet;
 import org.json.JSONObject;
 
+import java.nio.ByteBuffer;
+
 /**
  * Tests functionality of BidirectionalStream's QUIC implementation.
  */
 public class BidirectionalStreamQuicTest extends CronetTestBase {
     private CronetTestFramework mTestFramework;
+    // Whether to skip server shutdown in tearDown().
+    private boolean mSkipServerShutdown = false;
     private enum QuicBidirectionalStreams {
         ENABLED,
         DISABLED,
@@ -46,7 +50,9 @@ public class BidirectionalStreamQuicTest extends CronetTestBase {
 
     @Override
     protected void tearDown() throws Exception {
-        QuicTestServer.shutdownQuicTestServer();
+        if (!mSkipServerShutdown) {

[kapishnikov, 2016/04/23 00:29:47]

To avoid adding test logic to tearDown(), I think it is better if we add a
method (flag) to QuicTestServer.java to check whether the server has been
already shut down or not. We can also make
QuicTestServer.shutdownQuicTestServer() method idempotent and always call it in
tearDown() unconditionally. Both approaches eliminate need for
mSkipServerShutdown.

[xunjieli, 2016/04/23 10:39:00]

Done.

+            QuicTestServer.shutdownQuicTestServer();
+        }
         super.tearDown();
     }
 
@@ -92,4 +98,50 @@ public class BidirectionalStreamQuicTest extends CronetTestBase {
         assertTrue(callback.mOnErrorCalled);
         assertNull(callback.mResponseInfo);
     }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    // Tests that if the stream failed between the time when we issue a Write()
+    // and when the Write() is executed in the native stack, there is no crash.
+    // This test is racy, but it should catch a crash (if there is any) most of
+    // the time.
+    public void testStreamFailBeforeWriteIsExecutedOnNetworkThread() throws Exception {
+        setUp(QuicBidirectionalStreams.ENABLED);
+        String path = "/simple.txt";
+        String quicURL = QuicTestServer.getServerURL() + path;
+
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback() {
+            @Override
+            public void onWriteCompleted(
+                    BidirectionalStream stream, UrlResponseInfo info, ByteBuffer buffer) {
+                // Super class will write the next piece of data.
+                super.onWriteCompleted(stream, info, buffer);
+                // Only shut down the server exactly once.
+                if (!mSkipServerShutdown) {
+                    QuicTestServer.shutdownQuicTestServer();
+                    mSkipServerShutdown = true;
+                }
+            }
+        };
+
+        callback.addWriteData("Test String".getBytes());
+        callback.addWriteData("1234567890".getBytes());
+        callback.addWriteData("woot!".getBytes());
+
+        BidirectionalStream stream = new BidirectionalStream
+                                             .Builder(quicURL, callback, callback.getExecutor(),
+                                                     mTestFramework.mCronetEngine)
+                                             .addHeader("foo", "bar")
+                                             .addHeader("empty", "")
+                                             .addHeader("Content-Type", "zebra")
+                                             .build();
+        stream.start();
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        // Server terminated on us, so the stream must fail.
+        // QUIC reports this as QUIC_PROTOCOL_ERROR = -356.
+        assertNotNull(callback.mError);
+        assertEquals(-356, callback.mError.getCronetInternalErrorCode());

[kapishnikov, 2016/04/23 00:29:47]

We can replace '-356' with NetError.ERR_QUIC_PROTOCOL_ERROR

[xunjieli, 2016/04/23 10:39:00]

Done. 
Ah, I didn't know that there is a java class for NetError codes. Good to know! I
guess we need to refactor error codes mapping in
cronet/android/api/src/org/chromium/net/UrlRequestException.java.

+    }
 }