Call onPageStarted, onPageFinished for HTTP-error error pages

Previously HTTP errors will not result in an error page being committed
immediately (instead the renderer will trigger an error page navigation
after the original committed navigation), so onPageStarted and
onPageFinished will be called for http error navigations.

After crrev.com/c/2487024, it's possible for navigations with an error
HTTP status code and an empty response body to immediately result in an
error page navigation. In this case, we should ensure that we still
call onPageStarted and onPageFinished to be consistent with other HTTP
error cases.

Bug: 1147227
Change-Id: I595c411ef61bd8bb8c6b2bf2779b7b29ef18dd07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2528511Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826050}

android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java

@@ -82,10 +82,19 @@ public class AwWebContentsObserver extends WebContentsObserver {
         String unreachableWebDataUrl = AwContentsStatics.getUnreachableWebDataUrl();
         boolean isErrorUrl =
                 unreachableWebDataUrl != null && unreachableWebDataUrl.equals(failingUrl);
-        if (isMainFrame && !isErrorUrl && errorCode == NetError.ERR_ABORTED) {
-            // Need to call onPageFinished for backwards compatibility with the classic webview.
-            // See also AwContents.IoThreadClientImpl.onReceivedError.
-            client.getCallbackHelper().postOnPageFinished(failingUrl);
+        if (isMainFrame && !isErrorUrl) {
+            if (errorCode == NetError.ERR_ABORTED) {
+                // Need to call onPageFinished for backwards compatibility with the classic webview.
+                // See also AwContentsClientBridge.onReceivedError.
+                client.getCallbackHelper().postOnPageFinished(failingUrl);
+            } else if (errorCode == NetError.ERR_HTTP_RESPONSE_CODE_FAILURE) {
+                // This is a HTTP error that results in an error page. We need to call onPageStarted
+                // and onPageFinished to have the same behavior with HTTP error navigations that
+                // don't result in an error page. See also
+                // AwContentsClientBridge.onReceivedHttpError.
+                client.getCallbackHelper().postOnPageStarted(failingUrl);
+                client.getCallbackHelper().postOnPageFinished(failingUrl);
+            }
         }
     }
 

android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java

@@ -23,6 +23,7 @@ import org.chromium.android_webview.test.util.AwTestTouchUtils;
 import org.chromium.android_webview.test.util.CommonResources;
 import org.chromium.base.test.util.Feature;
 import org.chromium.components.embedder_support.util.WebResourceResponseInfo;
+import org.chromium.content_public.browser.test.util.TestCallbackHelperContainer;
 import org.chromium.net.test.util.TestWebServer;
 
 import java.util.ArrayList;
@@ -262,4 +263,89 @@ public class ClientOnReceivedHttpErrorTest {
         Assert.assertEquals(
                 "text/html; charset=utf-8", response.getResponseHeaders().get("Content-Type"));
     }
+
+    @Test
+    @SmallTest
+    @Feature({"AndroidWebView"})
+    public void testOnPageStartedAndFinishedEmpty() throws Throwable {
+        useDefaultTestAwContentsClient();
+        TestCallbackHelperContainer.OnPageStartedHelper onPageStartedHelper =
+                mContentsClient.getOnPageStartedHelper();
+        TestCallbackHelperContainer.OnPageFinishedHelper onPageFinishedHelper =
+                mContentsClient.getOnPageFinishedHelper();
+        TestAwContentsClient.OnReceivedHttpErrorHelper onReceivedHttpErrorHelper =
+                mContentsClient.getOnReceivedHttpErrorHelper();
+        final String badUrl = mWebServer.getResponseUrl("/404.html");
+        final String goodUrl =
+                mWebServer.setResponse("/good.html", CommonResources.ABOUT_HTML, null);
+        final int initialOnHttpErrorCount = onReceivedHttpErrorHelper.getCallCount();
+        final int initialOnPageStartedCount = onPageStartedHelper.getCallCount();
+        final int initialOnPageFinishedCount = onPageFinishedHelper.getCallCount();
+
+        // Navigate to a URL that doesn't exist.
+        mActivityTestRule.loadUrlSync(mAwContents, onPageFinishedHelper, badUrl);
+        Assert.assertEquals("onReceivedHttpErrorHelper should be called once",
+                initialOnHttpErrorCount + 1, onReceivedHttpErrorHelper.getCallCount());
+        AwWebResourceRequest request = onReceivedHttpErrorHelper.getRequest();
+        Assert.assertNotNull("onReceivedHttpError should have a non-null request", request);
+        Assert.assertEquals(badUrl, request.url);
+        Assert.assertEquals("onPageStartedHelper should be called once",
+                initialOnPageStartedCount + 1, onPageStartedHelper.getCallCount());
+        Assert.assertEquals(badUrl, onPageStartedHelper.getUrl());
+        Assert.assertEquals("onPageFinishedHelper should be called once",
+                initialOnPageFinishedCount + 1, onPageFinishedHelper.getCallCount());
+        Assert.assertEquals(badUrl, onPageFinishedHelper.getUrl());
+
+        // Rather than wait a fixed time to see that additional callbacks for badUrl aren't
+        // called, we load another valid page since callbacks arrive sequentially.
+        mActivityTestRule.loadUrlSync(mAwContents, onPageFinishedHelper, goodUrl);
+        Assert.assertEquals(initialOnHttpErrorCount + 1, onReceivedHttpErrorHelper.getCallCount());
+        Assert.assertEquals(initialOnPageStartedCount + 2, onPageStartedHelper.getCallCount());
+        Assert.assertEquals(goodUrl, onPageStartedHelper.getUrl());
+        Assert.assertEquals(initialOnPageFinishedCount + 2, onPageFinishedHelper.getCallCount());
+        Assert.assertEquals(goodUrl, onPageFinishedHelper.getUrl());
+    }
+
+    @Test
+    @SmallTest
+    @Feature({"AndroidWebView"})
+    public void testOnPageStartedAndFinishedNonEmpty() throws Throwable {
+        useDefaultTestAwContentsClient();
+        TestCallbackHelperContainer.OnPageStartedHelper onPageStartedHelper =
+                mContentsClient.getOnPageStartedHelper();
+        TestCallbackHelperContainer.OnPageFinishedHelper onPageFinishedHelper =
+                mContentsClient.getOnPageFinishedHelper();
+        TestAwContentsClient.OnReceivedHttpErrorHelper onReceivedHttpErrorHelper =
+                mContentsClient.getOnReceivedHttpErrorHelper();
+        final String badUrl = mWebServer.setResponseWithNotFoundStatus("/404.html");
+        final String goodUrl =
+                mWebServer.setResponse("/good.html", CommonResources.ABOUT_HTML, null);
+        final int initialOnHttpErrorCount = onReceivedHttpErrorHelper.getCallCount();
+        final int initialOnPageStartedCount = onPageStartedHelper.getCallCount();
+        final int initialOnPageFinishedCount = onPageFinishedHelper.getCallCount();
+
+        // Navigate to a URL that 404s but has a non-empty body (because
+        // setResponseWithNotFoundStatus will add some content to 404 responses).
+        mActivityTestRule.loadUrlSync(mAwContents, onPageFinishedHelper, badUrl);
+        Assert.assertEquals("onReceivedHttpErrorHelper should be called once",
+                initialOnHttpErrorCount + 1, onReceivedHttpErrorHelper.getCallCount());
+        AwWebResourceRequest request = onReceivedHttpErrorHelper.getRequest();
+        Assert.assertNotNull("onReceivedHttpError should have a non-null request", request);
+        Assert.assertEquals(badUrl, request.url);
+        Assert.assertEquals("onPageStartedHelper should be called once",
+                initialOnPageStartedCount + 1, onPageStartedHelper.getCallCount());
+        Assert.assertEquals(badUrl, onPageStartedHelper.getUrl());
+        Assert.assertEquals("onPageFinishedHelper should be called once",
+                initialOnPageFinishedCount + 1, onPageFinishedHelper.getCallCount());
+        Assert.assertEquals(badUrl, onPageFinishedHelper.getUrl());
+
+        // Rather than wait a fixed time to see that additional callbacks for badUrl aren't
+        // called, we load another valid page since callbacks arrive sequentially.
+        mActivityTestRule.loadUrlSync(mAwContents, onPageFinishedHelper, goodUrl);
+        Assert.assertEquals(initialOnHttpErrorCount + 1, onReceivedHttpErrorHelper.getCallCount());
+        Assert.assertEquals(initialOnPageStartedCount + 2, onPageStartedHelper.getCallCount());
+        Assert.assertEquals(goodUrl, onPageStartedHelper.getUrl());
+        Assert.assertEquals(initialOnPageFinishedCount + 2, onPageFinishedHelper.getCallCount());
+        Assert.assertEquals(goodUrl, onPageFinishedHelper.getUrl());
+    }
 }