Revert 265044 "Don't leave aborted URLs in the omnibox unless we..."

This appears to be breaking prerender tests on some Mac and Linux builders.

e.g.

Mac:
http://build.chromium.org/p/chromium.mac/builders/Mac%2010.7%20Tests%20%28dbg%29%284%29/builds/22698/steps/browser_tests/logs/PrerenderDoublePendingSwap
http://build.chromium.org/p/chromium.mac/builders/Mac%2010.6%20Tests%20%28dbg%29%284%29/builds/40342/steps/browser_tests/logs/PrerenderPageNewTab

Linux ASan:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%28sandboxed%29/builds/971/steps/browser_tests/logs/PrerenderPageNewTab

> Don't leave aborted URLs in the omnibox unless we're on the NTP.
> 
> BUG=355537
> TEST=See bug for repro steps.
> 
> Review URL: https://codereview.chromium.org/232463007

TBR=creis@chromium.org

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

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

14 files changed, 41 insertions, 180 deletions

diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 36462c0..2188455 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -1132,16 +1132,6 @@ bool Browser::CanOverscrollContent() const {
 #endif
 }
 
-bool Browser::ShouldPreserveAbortedURLs(WebContents* source) {
-  // Allow failed URLs to stick around in the omnibox on the NTP, but not when
-  // other pages have committed.
-  Profile* profile = Profile::FromBrowserContext(source->GetBrowserContext());
-  if (!profile || !source->GetController().GetLastCommittedEntry())
-    return false;
-  GURL committed_url(source->GetController().GetLastCommittedEntry()->GetURL());
-  return chrome::IsNTPURL(committed_url, profile);
-}
-
 bool Browser::PreHandleKeyboardEvent(content::WebContents* source,
                                      const NativeWebKeyboardEvent& event,
                                      bool* is_keyboard_shortcut) {
diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
index 66c079e..2d7172e 100644
--- a/chrome/browser/ui/browser.h
+++ b/chrome/browser/ui/browser.h
@@ -428,7 +428,6 @@ class Browser : public TabStripModelObserver,
 
   // Overridden from content::WebContentsDelegate:
   virtual bool CanOverscrollContent() const OVERRIDE;
-  virtual bool ShouldPreserveAbortedURLs(content::WebContents* source) OVERRIDE;
   virtual bool PreHandleKeyboardEvent(
       content::WebContents* source,
       const content::NativeWebKeyboardEvent& event,
diff --git a/chrome/browser/ui/browser_browsertest.cc b/chrome/browser/ui/browser_browsertest.cc
index b333ac7..930e4e3 100644
--- a/chrome/browser/ui/browser_browsertest.cc
+++ b/chrome/browser/ui/browser_browsertest.cc
@@ -25,7 +25,6 @@
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/search/search.h"
 #include "chrome/browser/sessions/session_backend.h"
 #include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/translate/translate_tab_helper.h"
@@ -470,50 +469,6 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, MAYBE_ThirtyFourTabs) {
   }
 }
 
