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

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

 // Copyright 2016 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;
 
 import android.test.suitebuilder.annotation.SmallTest;
 
 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,
     }
 
     private void setUp(QuicBidirectionalStreams enabled) throws Exception {
         // Load library first to create MockCertVerifier.
         System.loadLibrary("cronet_tests");
         CronetEngine.Builder builder = new CronetEngine.Builder(getContext());
 
@@ skipping 11 matching lines @@
                 QuicTestServer.getServerPort());
 
         builder.setMockCertVerifierForTesting(QuicTestServer.createMockCertVerifier());
 
         mTestFramework = startCronetTestFrameworkWithUrlAndCronetEngineBuilder(null, builder);
         registerHostResolver(mTestFramework);
     }
 
     @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();
     }
 
     @SmallTest
     @Feature({"Cronet"})
     @OnlyRunNativeCronet
     // Test that QUIC is negotiated.
     public void testSimpleGet() throws Exception {
         setUp(QuicBidirectionalStreams.ENABLED);
         String path = "/simple.txt";
@@ skipping 25 matching lines @@
                                              .Builder(quicURL, callback, callback.getExecutor(),
                                                      mTestFramework.mCronetEngine)
                                              .setHttpMethod("GET")
                                              .build();
         stream.start();
         callback.blockForDone();
         assertTrue(stream.isDone());
         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.

+    }
 }