Fix DCHECK in history_backend by ensuring we clear redirect tracking state whenever the RenderView is told that a provisional load has started for the main frame.

The DCHECK(params->referrer == params->redirects[0]) was firing because:
a) page A was loaded, triggered WillPerformClientRedirect
b) after the provisional load started for the destination page of A's client redirect, but before this load was committed, the browser makes a Navigation request for page B.
c) When page B's load is committed, the RenderView's completed_client_redirect_src_ was still set, resulting in a CLIENT_REDIRECT transition type and forwarding the src value through params->referrer -- but params->redirects was now completely unrelated. Kaboom.

This fix should be general enough to handle cases (that are relatively likely in the wild) where WebKit legitimately cancels the redirect, instead of just the browser doing so. Note we can't depend on dispatchDidCancelClientRedirect because we get that callback on both completion and cancellation of a client redirect.

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 81 insertions, 6 deletions

diff --git a/chrome/browser/history/redirect_uitest.cc b/chrome/browser/history/redirect_uitest.cc
index 0ee58f7..dabeddb 100644
--- a/chrome/browser/history/redirect_uitest.cc
+++ b/chrome/browser/history/redirect_uitest.cc
@@ -251,4 +251,64 @@ TEST_F(RedirectTest, ClientFragments) {
 
   EXPECT_EQ(1, redirects.size());
   EXPECT_EQ(first_url.spec() + "#myanchor", redirects[0].spec());
-}
\ No newline at end of file
+}
+
+// TODO(timsteele): This is disabled because our current testserver can't
+// handle multiple requests in parallel, making it hang on the first request to
+// /slow?60. It's unable to serve our second request for files/title2.html until
+// /slow? completes, which doesn't give the desired behavior. We could
+// alternatively load the second page from disk, but we would need to start the
+// browser for this testcase with --process-per-tab, and I don't think we can do
+// this at test-case-level granularity at the moment.
+TEST_F(RedirectTest, DISABLED_ClientCancelledByNewNavigationAfterProvisionalLoad) {
+  // We want to initiate a second navigation after the provisional load for
+  // the client redirect destination has started, but before this load is
+  // committed. To achieve this, we tell the browser to load a slow page,
+  // which causes it to start a provisional load, and while it is waiting
+  // for the response (which means it hasn't committed the load for the client
+  // redirect destination page yet), we issue a new navigation request.
+  TestServer server(kDocRoot);
+  
+  GURL final_url = server.TestServerPageW(std::wstring(L"files/title2.html"));
+  GURL slow = server.TestServerPageW(std::wstring(L"slow?60"));
+  GURL first_url = server.TestServerPageW(
+      std::wstring(L"client-redirect?") + UTF8ToWide(slow.spec()));
+  std::vector<GURL> redirects;
+
+  NavigateToURL(first_url);
+  // We don't sleep here - the first navigation won't have been committed yet
+  // because we told the server to wait a minute. This means the browser has
+  // started it's provisional load for the client redirect destination page but
+  // hasn't completed. Our time is now!
+  NavigateToURL(final_url);
+  
+  std::wstring tab_title;
+  std::wstring final_url_title = L"Title Of Awesomeness";
+  // Wait till the final page has been loaded.
+  for (int i = 0; i < 10; ++i) {
+    Sleep(kWaitForActionMaxMsec / 10);
+    scoped_ptr<TabProxy> tab_proxy(GetActiveTab());
+    ASSERT_TRUE(tab_proxy.get());
+    ASSERT_TRUE(tab_proxy->GetTabTitle(&tab_title));
+    if (tab_title == final_url_title) {
+      ASSERT_TRUE(tab_proxy->GetRedirectsFrom(first_url, &redirects));
+      break;
+    }
+  }
+
+  // Check to make sure the navigation did in fact take place and we are 
+  // at the expected page.
+  EXPECT_EQ(final_url_title, tab_title); 
+
+  bool final_navigation_not_redirect = true;
+  // Check to make sure our request for files/title2.html doesn't get flagged
+  // as a client redirect from the first (/client-redirect?) page.
+  for (std::vector<GURL>::iterator it = redirects.begin();
+       it != redirects.end(); ++it) {
+    if (final_url.spec() == it->spec()) {
+      final_navigation_not_redirect = false;
+      break;
+    }
+  }
+  EXPECT_TRUE(final_navigation_not_redirect);
+}
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index 519e37d..28a9195 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -1015,6 +1015,7 @@ void RenderView::UpdateURL(WebFrame* frame) {
     // the page contained a client redirect (meta refresh, document.loc...),
     // so we set the referrer and transition to match.
     if (completed_client_redirect_src_.is_valid()) {
+      DCHECK(completed_client_redirect_src_ == params.redirects[0]);
       params.referrer = completed_client_redirect_src_;
       params.transition = static_cast<PageTransition::Type>(
           params.transition | PageTransition::CLIENT_REDIRECT);
@@ -1146,8 +1147,12 @@ void RenderView::DidStartProvisionalLoadForFrame(
     WebView* webview,
     WebFrame* frame,
     NavigationGesture gesture) {
-  if (webview->GetMainFrame() == frame)
+  if (webview->GetMainFrame() == frame) {
     navigation_gesture_ = gesture;
+    
+    // Make sure redirect tracking state is clear for the new load.
+    completed_client_redirect_src_ = GURL();
+  }
 
   Send(new ViewHostMsg_DidStartProvisionalLoadForFrame(
        routing_id_, webview->GetMainFrame() == frame,
diff --git a/webkit/glue/webframeloaderclient_impl.cc b/webkit/glue/webframeloaderclient_impl.cc
index 232230f..ec1ff4b 100644
--- a/webkit/glue/webframeloaderclient_impl.cc
+++ b/webkit/glue/webframeloaderclient_impl.cc
@@ -671,6 +671,7 @@ void WebFrameLoaderClient::dispatchDidStartProvisionalLoad() {
   // If this load is what we expected from a client redirect, treat it as a
   // redirect from that original page. The expected redirect urls will be
   // cleared by DidCancelClientRedirect.
+  bool completing_client_redirect = false;
   if (expected_client_redirect_src_.is_valid()) {
     // expected_client_redirect_dest_ could be something like 
     // "javascript:history.go(-1)" thus we need to exclude url starts with
@@ -678,15 +679,22 @@ void WebFrameLoaderClient::dispatchDidStartProvisionalLoad() {
     DCHECK(expected_client_redirect_dest_.SchemeIs("javascript") || 
            expected_client_redirect_dest_ == url);
     ds->AppendRedirect(expected_client_redirect_src_);
-    if (d)
-      d->DidCompleteClientRedirect(webview, webframe_, 
-                                   expected_client_redirect_src_);
+    completing_client_redirect = true;
   }
   ds->AppendRedirect(url);
 
-  if (d)
+  if (d) {
+    // As the comment for DidCompleteClientRedirect in webview_delegate.h
+    // points out, whatever information its invocation contains should only
+    // be considered relevant until the next provisional load has started.
+    // So we first tell the delegate that the load started, and then tell it
+    // about the client redirect the load is responsible for completing.
     d->DidStartProvisionalLoadForFrame(webview, webframe_,
                                        NavigationGestureForLastLoad());
+    if (completing_client_redirect)
+      d->DidCompleteClientRedirect(webview, webframe_, 
+                                   expected_client_redirect_src_);
+  }
 
   // Cancel any pending loads.
   if (alt_404_page_fetcher_.get())
diff --git a/webkit/glue/webview_delegate.h b/webkit/glue/webview_delegate.h
index f7aedd5..cf18ca5 100644
--- a/webkit/glue/webview_delegate.h
+++ b/webkit/glue/webview_delegate.h
@@ -404,6 +404,8 @@ class WebViewDelegate : virtual public WebWidgetDelegate {
 
   // Notifies the delegate that the load about to be committed for the specified
   // webview and frame was due to a client redirect originating from source URL.
+  // The information/notification obtained from this method is relevant until
+  // the next provisional load is started, at which point it becomes obsolete.
   virtual void DidCompleteClientRedirect(WebView* webview,
                                          WebFrame* frame,
                                          const GURL& source) {