Classify navigations without page id in parallel to the existing classifier.

chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <list>
 
 #include "base/prefs/pref_service.h"
 #include "base/run_loop.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/malware_details.h"
@@ skipping 102 matching lines @@
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
   void OnBlockingPageComplete(bool proceed) {
     if (proceed)
       user_response_ = OK;
     else
       user_response_ = CANCEL;
   }
 
-  void Navigate(const char* url, int page_id) {
+  void Navigate(const char* url,
+                int page_id,
+                int nav_entry_id,
+                bool did_create_new_entry) {
+    NavigateCrossSite(url, page_id, nav_entry_id, did_create_new_entry, false);
+  }
+
+  void NavigateCrossSite(const char* url,
+                         int page_id,
+                         int nav_entry_id,
+                         bool did_create_new_entry,
+                         bool is_cross_site) {
+    // The pending RVH should commit for cross-site navigations.
+    content::RenderFrameHost* rfh = is_cross_site ?
+        WebContentsTester::For(web_contents())->GetPendingMainFrame() :
+        web_contents()->GetMainFrame();
+
     WebContentsTester::For(web_contents())->TestDidNavigate(
-        web_contents()->GetMainFrame(), page_id, GURL(url),
+        rfh,
+        page_id,
+        nav_entry_id,
+        did_create_new_entry,
+        GURL(url),
         ui::PAGE_TRANSITION_TYPED);
   }
 
   void GoBack(bool is_cross_site) {
     NavigationEntry* entry =
         web_contents()->GetController().GetEntryAtOffset(-1);
     ASSERT_TRUE(entry);
     web_contents()->GetController().GoBack();
 
     // The pending RVH should commit for cross-site navigations.
     content::RenderFrameHost* rfh = is_cross_site ?
         WebContentsTester::For(web_contents())->GetPendingMainFrame() :
         web_contents()->GetMainFrame();
     WebContentsTester::For(web_contents())->TestDidNavigate(
         rfh,
         entry->GetPageID(),
-        GURL(entry->GetURL()),
+        entry->GetUniqueID(),
+        false,
+        entry->GetURL(),
         ui::PAGE_TRANSITION_TYPED);
   }
 
   void ShowInterstitial(bool is_subresource, const char* url) {
     SafeBrowsingUIManager::UnsafeResource resource;
     InitResource(&resource, is_subresource, GURL(url));
     SafeBrowsingBlockingPage::ShowBlockingPage(ui_manager_.get(), resource);
   }
 
   // Returns the SafeBrowsingBlockingPage currently showing or NULL if none is
@@ skipping 64 matching lines @@
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Start a load.
   controller().LoadURL(GURL(kBadURL), content::Referrer(),
                        ui::PAGE_TRANSITION_TYPED, std::string());
 
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   base::RunLoop().RunUntilIdle();
 
   // Simulate the user clicking "don't proceed".
   DontProceedThroughInterstitial(sb_interstitial);
 
   // The interstitial should be gone.
   EXPECT_EQ(CANCEL, user_response());
   EXPECT_FALSE(GetSafeBrowsingBlockingPage());
@@ skipping 10 matching lines @@
 TEST_F(SafeBrowsingBlockingPageTest, MalwarePageProceed) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Start a load.
   controller().LoadURL(GURL(kBadURL), content::Referrer(),
                        ui::PAGE_TRANSITION_TYPED, std::string());
+  int pending_id = controller().GetPendingEntry()->GetUniqueID();
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "proceed".
   ProceedThroughInterstitial(sb_interstitial);
 
   // The interstitial is shown until the navigation commits.
   ASSERT_TRUE(InterstitialPage::GetInterstitialPage(web_contents()));
   // Commit the navigation.
-  Navigate(kBadURL, 1);
+  Navigate(kBadURL, 1, pending_id, true);
   // The interstitial should be gone now.
   ASSERT_FALSE(InterstitialPage::GetInterstitialPage(web_contents()));
 
   // A report should have been sent.
   EXPECT_EQ(1u, ui_manager_->GetDetails()->size());
   ui_manager_->GetDetails()->clear();
 }
 
 // Tests showing a blocking page for a page that contains malware subresources
 // and not proceeding.
 TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceDontProceed) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoogleURL, 1);
+  Navigate(kGoogleURL, 1, 0, true);
 
   // Navigate somewhere else.
-  Navigate(kGoodURL, 2);
+  Navigate(kGoodURL, 2, 0, true);

[mattm, 2015/04/24 23:42:37]

is nav_entry_id=0 some magic value? Why is it only non-zero sometimes? Would be
nice if TestDidNavigate was documented.

[Avi (use Gerrit), 2015/04/25 19:24:51]