-// Test that a browser-initiated navigation to an aborted URL load leaves around
-// a pending entry if we start from the NTP but not from a normal page.
-// See http://crbug.com/355537.
-IN_PROC_BROWSER_TEST_F(BrowserTest, ClearPendingOnFailUnlessNTP) {
-  ASSERT_TRUE(test_server()->Start());
-  WebContents* web_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  GURL ntp_url(chrome::GetNewTabPageURL(browser()->profile()));
-  ui_test_utils::NavigateToURL(browser(), ntp_url);
-
-  // Navigate to a 204 URL (aborts with no content) on the NTP and make sure it
-  // sticks around so that the user can edit it.
-  GURL abort_url(test_server()->GetURL("nocontent"));
-  {
-    content::WindowedNotificationObserver stop_observer(
-        content::NOTIFICATION_LOAD_STOP,
-        content::Source<NavigationController>(
-            &web_contents->GetController()));
-    browser()->OpenURL(OpenURLParams(abort_url, Referrer(), CURRENT_TAB,
-                                     content::PAGE_TRANSITION_TYPED, false));
-    stop_observer.Wait();
-    EXPECT_TRUE(web_contents->GetController().GetPendingEntry());
-    EXPECT_EQ(abort_url, web_contents->GetVisibleURL());
-  }
-
-  // Navigate to a real URL.
-  GURL real_url(test_server()->GetURL("title1.html"));
-  ui_test_utils::NavigateToURL(browser(), real_url);
-  EXPECT_EQ(real_url, web_contents->GetVisibleURL());
-
-  // Now navigating to a 204 URL should clear the pending entry.
-  {
-    content::WindowedNotificationObserver stop_observer(
-        content::NOTIFICATION_LOAD_STOP,
-        content::Source<NavigationController>(
-            &web_contents->GetController()));
-    browser()->OpenURL(OpenURLParams(abort_url, Referrer(), CURRENT_TAB,
-                                     content::PAGE_TRANSITION_TYPED, false));
-    stop_observer.Wait();
-    EXPECT_FALSE(web_contents->GetController().GetPendingEntry());
-    EXPECT_EQ(real_url, web_contents->GetVisibleURL());
-  }
-}
-
 // Test for crbug.com/297289.  Ensure that modal dialogs are closed when a
 // cross-process navigation is ready to commit.
 IN_PROC_BROWSER_TEST_F(BrowserTest, CrossProcessNavCancelsDialogs) {
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 715c895..af19192 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -429,12 +429,17 @@ NavigationEntry* NavigationControllerImpl::GetVisibleEntry() const {
   // long as no other page has tried to access the initial empty document in
   // the new tab.  If another page modifies this blank page, a URL spoof is
   // possible, so we must stop showing the pending entry.
+  RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
+      delegate_->GetRenderViewHost());
   bool safe_to_show_pending =
       pending_entry_ &&
       // Require a new navigation.
       pending_entry_->GetPageID() == -1 &&
       // Require either browser-initiated or an unmodified new tab.
-      (!pending_entry_->is_renderer_initiated() || IsUnmodifiedBlankTab());
+      (!pending_entry_->is_renderer_initiated() ||
+       (IsInitialNavigation() &&
+        !GetLastCommittedEntry() &&
+        !rvh->has_accessed_initial_document()));
 
   // Also allow showing the pending entry for history navigations in a new tab,
   // such as Ctrl+Back.  In this case, no existing page is visible and no one
@@ -1403,16 +1408,6 @@ int32 NavigationControllerImpl::GetMaxRestoredPageID() const {
   return max_restored_page_id_;
 }
 
-bool NavigationControllerImpl::IsUnmodifiedBlankTab() const {
-  // TODO(creis): Move has_accessed_initial_document from RenderViewHost to
-  // WebContents and NavigationControllerDelegate.
-  RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
-      delegate_->GetRenderViewHost());
-  return IsInitialNavigation() &&
-      !GetLastCommittedEntry() &&
-      !rvh->has_accessed_initial_document();
-}
-
 SessionStorageNamespace*
 NavigationControllerImpl::GetSessionStorageNamespace(SiteInstance* instance) {
   std::string partition_id;
diff --git a/content/browser/frame_host/navigation_controller_impl.h b/content/browser/frame_host/navigation_controller_impl.h
index 7ec82f3..c0465f8 100644
--- a/content/browser/frame_host/navigation_controller_impl.h
+++ b/content/browser/frame_host/navigation_controller_impl.h
@@ -94,10 +94,6 @@ class CONTENT_EXPORT NavigationControllerImpl
   virtual void PruneAllButLastCommitted() OVERRIDE;
   virtual void ClearAllScreenshots() OVERRIDE;
 
-  // Whether this is the initial navigation in an unmodified new tab.  In this
-  // case, we know there is no content displayed in the page.
-  bool IsUnmodifiedBlankTab() const;
-
   // The session storage namespace that all child RenderViews belonging to
   // |instance| should use.
   SessionStorageNamespace* GetSessionStorageNamespace(
diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc
index 16850fd..7ffd0fd 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -1046,7 +1046,7 @@ TEST_F(NavigationControllerTest, LoadURL_RedirectAbortDoesntShowPendingURL) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_FALSE(controller.GetPendingEntry());
   EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(1, delegate->navigation_state_change_count());
+  EXPECT_EQ(0, delegate->navigation_state_change_count());
 
   // The visible entry should be the last committed URL, not the pending one,
   // so that no spoof is possible.
@@ -2933,57 +2933,6 @@ TEST_F(NavigationControllerTest, ShowBrowserURLAfterFailUntilModified) {
   notifications.Reset();
 }
 
-// Tests that the URLs for renderer-initiated navigations in new tabs are
-// displayed to the user even after they fail, as long as the initial
-// about:blank page has not been modified.  If so, we must revert to showing
-// about:blank. See http://crbug.com/355537.
-TEST_F(NavigationControllerTest, ShowRendererURLAfterFailUntilModified) {
-  NavigationControllerImpl& controller = controller_impl();
-  TestNotificationTracker notifications;
-  RegisterForAllNavNotifications(&notifications, &controller);
-
-  const GURL url("http://foo");
-
-  // For renderer-initiated navigations in new tabs (with no committed entries),
-  // we show the pending entry's URL as long as the about:blank page is not
-  // modified.
-  NavigationController::LoadURLParams load_url_params(url);
-  load_url_params.transition_type = PAGE_TRANSITION_LINK;
-  load_url_params.is_renderer_initiated = true;
-  controller.LoadURLWithParams(load_url_params);
-  EXPECT_EQ(url, controller.GetVisibleEntry()->GetURL());
-  EXPECT_EQ(url, controller.GetPendingEntry()->GetURL());
-  EXPECT_TRUE(
-      NavigationEntryImpl::FromNavigationEntry(controller.GetPendingEntry())->
-          is_renderer_initiated());
-  EXPECT_TRUE(controller.IsInitialNavigation());
-  EXPECT_FALSE(test_rvh()->has_accessed_initial_document());
-
-  // There should be no title yet.
-  EXPECT_TRUE(contents()->GetTitle().empty());
-
-  // Suppose it aborts before committing, if it's a 204 or download or due to a
-  // stop or a new navigation from the user.  The URL should remain visible.
-  FrameHostMsg_DidFailProvisionalLoadWithError_Params params;
-  params.error_code = net::ERR_ABORTED;
-  params.error_description = base::string16();
-  params.url = url;
-  params.showing_repost_interstitial = false;
-  main_test_rfh()->OnMessageReceived(
-      FrameHostMsg_DidFailProvisionalLoadWithError(0, params));
-  EXPECT_EQ(url, controller.GetVisibleEntry()->GetURL());
-
-  // If something else later modifies the contents of the about:blank page, then
-  // we must revert to showing about:blank to avoid a URL spoof.
-  test_rvh()->OnMessageReceived(
-        ViewHostMsg_DidAccessInitialDocument(0));
-  EXPECT_TRUE(test_rvh()->has_accessed_initial_document());
-  EXPECT_FALSE(controller.GetVisibleEntry());
-  EXPECT_EQ(url, controller.GetPendingEntry()->GetURL());
-
-  notifications.Reset();
-}
-
 TEST_F(NavigationControllerTest, DontShowRendererURLInNewTabAfterCommit) {
   NavigationControllerImpl& controller = controller_impl();
   TestNotificationTracker notifications;
diff --git a/content/browser/frame_host/navigator_delegate.cc b/content/browser/frame_host/navigator_delegate.cc
index 4685c7a..a284c0f 100644
--- a/content/browser/frame_host/navigator_delegate.cc
+++ b/content/browser/frame_host/navigator_delegate.cc
@@ -10,8 +10,4 @@ bool NavigatorDelegate::CanOverscrollContent() {
   return false;
 }
 
-bool NavigatorDelegate::ShouldPreserveAbortedURLs() {
-  return false;
-}
-
 }  // namespace content
diff --git a/content/browser/frame_host/navigator_delegate.h b/content/browser/frame_host/navigator_delegate.h
index 369ee8f..e92dbee 100644
--- a/content/browser/frame_host/navigator_delegate.h
+++ b/content/browser/frame_host/navigator_delegate.h
@@ -99,10 +99,6 @@ class CONTENT_EXPORT NavigatorDelegate {
   // this forwards to.
   virtual void RequestOpenURL(RenderFrameHostImpl* render_frame_host,
                               const OpenURLParams& params) {}
-
-  // Returns whether URLs for aborted browser-initiated navigations should be
-  // preserved in the omnibox.  Defaults to false.
-  virtual bool ShouldPreserveAbortedURLs();
 };
 
 }  // namspace content
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 603bb28..7be5f8c 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -230,31 +230,18 @@ void NavigatorImpl::DidFailProvisionalLoadWithError(
     // TODO(creis): Find a way to cancel any pending RFH here.
   }
 
-  // We usually clear the pending entry when it fails, so that an arbitrary URL
-  // isn't left visible above a committed page.  This must be enforced when
-  // the pending entry isn't visible (e.g., renderer-initiated navigations) to
-  // prevent URL spoofs for in-page navigations that don't go through
+  // Do not usually clear the pending entry if one exists, so that the user's
+  // typed URL is not lost when a navigation fails or is aborted.  However, in
+  // cases that we don't show the pending entry (e.g., renderer-initiated
+  // navigations in an existing tab), we don't keep it around.  That prevents
+  // spoofs on in-page navigations that don't go through
   // DidStartProvisionalLoadForFrame.
-  //
-  // However, we do preserve the pending entry in some cases, such as on the
-  // initial navigation of an unmodified blank tab.  We also allow the delegate
-  // to say when it's safe to leave aborted URLs in the omnibox, to let the user
-  // edit the URL and try again.  This may be useful in cases that the committed
-  // page cannot be attacker-controlled.  In these cases, we still allow the
-  // view to clear the pending entry and typed URL if the user requests
-  // (e.g., hitting Escape with focus in the address bar).
-  //
+  // In general, we allow the view to clear the pending entry and typed URL if
+  // the user requests (e.g., hitting Escape with focus in the address bar).
   // Note: don't touch the transient entry, since an interstitial may exist.
-  bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
-      delegate_->ShouldPreserveAbortedURLs();
-  if (controller_->GetPendingEntry() != controller_->GetVisibleEntry() ||
-      !should_preserve_entry) {
+  if (controller_->GetPendingEntry() != controller_->GetVisibleEntry())
     controller_->DiscardPendingEntry();
 
-    // Also force the UI to refresh.
-    controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
-  }
-
   if (delegate_)
     delegate_->DidFailProvisionalLoadWithError(render_frame_host, params);
 }
