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/245943002

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

11 files changed, 134 insertions, 46 deletions

diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index af19192..715c895 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -429,17 +429,12 @@ 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() ||
-       (IsInitialNavigation() &&
-        !GetLastCommittedEntry() &&
-        !rvh->has_accessed_initial_document()));
+      (!pending_entry_->is_renderer_initiated() || IsUnmodifiedBlankTab());
 
   // 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
@@ -1408,6 +1403,16 @@ 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 c0465f8..7ec82f3 100644
--- a/content/browser/frame_host/navigation_controller_impl.h
+++ b/content/browser/frame_host/navigation_controller_impl.h
@@ -94,6 +94,10 @@ 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 7ffd0fd..16850fd 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(0, delegate->navigation_state_change_count());
+  EXPECT_EQ(1, 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,6 +2933,57 @@ 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 a284c0f..4685c7a 100644
--- a/content/browser/frame_host/navigator_delegate.cc
+++ b/content/browser/frame_host/navigator_delegate.cc
@@ -10,4 +10,8 @@ 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 e92dbee..369ee8f 100644
--- a/content/browser/frame_host/navigator_delegate.h
+++ b/content/browser/frame_host/navigator_delegate.h
@@ -99,6 +99,10 @@ 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 7be5f8c..603bb28 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -230,18 +230,31 @@ void NavigatorImpl::DidFailProvisionalLoadWithError(
     // TODO(creis): Find a way to cancel any pending RFH here.
   }
 
-  // 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
+  // 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
   // DidStartProvisionalLoadForFrame.
-  // 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).
+  //
+  // 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).
+  //
   // Note: don't touch the transient entry, since an interstitial may exist.
-  if (controller_->GetPendingEntry() != controller_->GetVisibleEntry())
+  bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
+      delegate_->ShouldPreserveAbortedURLs();
+  if (controller_->GetPendingEntry() != controller_->GetVisibleEntry() ||
+      !should_preserve_entry) {
     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 cb1901f..2e83028 100644
--- a/content/browser/frame_host/render_frame_host_manager_browsertest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_browsertest.cc
@@ -844,45 +844,33 @@ 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.  The typed URL will
-  // still be visible until the user clears it manually, but the last
-  // committed URL will be the previous page.
+  // 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.
   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_EQ("/files/click-noreferrer-links.html",
-            shell()->web_contents()->GetController().
-                GetLastCommittedEntry()->GetVirtualURL().path());
+  EXPECT_FALSE(
+      shell()->web_contents()->GetController().GetLastCommittedEntry());
 
   // Renderer-initiated navigations should work.
-  bool success = false;
-  EXPECT_TRUE(ExecuteScriptAndExtractBool(
+  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(
       shell()->web_contents(),
-      "window.domAutomationController.send(clickNoRefLink());",
-      &success));
-  EXPECT_TRUE(success);
-
-  // Wait for the cross-site transition in the current tab to finish.
-  WaitForLoadStop(shell()->web_contents());
+      base::StringPrintf("location.href = '%s'", url.spec().c_str())));
+  ASSERT_EQ(expected_title, title_watcher.WaitAndGetTitle());
 
   // Opens in same tab.
   EXPECT_EQ(1u, Shell::windows().size());
@@ -890,9 +878,9 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, ClickLinkAfter204Error) {
             shell()->web_contents()->GetLastCommittedURL().path());
 
   // Should have the same SiteInstance.
-  scoped_refptr<SiteInstance> noref_site_instance(
+  scoped_refptr<SiteInstance> new_site_instance(
       shell()->web_contents()->GetSiteInstance());
-  EXPECT_EQ(orig_site_instance, noref_site_instance);
+  EXPECT_EQ(orig_site_instance, new_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 2d05483..566e7f7 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1263,10 +1263,12 @@ bool WebContentsImpl::HandleGestureEvent(
     totalPinchGestureAmount_ += event.data.pinchUpdate.scale;
     if (totalPinchGestureAmount_ > zoomInThreshold) {
       currentPinchZoomStepDelta_++;
-      delegate_->ContentsZoomChange(true);
+      if (delegate_)
+        delegate_->ContentsZoomChange(true);
     } else if (totalPinchGestureAmount_ < zoomOutThreshold) {
       currentPinchZoomStepDelta_--;
-      delegate_->ContentsZoomChange(false);
+      if (delegate_)
+        delegate_->ContentsZoomChange(false);
     }
     return true;
   }
@@ -2372,6 +2374,12 @@ void WebContentsImpl::RequestOpenURL(RenderFrameHostImpl* render_frame_host,
   }
 }
 
+bool WebContentsImpl::ShouldPreserveAbortedURLs() {
+  if (!delegate_)
+    return false;
+  return delegate_->ShouldPreserveAbortedURLs(this);
+}
+
 void WebContentsImpl::DidRedirectProvisionalLoad(
     RenderFrameHostImpl* render_frame_host,
     const GURL& validated_target_url) {
@@ -2685,8 +2693,9 @@ void WebContentsImpl::OnOpenColorChooser(
     int color_chooser_id,
     SkColor color,
     const std::vector<ColorSuggestion>& suggestions) {
-  ColorChooser* new_color_chooser =
-      delegate_->OpenColorChooser(this, color, suggestions);
+  ColorChooser* new_color_chooser = delegate_ ?
+      delegate_->OpenColorChooser(this, color, suggestions) :
+      NULL;
   if (!new_color_chooser)
     return;
   if (color_chooser_info_.get())
@@ -3580,7 +3589,7 @@ void WebContentsImpl::RendererUnresponsive(RenderViewHost* rvh,
     // close. Otherwise, pretend the unload listeners have all fired and close
     // the tab.
     bool close = true;
-    if (is_during_beforeunload) {
+    if (is_during_beforeunload && delegate_) {
       delegate_->BeforeUnloadFired(this, true, &close);
     }
     if (close)
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index cc4f435..d00938e4 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -490,6 +490,7 @@ 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 22430da..5346c2a 100644
--- a/content/public/browser/web_contents_delegate.cc
+++ b/content/public/browser/web_contents_delegate.cc
@@ -37,6 +37,10 @@ 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 0210314..d22d7c7 100644
--- a/content/public/browser/web_contents_delegate.h
+++ b/content/public/browser/web_contents_delegate.h
@@ -173,6 +173,11 @@ 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.