Don't delete the current NavigationEntry when leaving an interstitial page.

BUG=107182
TEST=See bug

Review URL: http://codereview.chromium.org/8976014

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

3 files changed, 52 insertions, 15 deletions

diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
index 05dfb39..feca329 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
@@ -457,8 +457,24 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
   }
 
   if (command == kTakeMeBackCommand) {
-    DontProceed();
-    // We are deleted after this.
+    if (is_main_frame_load_blocked_) {
+      // If the load is blocked, we want to close the interstitial and discard
+      // the pending entry.
+      DontProceed();
+      // We are deleted after this.
+      return;
+    }
+
+    // Otherwise the offending entry has committed, and we need to go back or
+    // to a safe page.  We will close the interstitial when that page commits.
+    if (tab()->controller().CanGoBack()) {
+      tab()->controller().GoBack();
+    } else {
+      tab()->controller().LoadURL(GURL(chrome::kChromeUINewTabURL),
+                                  content::Referrer(),
+                                  content::PAGE_TRANSITION_START_PAGE,
+                                  std::string());
+    }
     return;
   }
 
@@ -580,10 +596,14 @@ void SafeBrowsingBlockingPage::DontProceed() {
 
   // We don't remove the navigation entry if the tab is being destroyed as this
   // would trigger a navigation that would cause trouble as the render view host
-  // for the tab has by then already been destroyed.
-  if (navigation_entry_index_to_remove_ != -1 && !tab()->is_being_destroyed()) {
-    tab()->controller().RemoveEntryAtIndex(navigation_entry_index_to_remove_,
-                                           GURL(chrome::kChromeUINewTabURL));
+  // for the tab has by then already been destroyed.  We also don't delete the
+  // current entry if it has been committed again, which is possible on a page
+  // that had a subresource warning.
+  int last_committed_index = tab()->controller().last_committed_entry_index();
+  if (navigation_entry_index_to_remove_ != -1 &&
+      navigation_entry_index_to_remove_ != last_committed_index &&
+      !tab()->is_being_destroyed()) {
+    tab()->controller().RemoveEntryAtIndex(navigation_entry_index_to_remove_);
     navigation_entry_index_to_remove_ = -1;
   }
   InterstitialPage::DontProceed();
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
index 51a5ae6..b7e54d3 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
@@ -507,7 +507,13 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingBlockingPageTest, MalwareIframeDontProceed) {
 
   ui_test_utils::NavigateToURL(browser(), url);
 
+  ui_test_utils::WindowedNotificationObserver observer(
+      content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+      content::Source<NavigationController>(
+          &browser()->GetSelectedTabContentsWrapper()->tab_contents()->
+              controller()));
   SendCommand("\"takeMeBack\"");    // Simulate the user clicking "back"
+  observer.Wait();
   AssertNoInterstitial(false);  // Assert the interstitial is gone
 
   EXPECT_EQ(GURL(chrome::kAboutBlankURL),    // Back to "about:blank"
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
index 1bf2e80..37b1c2c 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
@@ -114,15 +114,17 @@ class SafeBrowsingBlockingPageTest : public ChromeRenderViewHostTestHarness,
         content::PAGE_TRANSITION_TYPED);
   }
 
-  void GoBackCrossSite() {
+  void GoBack(bool is_cross_site) {
     NavigationEntry* entry = contents()->controller().GetEntryAtOffset(-1);
     ASSERT_TRUE(entry);
     contents()->controller().GoBack();
 
-    // The navigation should commit in the pending RVH.
-    contents()->TestDidNavigate(
-        contents()->pending_rvh(), entry->page_id(), GURL(entry->url()),
-        content::PAGE_TRANSITION_TYPED);
+    // The pending RVH should commit for cross-site navigations.
+    RenderViewHost* rvh = is_cross_site ?
+        contents()->pending_rvh() :
+        contents()->render_view_host();
+    contents()->TestDidNavigate(rvh, entry->page_id(), GURL(entry->url()),
+                                content::PAGE_TRANSITION_TYPED);
   }
 
   void ShowInterstitial(bool is_subresource, const char* url) {
@@ -158,6 +160,15 @@ class SafeBrowsingBlockingPageTest : public ChromeRenderViewHostTestHarness,
     MessageLoop::current()->RunAllPending();
   }
 
+  void DontProceedThroughSubresourceInterstitial(
+      SafeBrowsingBlockingPage* sb_interstitial) {
+    // CommandReceived(kTakeMeBackCommand) does a back navigation for
+    // subresource interstitials.
+    GoBack(false);
+    // DontProceed() posts a task to update the SafeBrowsingService::Client.
+    MessageLoop::current()->RunAllPending();
+  }
+
   scoped_refptr<TestSafeBrowsingService> service_;
 
  private:
@@ -262,7 +273,7 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceDontProceed) {
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "don't proceed".
-  DontProceedThroughInterstitial(sb_interstitial);
+  DontProceedThroughSubresourceInterstitial(sb_interstitial);
   EXPECT_EQ(CANCEL, user_response());
   EXPECT_FALSE(GetSafeBrowsingBlockingPage());
 
@@ -333,7 +344,7 @@ TEST_F(SafeBrowsingBlockingPageTest,
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "don't proceed".
-  DontProceedThroughInterstitial(sb_interstitial);
+  DontProceedThroughSubresourceInterstitial(sb_interstitial);
   EXPECT_EQ(CANCEL, user_response());
   EXPECT_FALSE(GetSafeBrowsingBlockingPage());
 
@@ -388,7 +399,7 @@ TEST_F(SafeBrowsingBlockingPageTest,
   ASSERT_TRUE(sb_interstitial);
 
   // Don't proceed through the 2nd interstitial.
-  DontProceedThroughInterstitial(sb_interstitial);
+  DontProceedThroughSubresourceInterstitial(sb_interstitial);
   EXPECT_EQ(CANCEL, user_response());
   EXPECT_FALSE(GetSafeBrowsingBlockingPage());
 
@@ -473,7 +484,7 @@ TEST_F(SafeBrowsingBlockingPageTest, NavigatingBackAndForth) {
   // Proceed, then navigate back.
   ProceedThroughInterstitial(sb_interstitial);
   Navigate(kBadURL, 2);  // Commit the navigation.
-  GoBackCrossSite();
+  GoBack(true);
 
   // We are back on the good page.
   sb_interstitial = GetSafeBrowsingBlockingPage();