diff --git a/content/browser/frame_host/render_frame_host_manager_browsertest.cc b/content/browser/frame_host/render_frame_host_manager_browsertest.cc
index 2e83028..cb1901f 100644
--- a/content/browser/frame_host/render_frame_host_manager_browsertest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_browsertest.cc
@@ -844,33 +844,45 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
 IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, ClickLinkAfter204Error) {
   StartServer();
 
+  // Load a page with links that open in a new window.
+  // The links will point to foo.com.
+  std::string replacement_path;
+  ASSERT_TRUE(GetFilePathWithHostAndPortReplacement(
+      "files/click-noreferrer-links.html",
+      foo_host_port_,
+      &replacement_path));
+  NavigateToURL(shell(), test_server()->GetURL(replacement_path));
+
   // Get the original SiteInstance for later comparison.
   scoped_refptr<SiteInstance> orig_site_instance(
       shell()->web_contents()->GetSiteInstance());
   EXPECT_TRUE(orig_site_instance.get() != NULL);
 
   // Load a cross-site page that fails with a 204 error.
-  NavigateToURL(shell(), GetCrossSiteURL("nocontent"));
+  NavigateToURL(shell(),GetCrossSiteURL("nocontent"));
 
-  // We should still be looking at the normal page.  Because we started from a
-  // blank new tab, the typed URL will still be visible until the user clears it
-  // manually.  The last committed URL will be the previous page.
+  // We should still be looking at the normal page.  The typed URL will
+  // still be visible until the user clears it manually, but the last
+  // committed URL will be the previous page.
   scoped_refptr<SiteInstance> post_nav_site_instance(
       shell()->web_contents()->GetSiteInstance());
   EXPECT_EQ(orig_site_instance, post_nav_site_instance);
   EXPECT_EQ("/nocontent",
             shell()->web_contents()->GetVisibleURL().path());
-  EXPECT_FALSE(
-      shell()->web_contents()->GetController().GetLastCommittedEntry());
+  EXPECT_EQ("/files/click-noreferrer-links.html",
+            shell()->web_contents()->GetController().
+                GetLastCommittedEntry()->GetVirtualURL().path());
 
   // Renderer-initiated navigations should work.
-  base::string16 expected_title = ASCIIToUTF16("Title Of Awesomeness");
-  TitleWatcher title_watcher(shell()->web_contents(), expected_title);
-  GURL url = test_server()->GetURL("files/title2.html");
-  EXPECT_TRUE(ExecuteScript(
+  bool success = false;
+  EXPECT_TRUE(ExecuteScriptAndExtractBool(
       shell()->web_contents(),
-      base::StringPrintf("location.href = '%s'", url.spec().c_str())));
-  ASSERT_EQ(expected_title, title_watcher.WaitAndGetTitle());
+      "window.domAutomationController.send(clickNoRefLink());",
+      &success));
+  EXPECT_TRUE(success);
+
+  // Wait for the cross-site transition in the current tab to finish.
+  WaitForLoadStop(shell()->web_contents());
 
   // Opens in same tab.
   EXPECT_EQ(1u, Shell::windows().size());
@@ -878,9 +890,9 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, ClickLinkAfter204Error) {
             shell()->web_contents()->GetLastCommittedURL().path());
 
   // Should have the same SiteInstance.
