[Cronet] Fix HttpURLConnection to not throw IllegalStateExceptions

Some methods like HttpURLConnection.getHeader() hide IOExceptions,
but later calls to getResponseCode() would then throw unexpected
IllegalStateExceptions. Instead getResponseCode() should rethrow
the previously hidden IOException.

Bug: 866911
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: Icc533d4a69875af5570a6f793463754d1bf477ab
Reviewed-on: https://chromium-review.googlesource.com/1148475Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577931}

components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java

@@ -27,6 +27,12 @@ class MessageLoop implements Executor {
     // this might cause the loop to terminate immediately if there is a quit
     // task enqueued.
     private boolean mLoopFailed = false;
+    // The exception that caused mLoopFailed to be set to true. Will be
+    // rethrown if loop() is called again. If mLoopFailed is set then
+    // exactly one of mPriorInterruptedIOException and mPriorRuntimeException
+    // will be set.
+    private InterruptedIOException mPriorInterruptedIOException;
+    private RuntimeException mPriorRuntimeException;
 
     // Used when assertions are enabled to enforce single-threaded use.
     private static final long INVALID_THREAD_ID = -1;
@@ -96,8 +102,11 @@ class MessageLoop implements Executor {
         long startNano = System.nanoTime();
         long timeoutNano = TimeUnit.NANOSECONDS.convert(timeoutMilli, TimeUnit.MILLISECONDS);
         if (mLoopFailed) {
-            throw new IllegalStateException(
-                    "Cannot run loop as an exception has occurred previously.");
+            if (mPriorInterruptedIOException != null) {
+                throw mPriorInterruptedIOException;
+            } else {
+                throw mPriorRuntimeException;
+            }
         }
         if (mLoopRunning) {
             throw new IllegalStateException(
@@ -111,9 +120,15 @@ class MessageLoop implements Executor {
                 } else {
                     take(true, timeoutNano - System.nanoTime() + startNano).run();
                 }
-            } catch (InterruptedIOException | RuntimeException e) {
+            } catch (InterruptedIOException e) {
+                mLoopRunning = false;
+                mLoopFailed = true;
+                mPriorInterruptedIOException = e;
+                throw e;
+            } catch (RuntimeException e) {
                 mLoopRunning = false;
                 mLoopFailed = true;
+                mPriorRuntimeException = e;
                 throw e;
             }
         }

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

@@ -38,6 +38,7 @@ import java.io.ByteArrayOutputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.InterruptedIOException;
 import java.io.OutputStream;
 import java.lang.reflect.Method;
 import java.net.HttpURLConnection;
@@ -1402,4 +1403,70 @@ public class CronetHttpURLConnectionTest {
         urlConnection.disconnect();
         assertTrue(CronetTestUtil.nativeGetTaggedBytes(tag) > priorBytes);
     }
+
+    @Test
+    @SmallTest
+    @Feature({"Cronet"})
+    @CompareDefaultWithCronet
+    public void testIOExceptionErrorRethrown() throws Exception {
+        // URL that should fail to connect.
+        URL url = new URL("http://localhost");
+        HttpURLConnection connection = (HttpURLConnection) url.openConnection();
+        // This should not throw, even though internally it may encounter an exception.
+        connection.getHeaderField("blah");
+        try {
+            // This should throw an IOException.
+            connection.getResponseCode();
+            fail();
+        } catch (IOException e) {
+            // Expected
+        }
+        connection.disconnect();
+    }
+
+    @Test
+    @SmallTest
+    @Feature({"Cronet"})
+    @CompareDefaultWithCronet
+    public void testIOExceptionInterruptRethrown() throws Exception {
+        // Older system impls don't reliably respond to interrupt.
+        if (mTestRule.testingSystemHttpURLConnection()
+                && Build.VERSION.SDK_INT < Build.VERSION_CODES.M) {
+            return;
+        }
+        ServerSocket hangingServer = new ServerSocket(0);
+        URL url = new URL("http://localhost:" + hangingServer.getLocalPort());
+        final HttpURLConnection connection = (HttpURLConnection) url.openConnection();
+        // connect() is non-blocking.
+        connection.connect();
+        FutureTask<IOException> task = new FutureTask<IOException>(new Callable<IOException>() {
+            @Override
+            public IOException call() {
+                // This should not throw, even though internally it may encounter an exception.
+                connection.getHeaderField("blah");
+                try {
+                    // This should throw an InterruptedIOException.
+                    connection.getResponseCode();
+                } catch (InterruptedIOException e) {
+                    // Expected
+                    return e;
+                } catch (IOException e) {
+                    return null;
+                }
+                return null;
+            }
+        });
+        Thread t = new Thread(task);
+        t.start();
+        Socket s = hangingServer.accept();
+        hangingServer.close();
+        // This will trigger an InterruptException in getHeaderField() and getResponseCode().
+        // getHeaderField() should not re-throw it.  getResponseCode() should re-throw it as an
+        // InterruptedIOException.
+        t.interrupt();
+        // Make sure an IOException is thrown.
+        assertNotNull(task.get());
+        s.close();
+        connection.disconnect();
+    }
 }

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

@@ -19,6 +19,7 @@ import org.chromium.base.test.util.Feature;
 import org.chromium.net.CronetTestRule;
 
 import java.io.IOException;
+import java.io.InterruptedIOException;
 import java.net.SocketTimeoutException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -78,7 +79,7 @@ public class MessageLoopTest {
                     loop.loop();
                     fail();
                 } catch (Exception e) {
-                    if (!(e instanceof IllegalStateException)) {
+                    if (!(e instanceof InterruptedIOException)) {
                         fail();
                     }
                 }
@@ -127,7 +128,7 @@ public class MessageLoopTest {
                     loop.loop();
                     fail();
                 } catch (Exception e) {
-                    if (!(e instanceof IllegalStateException)) {
+                    if (!(e instanceof NullPointerException)) {
                         fail();
                     }
                 }