CronetUrlRequestTest#testFailures

CronetUrlRequestTest#testFailures() simulates callback
failures in each response step. However, the response step
is not updated in onFailed, so maybeThrowCancelOrPause()
will throw an exception again. This results in a flake
in the Java implementation.

This CL updates response step in TestUrlRequestCallback,
and adds two more tests to make sure throwing exception
or canceling does nothing in terminal callbacks (onFailed,
onCanceled, onSucceeded)

R=kapishnikov@chromium.org
BUG=635026

Review-Url: https://codereview.chromium.org/2292113002
Cr-Commit-Position: refs/heads/master@{#415764}

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

@@ -63,6 +63,8 @@ public class CronetUrlRequestTest extends CronetTestBase {
         UrlRequest urlRequest = builder.build();
         urlRequest.start();
         callback.blockForDone();
+        // Wait for all posted tasks to be executed to ensure there is no unhandled exception.
+        callback.shutdownExecutorAndWait();
         assertTrue(urlRequest.isDone());
         return callback;
     }
@@ -627,7 +629,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
         assertEquals(arbitraryNetError, callback.mError.getCronetInternalErrorCode());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.NOTHING);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     @SmallTest
@@ -644,7 +646,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
         assertEquals(arbitraryNetError, callback.mError.getCronetInternalErrorCode());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.ON_RESPONSE_STARTED);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     @SmallTest