-  scoped_refptr<SiteInstance> new_site_instance(
+  scoped_refptr<SiteInstance> noref_site_instance(
       shell()->web_contents()->GetSiteInstance());
-  EXPECT_EQ(orig_site_instance, new_site_instance);
+  EXPECT_EQ(orig_site_instance, noref_site_instance);
 }
 
 // Test for crbug.com/9682.  We should show the URL for a pending renderer-
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 73ea8d3..2d05483 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -2372,10 +2372,6 @@ void WebContentsImpl::RequestOpenURL(RenderFrameHostImpl* render_frame_host,
   }
 }
 
-bool WebContentsImpl::ShouldPreserveAbortedURLs() {
-  return delegate_->ShouldPreserveAbortedURLs(this);
-}
-
 void WebContentsImpl::DidRedirectProvisionalLoad(
     RenderFrameHostImpl* render_frame_host,
     const GURL& validated_target_url) {
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index d00938e4..cc4f435 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -490,7 +490,6 @@ class CONTENT_EXPORT WebContentsImpl
       NavigationController::ReloadType reload_type) OVERRIDE;
   virtual void RequestOpenURL(RenderFrameHostImpl* render_frame_host,
                               const OpenURLParams& params) OVERRIDE;
-  virtual bool ShouldPreserveAbortedURLs() OVERRIDE;
 
   // RenderWidgetHostDelegate --------------------------------------------------
 