I documented those values on RenderFrameHostTester but failed to document them
on WebContentsTester. Done.

 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "don't proceed".
   DontProceedThroughSubresourceInterstitial(sb_interstitial);
   EXPECT_EQ(CANCEL, user_response());
   EXPECT_FALSE(GetSafeBrowsingBlockingPage());
 
   // We did not proceed, we should be back to the first page, the 2nd one should
   // have been removed from the navigation controller.
   ASSERT_EQ(1, controller().GetEntryCount());
   EXPECT_EQ(kGoogleURL, controller().GetActiveEntry()->GetURL().spec());
 
   // A report should have been sent.
   EXPECT_EQ(1u, ui_manager_->GetDetails()->size());
   ui_manager_->GetDetails()->clear();
 }
 
 // Tests showing a blocking page for a page that contains malware subresources
 // and proceeding.
 TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceProceed) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoodURL, 1);
+  Navigate(kGoodURL, 1, 0, true);
 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "proceed".
   ProceedThroughInterstitial(sb_interstitial);
   EXPECT_EQ(OK, user_response());
   EXPECT_FALSE(GetSafeBrowsingBlockingPage());
 
   // We did proceed, we should be back to showing the page.
   ASSERT_EQ(1, controller().GetEntryCount());
   EXPECT_EQ(kGoodURL, controller().GetActiveEntry()->GetURL().spec());
 
   // A report should have been sent.
   EXPECT_EQ(1u, ui_manager_->GetDetails()->size());
   ui_manager_->GetDetails()->clear();
 }
 
 // Tests showing a blocking page for a page that contains multiple malware
 // subresources and not proceeding.  This just tests that the extra malware
 // subresources (which trigger queued interstitial pages) do not break anything.
 TEST_F(SafeBrowsingBlockingPageTest,
        PageWithMultipleMalwareResourceDontProceed) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoogleURL, 1);
+  Navigate(kGoogleURL, 1, 0, true);
 
   // Navigate somewhere else.
-  Navigate(kGoodURL, 2);
+  Navigate(kGoodURL, 2, 0, true);
 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
 
   // More bad resources loading causing more interstitials. The new
   // interstitials should be queued.
   ShowInterstitial(true, kBadURL2);
   ShowInterstitial(true, kBadURL3);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "don't proceed".
   DontProceedThroughSubresourceInterstitial(sb_interstitial);
   EXPECT_EQ(CANCEL, user_response());
   EXPECT_FALSE(GetSafeBrowsingBlockingPage());
 
   // We did not proceed, we should be back to the first page, the 2nd one should
   // have been removed from the navigation controller.
   ASSERT_EQ(1, controller().GetEntryCount());
   EXPECT_EQ(kGoogleURL, controller().GetActiveEntry()->GetURL().spec());
 
   // A report should have been sent.
   EXPECT_EQ(1u, ui_manager_->GetDetails()->size());
   ui_manager_->GetDetails()->clear();
 }
 
 // Tests showing a blocking page for a page that contains multiple malware
 // subresources and proceeding through the first interstitial, but not the next.
 TEST_F(SafeBrowsingBlockingPageTest,
        PageWithMultipleMalwareResourceProceedThenDontProceed) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoogleURL, 1);
+  Navigate(kGoogleURL, 1, 0, true);
 
   // Navigate somewhere else.
-  Navigate(kGoodURL, 2);
+  Navigate(kGoodURL, 2, 0, true);
 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
 
   // More bad resources loading causing more interstitials. The new
   // interstitials should be queued.
   ShowInterstitial(true, kBadURL2);
   ShowInterstitial(true, kBadURL3);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Proceed through the 1st interstitial.
   ProceedThroughInterstitial(sb_interstitial);
   EXPECT_EQ(OK, user_response());
 
   // A report should have been sent.
   EXPECT_EQ(1u, ui_manager_->GetDetails()->size());
   ui_manager_->GetDetails()->clear();
 
@@ skipping 15 matching lines @@
   EXPECT_EQ(kGoogleURL, controller().GetActiveEntry()->GetURL().spec());
 
   // No report should have been sent -- we don't create a report the
   // second time.
   EXPECT_EQ(0u, ui_manager_->GetDetails()->size());
   ui_manager_->GetDetails()->clear();
 }
 
 // Tests showing a blocking page for a page that contains multiple malware
 // subresources and proceeding through the multiple interstitials.
-TEST_F(SafeBrowsingBlockingPageTest,
-    PageWithMultipleMalwareResourceProceed) {
+TEST_F(SafeBrowsingBlockingPageTest, PageWithMultipleMalwareResourceProceed) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere else.
-  Navigate(kGoodURL, 1);
+  Navigate(kGoodURL, 1, 0, true);
 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
 
   // More bad resources loading causing more interstitials. The new
   // interstitials should be queued.
   ShowInterstitial(true, kBadURL2);
   ShowInterstitial(true, kBadURL3);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Proceed through the 1st interstitial.
   ProceedThroughInterstitial(sb_interstitial);
   EXPECT_EQ(OK, user_response());
 
   // A report should have been sent.
   EXPECT_EQ(1u, ui_manager_->GetDetails()->size());
   ui_manager_->GetDetails()->clear();
 
@@ skipping 21 matching lines @@
 // Tests showing a blocking page then navigating back and forth to make sure the
 // controller entries are OK.  http://crbug.com/17627
 TEST_F(SafeBrowsingBlockingPageTest, NavigatingBackAndForth) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoodURL, 1);
