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}

7 files changed, 117 insertions, 59 deletions

diff --git a/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java b/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
index 47d0f28..ba9385f 100644
--- a/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
+++ b/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.
diff --git a/android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java b/android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java
index 1a04af4..d9b709a 100644
--- a/android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java
+++ b/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
diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
index acc17bf..aeaa73e 100644
--- a/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
+++ b/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());
+    }
 }
diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java b/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java
index c701907..798a214 100644
--- a/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java
+++ b/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;
     }
@@ -201,6 +222,11 @@ public class TestAwContentsClient extends NullContentsClient {
     }
 
     @Override
+    public void onFailedLoadForTesting(String url) {
+        mOnFailedLoadHelper.notifyCalled(url);
+    }
+
+    @Override
     public void onReceivedError(int errorCode, String description, String failingUrl) {
         mOnReceivedErrorHelper.notifyCalled(errorCode, description, failingUrl);
     }
diff --git a/content/browser/web_contents/web_contents_impl_browsertest.cc b/content/browser/web_contents/web_contents_impl_browsertest.cc
index 9b229746..21bd98b 100644
--- a/content/browser/web_contents/web_contents_impl_browsertest.cc
+++ b/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
diff --git a/third_party/WebKit/Source/core/loader/DocumentLoader.cpp b/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
index 01b2985..245b086 100644
--- a/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
+++ b/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();
 }
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index 3f7b65e1..0d4559b 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/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();