diff --git a/content/public/browser/web_contents_delegate.cc b/content/public/browser/web_contents_delegate.cc
index 5346c2a..22430da 100644
--- a/content/public/browser/web_contents_delegate.cc
+++ b/content/public/browser/web_contents_delegate.cc
@@ -37,10 +37,6 @@ bool WebContentsDelegate::ShouldSuppressDialogs() {
   return false;
 }
 
-bool WebContentsDelegate::ShouldPreserveAbortedURLs(WebContents* source) {
-  return false;
-}
-
 bool WebContentsDelegate::AddMessageToConsole(WebContents* source,
                                               int32 level,
                                               const base::string16& message,
diff --git a/content/public/browser/web_contents_delegate.h b/content/public/browser/web_contents_delegate.h
index d22d7c7..0210314 100644
--- a/content/public/browser/web_contents_delegate.h
+++ b/content/public/browser/web_contents_delegate.h
@@ -173,11 +173,6 @@ class CONTENT_EXPORT WebContentsDelegate {
   // Default is false.
   virtual bool ShouldSuppressDialogs();
 
-  // Returns whether pending NavigationEntries for aborted browser-initiated
-  // navigations should be preserved (and thus returned from GetVisibleURL).
-  // Defaults to false.
-  virtual bool ShouldPreserveAbortedURLs(WebContents* source);
-
   // Add a message to the console. Returning true indicates that the delegate
   // handled the message. If false is returned the default logging mechanism
   // will be used for the message.