+  Navigate(kGoodURL, 1, 0, true);
 
   // Now navigate to a bad page triggerring an interstitial.
   controller().LoadURL(GURL(kBadURL), content::Referrer(),
                        ui::PAGE_TRANSITION_TYPED, std::string());
+  int pending_id = controller().GetPendingEntry()->GetUniqueID();
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Proceed, then navigate back.
   ProceedThroughInterstitial(sb_interstitial);
-  Navigate(kBadURL, 2);  // Commit the navigation.
+  Navigate(kBadURL, 2, pending_id, true);  // Commit the navigation.
   GoBack(true);
 
   // We are back on the good page.
   sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_FALSE(sb_interstitial);
   ASSERT_EQ(2, controller().GetEntryCount());
   EXPECT_EQ(kGoodURL, controller().GetActiveEntry()->GetURL().spec());
 
   // Navigate forward to the malware URL.
   web_contents()->GetController().GoForward();
+  pending_id = controller().GetPendingEntry()->GetUniqueID();
   ShowInterstitial(false, kBadURL);
   sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Let's proceed and make sure everything is OK (bug 17627).
   ProceedThroughInterstitial(sb_interstitial);
-  Navigate(kBadURL, 2);  // Commit the navigation.
+  // Commit the navigation.
+  NavigateCrossSite(kBadURL, 2, pending_id, false, true);

[mattm, 2015/04/24 23:42:37]

Since NavigateCrossSite is only directly called with is_cross_site=true, maybe
it should just mean that, and there can be some internal function that both
Navigate and NavigateCrossSite call. Also it looks like did_create_new_entry is
always true when calling Navigate and always false when calling
NavigateCrossSite, is that always the case? Can it also only be exposed on the
internal function to simplify things?

[Avi (use Gerrit), 2015/04/25 19:24:51]

did_create_new_entry is false because it is a history navigation. See the new
documentation that I added on WebContentsTester. I can't hide it because only
the caller of the Navigate functions would know the correct value.

Re the code cleanup, done. (I stole this bit of the code from the
DataReductionProxyDebugBlockingPageTest, so I made these changes there too.)

   sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_FALSE(sb_interstitial);
   ASSERT_EQ(2, controller().GetEntryCount());
   EXPECT_EQ(kBadURL, controller().GetActiveEntry()->GetURL().spec());
 
   // Two reports should have been sent.
   EXPECT_EQ(2u, ui_manager_->GetDetails()->size());
   ui_manager_->GetDetails()->clear();
 }
 
 // Tests that calling "don't proceed" after "proceed" has been called doesn't
 // cause problems. http://crbug.com/30079
 TEST_F(SafeBrowsingBlockingPageTest, ProceedThenDontProceed) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Start a load.
   controller().LoadURL(GURL(kBadURL), content::Referrer(),
                        ui::PAGE_TRANSITION_TYPED, std::string());
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   base::RunLoop().RunUntilIdle();
 
   // Simulate the user clicking "proceed" then "don't proceed" (before the
   // interstitial is shown).
   sb_interstitial->interstitial_page()->Proceed();
   sb_interstitial->interstitial_page()->DontProceed();
   // Proceed() and DontProceed() post a task to update the
   // SafeBrowsingService::Client.
@@ skipping 15 matching lines @@
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, false);
 
   // Start a load.
   controller().LoadURL(GURL(kBadURL), content::Referrer(),
                        ui::PAGE_TRANSITION_TYPED, std::string());
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   base::RunLoop().RunUntilIdle();
 
   // Simulate the user clicking "don't proceed".
   DontProceedThroughInterstitial(sb_interstitial);
 
   // The interstitial should be gone.
   EXPECT_EQ(CANCEL, user_response());
   EXPECT_FALSE(GetSafeBrowsingBlockingPage());
@@ skipping 13 matching lines @@
       web_contents()->GetBrowserContext());
   profile->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled, false);
 
   // Start a load.
   controller().LoadURL(GURL(kBadURL), content::Referrer(),
                        ui::PAGE_TRANSITION_TYPED, std::string());
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   base::RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(profile->GetPrefs()->GetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled));
 
   // Simulate the user check the report agreement checkbox.
   sb_interstitial->SetReportingPreference(true);
 
   EXPECT_TRUE(profile->GetPrefs()->GetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled));
 
   // Simulate the user uncheck the report agreement checkbox.
   sb_interstitial->SetReportingPreference(false);
 
   EXPECT_FALSE(profile->GetPrefs()->GetBoolean(
       prefs::kSafeBrowsingExtendedReportingEnabled));
 }