Don't use didStopLoading in failed navigation when other nav in progress

For example if a JS redirect would fail while the original navigation is
still in progress we would call didStopLoading before the original
navigation calls didFinishLoading (breaking the assumption that
didFinishLoading is always called before didStopLoading).

BUG=557100, 298193

Review URL: https://codereview.chromium.org/1440933004

Cr-Commit-Position: refs/heads/master@{#362954}

android_webview/java/src/org/chromium/android_webview/AwContentsClient.java

@@ -334,6 +334,8 @@ public abstract class AwContentsClient {
 
     public abstract void onPageCommitVisible(String url);
 
+    public void onFailedLoadForTesting(String url) {}
+
     public final void onReceivedError(AwWebResourceRequest request, AwWebResourceError error) {
         // Only one of these callbacks actually reaches out the client code. The first callback
         // is used on API versions up to and including L, the second on subsequent releases.

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

@@ -81,6 +81,7 @@ public class AwWebContentsObserver extends WebContentsObserver {
             // AwContents.InterceptNavigationDelegateImpl.shouldIgnoreNavigation instead.
             client.getCallbackHelper().postOnPageFinished(failingUrl);
         }
+        client.onFailedLoadForTesting(failingUrl);
     }
 
     @Override

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

@@ -9,6 +9,7 @@ import android.util.Pair;
 
 import org.chromium.android_webview.AwContents;
 import org.chromium.android_webview.AwContentsClient;
+import org.chromium.android_webview.test.TestAwContentsClient.OnFailedLoadHelper;
 import org.chromium.android_webview.test.util.CommonResources;
 import org.chromium.android_webview.test.util.JSUtils;
 import org.chromium.android_webview.test.util.JavascriptEventObserver;
@@ -18,6 +19,7 @@ import org.chromium.base.test.util.Feature;
 import org.chromium.content.browser.test.util.CallbackHelper;
 import org.chromium.content.browser.test.util.DOMUtils;
 import org.chromium.content.browser.test.util.TestCallbackHelperContainer.OnEvaluateJavaScriptResultHelper;
+import org.chromium.content.browser.test.util.TestCallbackHelperContainer.OnPageFinishedHelper;
 import org.chromium.content.browser.test.util.TestCallbackHelperContainer.OnPageStartedHelper;
 import org.chromium.content.browser.test.util.TestCallbackHelperContainer.OnReceivedErrorHelper;
 import org.chromium.content_public.browser.LoadUrlParams;
@@ -37,6 +39,7 @@ public class AwContentsClientShouldOverrideUrlLoadingTest extends AwTestBase {
     private static final String DATA_URL = "data:text/html,<div/>";
     private static final String REDIRECT_TARGET_PATH = "/redirect_target.html";
     private static final String TITLE = "TITLE";
+    private static final String TAG = "AwContentsClientShouldOverrideUrlLoadingTest";
 
     private TestWebServer mWebServer;
     private TestAwContentsClient mContentsClient;
@@ -1120,4 +1123,80 @@ public class AwContentsClientShouldOverrideUrlLoadingTest extends AwTestBase {
             return mValue;
         }
     }
+
+    /**
+     * This is to test a bug where a JS redirect failing in its provisional state would prevent us
+     * from posting onPageFinished for the original page load.
+     * The original page contains an iframe so that we can commit the original load but then
+     * prevent it from finishing until the JS redirect fails by having the test server defer the
+     * response to the iframe.
+     */
+    @SmallTest
+    @Feature({"AndroidWebView"})
+    public void testOnPageFinishedOnFailedJSRedirect() throws Throwable {
+        final CountDownLatch jsRedirectSignal = new CountDownLatch(1);
+
+        final String redirectTargetPath = "/redirectTargetPath.html";
+        final String redirectTargetUrl = mWebServer.setResponse(
+                redirectTargetPath, CommonResources.makeHtmlPageFrom("", ""), null);
+
+        class DelayingOverrideClient extends TestAwContentsClient {
+            @Override
+            public boolean shouldOverrideUrlLoading(AwWebResourceRequest request) {
+                if (redirectTargetUrl.equals(request.url)) {
+                    try {
+                        // Wait here to make sure the load reaches its provisional state before we
+                        // cancel it. Waiting for a callback to the WebContentsObserver to make sure
+                        // we have reached the provisional state causes a deadlock here.
+                        Thread.sleep(Math.min(WAIT_TIMEOUT_MS / 2, 2000));
+                    } catch (InterruptedException e) {
+                    }
+                    return true;
+                }
+                return false;
+            }
+        }
+        mContentsClient = new DelayingOverrideClient();
+        setupWithProvidedContentsClient(mContentsClient);
+
+        final String redirectJs = "window.location.href='" + redirectTargetUrl + "';";
+
+        final String iframePath = "/iframePath.html";
+        final String iframeUrl = mWebServer.setResponseWithRunnableAction(
+                iframePath, CommonResources.makeHtmlPageFrom("", ""), null, new Runnable() {
+                    @Override
+                    public void run() {
+                        try {
+                            mAwContents.evaluateJavaScriptForTests(redirectJs, null);
+                            jsRedirectSignal.await();
+                        } catch (InterruptedException e) {
+                        }
+                    }
+                });
+        final String iframeJs = "<iframe src='" + iframeUrl + "'></iframe>";
+
+        String startPage = makeHtmlPageFrom("", iframeJs);
+        final String startPath = "/startPath.html";
+        final String startUrl = addPageToTestServer(startPath, startPage);
+
+        enableJavaScriptOnUiThread(mAwContents);
+
+        OnPageFinishedHelper onPageFinishedHelper = mContentsClient.getOnPageFinishedHelper();
+        int onPageFinishedCallCount = onPageFinishedHelper.getCallCount();
+
+        OnFailedLoadHelper onFailedLoadHelper = mContentsClient.getOnFailedLoadHelper();
+        int onFailedLoadCallCount = onFailedLoadHelper.getCallCount();
+
+        // load start url -> iframe -> JS redirect -> fail JS redirect -> finish start URL
+        loadUrlAsync(mAwContents, startUrl);
+
+        onFailedLoadHelper.waitForCallback(onFailedLoadCallCount);
+        assertEquals(redirectTargetUrl, onFailedLoadHelper.getUrl());
+
+        // let iframe finish
+        jsRedirectSignal.countDown();
+
+        onPageFinishedHelper.waitForCallback(onPageFinishedCallCount);
+        assertEquals(startUrl, onPageFinishedHelper.getUrl());
+    }
 }

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

@@ -29,6 +29,7 @@ public class TestAwContentsClient extends NullContentsClient {
     private boolean mAllowSslError;
     private final OnPageStartedHelper mOnPageStartedHelper;
     private final OnPageFinishedHelper mOnPageFinishedHelper;
+    private final OnFailedLoadHelper mOnFailedLoadHelper;
     private final OnPageCommitVisibleHelper mOnPageCommitVisibleHelper;
     private final OnReceivedErrorHelper mOnReceivedErrorHelper;
     private final OnReceivedError2Helper mOnReceivedError2Helper;
@@ -49,6 +50,7 @@ public class TestAwContentsClient extends NullContentsClient {
         super(ThreadUtils.getUiThreadLooper());
         mOnPageStartedHelper = new OnPageStartedHelper();
         mOnPageFinishedHelper = new OnPageFinishedHelper();
+        mOnFailedLoadHelper = new OnFailedLoadHelper();
         mOnPageCommitVisibleHelper = new OnPageCommitVisibleHelper();
         mOnReceivedErrorHelper = new OnReceivedErrorHelper();
         mOnReceivedError2Helper = new OnReceivedError2Helper();
@@ -79,6 +81,25 @@ public class TestAwContentsClient extends NullContentsClient {
         return mOnPageFinishedHelper;
     }
 
+    /**
+     * CallbackHelper for OnFailedLoad.
+     */
+    public static class OnFailedLoadHelper extends CallbackHelper {
+        private String mUrl;
+        public void notifyCalled(String url) {
+            mUrl = url;
+            notifyCalled();
+        }
+        public String getUrl() {
+            assert getCallCount() > 0;
+            return mUrl;
+        }
+    }
+
+    public OnFailedLoadHelper getOnFailedLoadHelper() {
+        return mOnFailedLoadHelper;
+    }
+
     public OnReceivedErrorHelper getOnReceivedErrorHelper() {
         return mOnReceivedErrorHelper;
     }
@@ -200,6 +221,11 @@ public class TestAwContentsClient extends NullContentsClient {
         mOnPageFinishedHelper.notifyCalled(url);
     }
 
+    @Override
+    public void onFailedLoadForTesting(String url) {
+        mOnFailedLoadHelper.notifyCalled(url);
+    }
+
     @Override
     public void onReceivedError(int errorCode, String description, String failingUrl) {
         mOnReceivedErrorHelper.notifyCalled(errorCode, description, failingUrl);

content/browser/web_contents/web_contents_impl_browsertest.cc

@@ -187,60 +187,6 @@ class LoadingStateChangedDelegate : public WebContentsDelegate {
   int loadingStateToDifferentDocumentCount_;
 };
 
-// See: http://crbug.com/298193
-#if defined(OS_WIN) || defined(OS_LINUX)
-#define MAYBE_DidStopLoadingDetails DISABLED_DidStopLoadingDetails
-#else
-#define MAYBE_DidStopLoadingDetails DidStopLoadingDetails
-#endif
-
-// Test that DidStopLoading includes the correct URL in the details.
-IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
-                       MAYBE_DidStopLoadingDetails) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-
-  LoadStopNotificationObserver load_observer(
-      &shell()->web_contents()->GetController());
-  NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html"));
-  load_observer.Wait();
-
-  EXPECT_EQ("/title1.html", load_observer.url_.path());
-  EXPECT_EQ(0, load_observer.session_index_);
-  EXPECT_EQ(&shell()->web_contents()->GetController(),
-            load_observer.controller_);
-}
-
-// See: http://crbug.com/298193
-#if defined(OS_WIN) || defined(OS_LINUX)
-#define MAYBE_DidStopLoadingDetailsWithPending \
-  DISABLED_DidStopLoadingDetailsWithPending
-#else
-#define MAYBE_DidStopLoadingDetailsWithPending DidStopLoadingDetailsWithPending
-#endif
-
-// Test that DidStopLoading includes the correct URL in the details when a
-// pending entry is present.
-IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
-                       MAYBE_DidStopLoadingDetailsWithPending) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-  GURL url("data:text/html,<div>test</div>");
-
-  // Listen for the first load to stop.
-  LoadStopNotificationObserver load_observer(
-      &shell()->web_contents()->GetController());
-  // Start a new pending navigation as soon as the first load commits.
-  // We will hear a DidStopLoading from the first load as the new load
-  // is started.
-  NavigateOnCommitObserver commit_observer(
-      shell(), embedded_test_server()->GetURL("/title2.html"));
-  NavigateToURL(shell(), url);
-  load_observer.Wait();
-
-  EXPECT_EQ(url, load_observer.url_);
-  EXPECT_EQ(0, load_observer.session_index_);
-  EXPECT_EQ(&shell()->web_contents()->GetController(),
-            load_observer.controller_);
-}
 // Test that a renderer-initiated navigation to an invalid URL does not leave
 // around a pending entry that could be used in a URL spoof.  We test this in
 // a browser test because our unit test framework incorrectly calls

