The new cross-process client-redirect logic is replacing the current navigation entry, which is correct if the redirect happens while the page is still loading but incorrect if the page has already finished loading. For each cross-site navigation, we further check whether the redirection happens before or after page finishes loading.

BUG=164419

Review URL: https://chromiumcodereview.appspot.com/11446070

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

4 files changed, 39 insertions, 28 deletions

diff --git a/chrome/browser/extensions/isolated_app_browsertest.cc b/chrome/browser/extensions/isolated_app_browsertest.cc
index 0e80417..de74087 100644
--- a/chrome/browser/extensions/isolated_app_browsertest.cc
+++ b/chrome/browser/extensions/isolated_app_browsertest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/automation/automation_util.h"
 #include "chrome/browser/extensions/extension_apitest.h"
@@ -90,7 +91,7 @@ IN_PROC_BROWSER_TEST_F(IsolatedAppTest, CrossProcessClientRedirect) {
       browser(), base_url.Resolve("app1/main.html"),
       CURRENT_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
 
-  // redirect to app2.
+  // Redirect to app2.
   GURL redirect_url(test_server()->GetURL(
       "client-redirect?files/extensions/isolated_apps/app2/main.html"));
   ui_test_utils::NavigateToURLWithDisposition(
@@ -100,7 +101,37 @@ IN_PROC_BROWSER_TEST_F(IsolatedAppTest, CrossProcessClientRedirect) {
   // Go back twice.
   // If bug fixed, we cannot go back anymore.
   // If not fixed, we will redirect back to app2 and can go back again.
+  EXPECT_TRUE(chrome::CanGoBack(browser()));
   chrome::GoBack(browser(), CURRENT_TAB);
+  EXPECT_TRUE(chrome::CanGoBack(browser()));
+  chrome::GoBack(browser(), CURRENT_TAB);
+  EXPECT_FALSE(chrome::CanGoBack(browser()));
+
+  // We also need to test script-initialized navigation (document.location.href)
+  // happened after page finishes loading. This one will also triggered the
+  // willPerformClientRedirect hook in RenderViewImpl but should not replace
+  // the previous history entry.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), base_url.Resolve("non_app/main.html"),
+      NEW_FOREGROUND_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+  WebContents* tab0 = chrome::GetWebContentsAt(browser(), 1);
+  RenderViewHost* rvh = tab0->GetRenderViewHost();
+
+  // Using JavaScript to navigate to app2 page,
+  // after the non_app page has finished loading.
+  content::WindowedNotificationObserver observer1(
+      content::NOTIFICATION_LOAD_STOP,
+      content::Source<NavigationController>(
+          &chrome::GetActiveWebContents(browser())->GetController()));
+  std::string script = base::StringPrintf(
+        "document.location.href=\"%s\";",
+        base_url.Resolve("app2/main.html").spec().c_str());
+  EXPECT_TRUE(ExecuteJavaScript(rvh, L"", ASCIIToWide(script)));
+  observer1.Wait();
+
+  // This kind of navigation should not replace previous navigation entry.
+  EXPECT_TRUE(chrome::CanGoBack(browser()));
   chrome::GoBack(browser(), CURRENT_TAB);
   EXPECT_FALSE(chrome::CanGoBack(browser()));
 }
diff --git a/content/public/renderer/navigation_state.cc b/content/public/renderer/navigation_state.cc
index 37fcfb7..9208efa 100644
--- a/content/public/renderer/navigation_state.cc
+++ b/content/public/renderer/navigation_state.cc
@@ -18,8 +18,7 @@ NavigationState::NavigationState(content::PageTransition transition_type,
       was_within_same_page_(false),
       transferred_request_child_id_(-1),
       transferred_request_request_id_(-1),
-      allow_download_(true),
-      is_redirect_in_progress_(false) {
+      allow_download_(true) {
 }
 
 NavigationState::~NavigationState() {}
diff --git a/content/public/renderer/navigation_state.h b/content/public/renderer/navigation_state.h
index 58a6534..f13b9b7 100644
--- a/content/public/renderer/navigation_state.h
+++ b/content/public/renderer/navigation_state.h
@@ -78,12 +78,6 @@ class NavigationState {
   bool allow_download() const {
     return allow_download_;
   }
-  void set_is_redirect_in_progress(bool value) {
-    is_redirect_in_progress_ = value;
-  }
-  bool is_redirect_in_progress() const {
-    return is_redirect_in_progress_;
-  }
 
  private:
   NavigationState(content::PageTransition transition_type,
@@ -102,13 +96,6 @@ class NavigationState {
   int transferred_request_request_id_;
   bool allow_download_;
 
-  // If we are handling a top-level client-side redirect, we need to set this
-  // to true when calling willPerformClientRedirect.
-  // If DecidePolicyForNavigation decides this redirect is a cross-process
-  // navigation, it will pass this flag to the browser process through OpenURL,
-  // which will eventually replace the current navigation entry.
-  bool is_redirect_in_progress_;
-
   DISALLOW_COPY_AND_ASSIGN(NavigationState);
 };
 
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index 21b30de..2042433 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -1697,10 +1697,12 @@ void RenderViewImpl::OpenURL(WebFrame* frame,
   params.referrer = referrer;
   params.disposition = NavigationPolicyToDisposition(policy);
   params.frame_id = frame->identifier();
-  DocumentState* document_state =
-      DocumentState::FromDataSource(frame->dataSource());
-  params.is_cross_site_redirect =
-      document_state->navigation_state()->is_redirect_in_progress();
+  WebDataSource* ds = frame->provisionalDataSource();
+  if (ds) {
+    params.is_cross_site_redirect = ds->isClientRedirect();
+  } else {
+    params.is_cross_site_redirect = false;
+  }
 
   Send(new ViewHostMsg_OpenURL(routing_id_, params));
 }
@@ -3019,9 +3021,6 @@ void RenderViewImpl::willPerformClientRedirect(
     double fire_time) {
   // Replace any occurrences of swappedout:// with about:blank.
   const WebURL& blank_url = GURL(chrome::kAboutBlankURL);
-  DocumentState* document_state =
-      DocumentState::FromDataSource(frame->dataSource());
-  document_state->navigation_state()->set_is_redirect_in_progress(true);
 
   FOR_EACH_OBSERVER(
       RenderViewObserver, observers_,
@@ -3031,10 +3030,6 @@ void RenderViewImpl::willPerformClientRedirect(
 }
 
 void RenderViewImpl::didCancelClientRedirect(WebFrame* frame) {
-  DocumentState* document_state =
-      DocumentState::FromDataSource(frame->dataSource());
-  document_state->navigation_state()->set_is_redirect_in_progress(false);
-
   FOR_EACH_OBSERVER(
       RenderViewObserver, observers_, DidCancelClientRedirect(frame));
 }
@@ -3464,7 +3459,6 @@ void RenderViewImpl::didCommitProvisionalLoad(WebFrame* frame,
   // If this committed load was initiated by a client redirect, we're
   // at the last stop now, so clear it.
   completed_client_redirect_src_ = Referrer();
-  navigation_state->set_is_redirect_in_progress(false);
 
   // Check whether we have new encoding name.
   UpdateEncoding(frame, frame->view()->pageEncoding().utf8());