@@ -661,7 +663,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
         assertEquals(arbitraryNetError, callback.mError.getCronetInternalErrorCode());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.ON_RESPONSE_STARTED);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     /**
@@ -696,7 +698,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
         assertEquals(-201, callback.mError.getCronetInternalErrorCode());
         assertEquals("Exception in CronetUrlRequest: net::ERR_CERT_DATE_INVALID",
                 callback.mError.getMessage());
-        assertEquals(callback.mResponseStep, ResponseStep.NOTHING);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     /**
@@ -1571,8 +1573,14 @@ public class CronetUrlRequestTest extends CronetTestBase {
         UrlRequest urlRequest = builder.build();
         urlRequest.start();
         callback.blockForDone();
+        // Wait for all posted tasks to be executed to ensure there is no unhandled exception.
+        callback.shutdownExecutorAndWait();
         assertEquals(1, callback.mRedirectCount);
-        assertEquals(callback.mResponseStep, failureStep);
+        if (failureType == FailureType.CANCEL_SYNC || failureType == FailureType.CANCEL_ASYNC) {
+            assertEquals(ResponseStep.ON_CANCELED, callback.mResponseStep);
+        } else if (failureType == FailureType.THROW_SYNC) {
+            assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
+        }
         assertTrue(urlRequest.isDone());
         assertEquals(expectResponseInfo, callback.mResponseInfo != null);
         assertEquals(expectError, callback.mError != null);
@@ -1616,20 +1624,83 @@ public class CronetUrlRequestTest extends CronetTestBase {
 
     @SmallTest
     @Feature({"Cronet"})
-    public void testThrowON_SUCCEEDED() {
+    public void testThrowOrCancelInOnSucceeded() {
+        FailureType[] testTypes = new FailureType[] {
+                FailureType.THROW_SYNC, FailureType.CANCEL_SYNC, FailureType.CANCEL_ASYNC};
+        for (FailureType type : testTypes) {
             TestUrlRequestCallback callback = new TestUrlRequestCallback();
-        callback.setFailure(FailureType.THROW_SYNC, ResponseStep.ON_SUCCEEDED);
-        UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getRedirectURL(),
+            callback.setFailure(type, ResponseStep.ON_SUCCEEDED);
+            UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getEchoMethodURL(),
                     callback, callback.getExecutor(), mTestFramework.mCronetEngine);
             UrlRequest urlRequest = builder.build();
             urlRequest.start();
             callback.blockForDone();
-        assertEquals(1, callback.mRedirectCount);
-        assertEquals(callback.mResponseStep, ResponseStep.ON_SUCCEEDED);
+            // Wait for all posted tasks to be executed to ensure there is no unhandled exception.
+            callback.shutdownExecutorAndWait();
+            assertNull(callback.mError);
+            assertEquals(ResponseStep.ON_SUCCEEDED, callback.mResponseStep);
             assertTrue(urlRequest.isDone());
             assertNotNull(callback.mResponseInfo);
-        assertNull(callback.mError);
             assertFalse(callback.mOnErrorCalled);
+            assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+            assertEquals("GET", callback.mResponseAsString);
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testThrowOrCancelInOnFailed() {
+        FailureType[] testTypes = new FailureType[] {
+                FailureType.THROW_SYNC, FailureType.CANCEL_SYNC, FailureType.CANCEL_ASYNC};
+        for (FailureType type : testTypes) {
+            String url = NativeTestServer.getEchoBodyURL();
+            // Shut down NativeTestServer so request will fail.
+            NativeTestServer.shutdownNativeTestServer();
+            TestUrlRequestCallback callback = new TestUrlRequestCallback();
+            callback.setFailure(type, ResponseStep.ON_FAILED);
+            UrlRequest.Builder builder = new UrlRequest.Builder(
+                    url, callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+            UrlRequest urlRequest = builder.build();
+            urlRequest.start();
+            callback.blockForDone();
+            // Wait for all posted tasks to be executed to ensure there is no unhandled exception.
+            callback.shutdownExecutorAndWait();
+            assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
+            assertTrue(callback.mOnErrorCalled);
+            assertNotNull(callback.mError);
+            assertTrue(urlRequest.isDone());
+            // Start NativeTestServer again to run the test for a second time.
+            assertTrue(NativeTestServer.startNativeTestServer(getContext()));
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testThrowOrCancelInOnCanceled() {
+        FailureType[] testTypes = new FailureType[] {
+                FailureType.THROW_SYNC, FailureType.CANCEL_SYNC, FailureType.CANCEL_ASYNC};
+        for (FailureType type : testTypes) {
+            TestUrlRequestCallback callback = new TestUrlRequestCallback() {
+                @Override
+                public void onResponseStarted(UrlRequest request, UrlResponseInfo info) {
+                    super.onResponseStarted(request, info);
+                    request.cancel();
+                }
+            };
+            callback.setFailure(type, ResponseStep.ON_CANCELED);
+            UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getEchoBodyURL(),
+                    callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+            UrlRequest urlRequest = builder.build();
+            urlRequest.start();
+            callback.blockForDone();
+            // Wait for all posted tasks to be executed to ensure there is no unhandled exception.
+            callback.shutdownExecutorAndWait();
+            assertEquals(ResponseStep.ON_CANCELED, callback.mResponseStep);
+            assertTrue(urlRequest.isDone());
+            assertNotNull(callback.mResponseInfo);
+            assertNull(callback.mError);
+            assertTrue(callback.mOnCanceledCalled);
+        }
     }
 
     @SmallTest
@@ -1837,7 +1908,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
                 "Exception in CronetUrlRequest: net::ERR_" + name, callback.mError.getMessage());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.NOTHING);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     // Returns the contents of byteBuffer, from its position() to its limit(),

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

@@ -19,6 +19,7 @@ import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Callback that tracks information from different callbacks and and has a
@@ -45,6 +46,8 @@ class TestUrlRequestCallback extends UrlRequest.Callback {
     // When false, the consumer is responsible for all calls into the request
     // that advance it.
     private boolean mAutoAdvance = true;
+    // Whether an exception is thrown by maybeThrowCancelOrPause().
+    private boolean mListenerExceptionThrown;
 
     // Conditionally fail on certain steps.
     private FailureType mFailureType = FailureType.NONE;
@@ -91,7 +94,9 @@ class TestUrlRequestCallback extends UrlRequest.Callback {
         ON_RECEIVED_REDIRECT,
         ON_RESPONSE_STARTED,
         ON_READ_COMPLETED,
-        ON_SUCCEEDED
+        ON_SUCCEEDED,
+        ON_FAILED,
+        ON_CANCELED,
     }
 
     public enum FailureType {
@@ -130,6 +135,21 @@ class TestUrlRequestCallback extends UrlRequest.Callback {
         mExecutorService.shutdown();
     }
 
+    /**
+     * Shuts down the ExecutorService and waits until it executes all posted
+     * tasks.
+     */
+    public void shutdownExecutorAndWait() {
+        mExecutorService.shutdown();
+        try {
+            // Termination shouldn't take long. Use 1 min which should be more than enough.
+            mExecutorService.awaitTermination(1, TimeUnit.MINUTES);
+        } catch (InterruptedException e) {
+            assertTrue("ExecutorService is interrupted while waiting for termination", false);
+        }
+        assertTrue(mExecutorService.isTerminated());
+    }
+
     @Override
     public void onRedirectReceived(
             UrlRequest request, UrlResponseInfo info, String newLocationUrl) {
@@ -218,7 +238,7 @@ class TestUrlRequestCallback extends UrlRequest.Callback {
         assertFalse(mOnErrorCalled);
         assertFalse(mOnCanceledCalled);
         assertNull(mError);
-        if (mFailureType == FailureType.THROW_SYNC) {
+        if (mListenerExceptionThrown) {
             assertEquals(UrlRequestError.LISTENER_EXCEPTION_THROWN, error.getErrorCode());
             assertEquals(0, error.getCronetInternalErrorCode());
             assertEquals("Exception received from UrlRequest.Callback", error.getMessage());
@@ -228,6 +248,7 @@ class TestUrlRequestCallback extends UrlRequest.Callback {
             assertFalse(error.immediatelyRetryable());
         }
 
+        mResponseStep = ResponseStep.ON_FAILED;
         mOnErrorCalled = true;
         mError = error;
         openDone();
@@ -243,6 +264,7 @@ class TestUrlRequestCallback extends UrlRequest.Callback {
         assertFalse(mOnErrorCalled);
         assertNull(mError);
 
+        mResponseStep = ResponseStep.ON_CANCELED;
         mOnCanceledCalled = true;
         openDone();
         maybeThrowCancelOrPause(request);
@@ -273,6 +295,7 @@ class TestUrlRequestCallback extends UrlRequest.Callback {
      * request.
      */
     private boolean maybeThrowCancelOrPause(final UrlRequest request) {
+        assertEquals(mExecutorThread, Thread.currentThread());
         if (mResponseStep != mFailureStep || mFailureType == FailureType.NONE) {
             if (!mAutoAdvance) {
                 mStepBlock.open();
@@ -282,6 +305,8 @@ class TestUrlRequestCallback extends UrlRequest.Callback {
         }
 
         if (mFailureType == FailureType.THROW_SYNC) {
+            assertFalse(mListenerExceptionThrown);
+            mListenerExceptionThrown = true;
             throw new IllegalStateException("Listener Exception.");
         }
         Runnable task = new Runnable() {