third_party/WebKit/Source/core/loader/DocumentLoader.cpp

@@ -218,7 +218,8 @@ void DocumentLoader::mainReceivedError(const ResourceError& error)
     if (!frameLoader())
         return;
     m_mainDocumentError = error;
-    m_state = MainResourceDone;
+    if (m_state < MainResourceDone)
+        m_state = MainResourceDone;
     frameLoader()->receivedMainResourceError(this, error);
     clearMainResourceHandle();
 }

third_party/WebKit/Source/core/loader/FrameLoader.cpp

@@ -1215,15 +1215,18 @@ void FrameLoader::receivedMainResourceError(DocumentLoader* loader, const Resour
         if (loader != m_provisionalDocumentLoader)
             return;
         detachDocumentLoader(m_provisionalDocumentLoader);
-        m_progressTracker->progressCompleted();
+        // If the provisional load failed, and we haven't yet rendered anything
+        // into the frame, then act as though the non-provisional loader failed
+        // as well. If we don't do this, the main load will never finish.
+        if (!stateMachine()->committedFirstRealDocumentLoad())
+            m_documentLoader->setSentDidFinishLoad();
     } else {
         ASSERT(loader == m_documentLoader);
         if (m_frame->document()->parser())
             m_frame->document()->parser()->stopParsing();
-        m_documentLoader->setSentDidFinishLoad();
-        if (!m_provisionalDocumentLoader && m_frame->isLoading()) {
+        if (!m_documentLoader->sentDidFinishLoad()) {
             client()->dispatchDidFailLoad(error, historyCommitType);
-            m_progressTracker->progressCompleted();
+            m_documentLoader->setSentDidFinishLoad();
         }
     }
     checkCompleted();