Fixes for Safe Browsing with unrelated pending navigations.

chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc

Index: chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc b/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
index 99cf0c807dfa01a41a30fc6079ffab55266d97c1..f3509ffad3166c25f3bc2431b6adb86cba36ed8b 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
@@ -18,11 +18,13 @@
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/thread_test_helper.h"
 #include "base/time/time.h"
 #include "chrome/browser/bookmarks/startup_task_runner_service_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -36,6 +38,7 @@
 #include "chrome/browser/safe_browsing/safe_browsing_util.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
@@ -43,8 +46,12 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/bookmarks/browser/startup_task_runner_service.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "content/public/browser/interstitial_page.h"
+#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
 #include "net/cookies/cookie_store.h"
 #include "net/cookies/cookie_util.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -75,12 +82,60 @@ namespace safe_browsing {
 
 namespace {
 
+const char kEmptyPage[] = "/empty.html";
+const char kMalwareFile[] = "/downloads/dangerous/dangerous.exe";
+const char kMalwarePage[] = "/safe_browsing/malware.html";
+const char kMalwareDelayedLoadsPage[] =
+    "/safe_browsing/malware_delayed_loads.html";
+const char kMalwareIFrame[] = "/safe_browsing/malware_iframe.html";
+const char kMalwareImg[] = "/safe_browsing/malware_image.png";
+const char kNeverCompletesPath[] = "/never_completes";
+
+class NeverCompletingHttpResponse : public net::test_server::HttpResponse {
+ public:
+  ~NeverCompletingHttpResponse() override {}
+
+  void SendResponse(
+      const net::test_server::SendBytesCallback& send,
+      const net::test_server::SendCompleteCallback& done) override {
+    // Do nothing. |done| is never called.
+  }
+};
+
+scoped_ptr<net::test_server::HttpResponse> HandleNeverCompletingRequests(
+    const net::test_server::HttpRequest& request) {
+  if (!base::StartsWith(request.relative_url, kNeverCompletesPath,
+                          base::CompareCase::SENSITIVE))
+    return scoped_ptr<net::test_server::HttpResponse>();

[Bernhard Bauer, 2015/12/17 16:02:02]

Turns out you can actually return nullptr here ☺

[mattm, 2015/12/18 21:41:04]

Done.

+  return make_scoped_ptr(new NeverCompletingHttpResponse());
+}
+
 void InvokeFullHashCallback(
     SafeBrowsingProtocolManager::FullHashCallback callback,
     const std::vector<SBFullHashResult>& result) {
   callback.Run(result, base::TimeDelta::FromMinutes(45));
 }
 
+class FakeSafeBrowsingUIManager : public SafeBrowsingUIManager {
+ public:
+  explicit FakeSafeBrowsingUIManager(SafeBrowsingService* service)
+      : SafeBrowsingUIManager(service) {}
+
+  void MaybeReportSafeBrowsingHit(
+      const safe_browsing::HitReport& hit_report) override {
+    EXPECT_FALSE(got_hit_report_);
+    got_hit_report_ = true;
+    hit_report_ = hit_report;
+    SafeBrowsingUIManager::MaybeReportSafeBrowsingHit(hit_report);
+  }
+
+  bool got_hit_report_ = false;
+  safe_browsing::HitReport hit_report_;
+
+ protected:

[Bernhard Bauer, 2015/12/17 16:02:02]

Why not private? Is this subclassed?

[mattm, 2015/12/18 21:41:04]

Done.

+  ~FakeSafeBrowsingUIManager() override {}
+};
+
 class FakeSafeBrowsingService : public SafeBrowsingService {
  public:
   explicit FakeSafeBrowsingService(const std::string& url_prefix)
@@ -96,6 +151,11 @@ class FakeSafeBrowsingService : public SafeBrowsingService {
     return config;
   }
 
+ protected:
+  SafeBrowsingUIManager* CreateUIManager() override {
+    return new FakeSafeBrowsingUIManager(this);
+  }
+
  private:
   ~FakeSafeBrowsingService() override {}
 
@@ -459,6 +519,8 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest {
   void SetUpInProcessBrowserTestFixture() override {
     base::FilePath test_data_dir;
     PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
+    embedded_test_server()->RegisterRequestHandler(
+        base::Bind(&HandleNeverCompletingRequests));
     embedded_test_server()->ServeFilesFromDirectory(test_data_dir);
     ASSERT_TRUE(embedded_test_server()->Start());
   }
@@ -523,6 +585,15 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest {
 #endif
   }
 
+  FakeSafeBrowsingUIManager* ui_manager() {
+    return static_cast<FakeSafeBrowsingUIManager*>(
+        g_browser_process->safe_browsing_service()->ui_manager().get());
+  }
+  bool got_hit_report() { return ui_manager()->got_hit_report_; }
+  const safe_browsing::HitReport& hit_report() {
+    return ui_manager()->hit_report_;
+  }
+
  protected:
   StrictMock<MockObserver> observer_;
 
@@ -602,15 +673,6 @@ class SafeBrowsingServiceMetadataTest
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingServiceMetadataTest);
 };
 
-namespace {
-
-const char kEmptyPage[] = "/empty.html";
-const char kMalwareFile[] = "/downloads/dangerous/dangerous.exe";
-const char kMalwarePage[] = "/safe_browsing/malware.html";
-const char kMalwareIFrame[] = "/safe_browsing/malware_iframe.html";
-const char kMalwareImg[] = "/safe_browsing/malware_image.png";
-
-// This test goes through DownloadResourceHandler.
 IN_PROC_BROWSER_TEST_P(SafeBrowsingServiceMetadataTest, MalwareMainFrame) {
   GURL url = embedded_test_server()->GetURL(kEmptyPage);
 
@@ -623,6 +685,12 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingServiceMetadataTest, MalwareMainFrame) {
   ui_test_utils::NavigateToURL(browser(), url);
   // All types should show the interstitial.
   EXPECT_TRUE(ShowingInterstitialPage());
+
+  EXPECT_TRUE(got_hit_report());
+  EXPECT_EQ(url, hit_report().malicious_url);
+  EXPECT_EQ(url, hit_report().page_url);
+  EXPECT_EQ(GURL(), hit_report().referrer_url);
+  EXPECT_FALSE(hit_report().is_subresource);
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingServiceMetadataTest, MalwareIFrame) {
@@ -638,6 +706,12 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingServiceMetadataTest, MalwareIFrame) {
   ui_test_utils::NavigateToURL(browser(), main_url);
   // All types should show the interstitial.
   EXPECT_TRUE(ShowingInterstitialPage());
+
+  EXPECT_TRUE(got_hit_report());
+  EXPECT_EQ(iframe_url, hit_report().malicious_url);
+  EXPECT_EQ(main_url, hit_report().page_url);
+  EXPECT_EQ(GURL(), hit_report().referrer_url);
+  EXPECT_TRUE(hit_report().is_subresource);
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingServiceMetadataTest, MalwareImg) {
@@ -665,9 +739,15 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingServiceMetadataTest, MalwareImg) {
     case METADATA_NONE:
     case METADATA_DISTRIBUTION:
       EXPECT_TRUE(ShowingInterstitialPage());
+      EXPECT_TRUE(got_hit_report());
+      EXPECT_EQ(img_url, hit_report().malicious_url);
+      EXPECT_EQ(main_url, hit_report().page_url);
+      EXPECT_EQ(GURL(), hit_report().referrer_url);
+      EXPECT_TRUE(hit_report().is_subresource);
       break;
     case METADATA_LANDING:
       EXPECT_FALSE(ShowingInterstitialPage());
+      EXPECT_FALSE(got_hit_report());
       break;
   }
 }
@@ -691,6 +771,7 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, UnwantedImgIgnored) {
   ui_test_utils::NavigateToURL(browser(), main_url);
 
   EXPECT_FALSE(ShowingInterstitialPage());
+  EXPECT_FALSE(got_hit_report());
 }
 
 IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, MalwareWithWhitelist) {
@@ -759,6 +840,7 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, Prefetch) {
   SetupResponseForUrl(malware_url, malware_full_hash);
   ui_test_utils::NavigateToURL(browser(), url);
   EXPECT_FALSE(ShowingInterstitialPage());
+  EXPECT_FALSE(got_hit_report());
   Mock::VerifyAndClear(&observer_);
 
   // However, when we navigate to the malware page, we should still get
@@ -767,10 +849,246 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, Prefetch) {
       .Times(1);
   ui_test_utils::NavigateToURL(browser(), malware_url);
   EXPECT_TRUE(ShowingInterstitialPage());
+  EXPECT_TRUE(got_hit_report());
   Mock::VerifyAndClear(&observer_);
 }
 
-}  // namespace
+IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, MainFrameHitWithReferrer) {
+  GURL first_url = embedded_test_server()->GetURL(kEmptyPage);
+  GURL bad_url = embedded_test_server()->GetURL(kMalwarePage);
+
+  SBFullHashResult malware_full_hash;
+  GenUrlFullhashResult(bad_url, MALWARE, &malware_full_hash);
+  SetupResponseForUrl(bad_url, malware_full_hash);
+
+  // Navigate to first, safe page.
+  ui_test_utils::NavigateToURL(browser(), first_url);
+  EXPECT_FALSE(ShowingInterstitialPage());
+  EXPECT_FALSE(got_hit_report());
+  Mock::VerifyAndClear(&observer_);
+
+  // Navigate to malware page, should show interstitial and have first page in
+  // referrer.
+  EXPECT_CALL(observer_, OnSafeBrowsingHit(IsUnsafeResourceFor(bad_url)))
+      .Times(1);
+
+  chrome::NavigateParams params(browser(), bad_url, ui::PAGE_TRANSITION_LINK);
+  params.referrer.url = first_url;
+  ui_test_utils::NavigateToURL(&params);
+
+  EXPECT_TRUE(ShowingInterstitialPage());
+  EXPECT_TRUE(got_hit_report());
+  EXPECT_EQ(bad_url, hit_report().malicious_url);
+  EXPECT_EQ(bad_url, hit_report().page_url);
+  EXPECT_EQ(first_url, hit_report().referrer_url);
+  EXPECT_FALSE(hit_report().is_subresource);
+}
+
+IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest,
+                       SubResourceHitWithMainFrameReferrer) {
+  GURL first_url = embedded_test_server()->GetURL(kEmptyPage);
+  GURL second_url = embedded_test_server()->GetURL(kMalwarePage);
+  GURL bad_url = embedded_test_server()->GetURL(kMalwareImg);
+
+  SBFullHashResult malware_full_hash;
+  GenUrlFullhashResult(bad_url, MALWARE, &malware_full_hash);
+  SetupResponseForUrl(bad_url, malware_full_hash);
+
+  // Navigate to first, safe page.
+  ui_test_utils::NavigateToURL(browser(), first_url);
+  EXPECT_FALSE(ShowingInterstitialPage());
+  EXPECT_FALSE(got_hit_report());
+  Mock::VerifyAndClear(&observer_);
+
+  // Navigate to page which has malware subresource, should show interstitial
+  // and have first page in referrer.
+  EXPECT_CALL(observer_, OnSafeBrowsingHit(IsUnsafeResourceFor(bad_url)))
+      .Times(1);
+
+  chrome::NavigateParams params(browser(), second_url,
+                                ui::PAGE_TRANSITION_LINK);
+  params.referrer.url = first_url;
+  ui_test_utils::NavigateToURL(&params);
+
+  EXPECT_TRUE(ShowingInterstitialPage());
+  EXPECT_TRUE(got_hit_report());
+  EXPECT_EQ(bad_url, hit_report().malicious_url);
+  EXPECT_EQ(second_url, hit_report().page_url);
+  EXPECT_EQ(first_url, hit_report().referrer_url);
+  EXPECT_TRUE(hit_report().is_subresource);
+}
+
+IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest,
+                       SubResourceHitWithMainFrameRendererInitiatedSlowLoad) {
+  GURL first_url = embedded_test_server()->GetURL(kEmptyPage);
+  GURL second_url = embedded_test_server()->GetURL(kMalwareDelayedLoadsPage);
+  GURL third_url = embedded_test_server()->GetURL(kNeverCompletesPath);
+  GURL bad_url = embedded_test_server()->GetURL(kMalwareImg);
+
+  SBFullHashResult malware_full_hash;
+  GenUrlFullhashResult(bad_url, MALWARE, &malware_full_hash);
+  SetupResponseForUrl(bad_url, malware_full_hash);
+
+  // Navigate to first, safe page.
+  ui_test_utils::NavigateToURL(browser(), first_url);
+  EXPECT_FALSE(ShowingInterstitialPage());
+  EXPECT_FALSE(got_hit_report());
+  Mock::VerifyAndClear(&observer_);
+
+  // Navigate to malware page. The malware subresources haven't loaded yet, so
+  // no interstitial should show yet.
+  chrome::NavigateParams params(browser(), second_url,
+                                ui::PAGE_TRANSITION_LINK);
+  params.referrer.url = first_url;
+  ui_test_utils::NavigateToURL(&params);
+
+  EXPECT_FALSE(ShowingInterstitialPage());
+  EXPECT_FALSE(got_hit_report());
+  Mock::VerifyAndClear(&observer_);
+
+  EXPECT_CALL(observer_, OnSafeBrowsingHit(IsUnsafeResourceFor(bad_url)))
+      .Times(1);
+
+  WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::WindowedNotificationObserver load_stop_observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::Source<content::NavigationController>(
+           &contents->GetController()));
+  // Run javascript function in the page which starts a timer to load the
+  // malware image, and also starts a renderer-initiated top-level navigation to
+  // a site that does not respond.  Should show interstitial and have first page
+  // in referrer.
+  contents->GetMainFrame()->ExecuteJavaScriptForTests(
+      base::ASCIIToUTF16("navigateAndLoadMalwareImage()"));
+  load_stop_observer.Wait();
+
+  EXPECT_TRUE(ShowingInterstitialPage());
+  EXPECT_TRUE(got_hit_report());
+  // Report URLs should be for the current page, not the pending load.
+  EXPECT_EQ(bad_url, hit_report().malicious_url);
+  EXPECT_EQ(second_url, hit_report().page_url);
+  EXPECT_EQ(first_url, hit_report().referrer_url);
+  EXPECT_TRUE(hit_report().is_subresource);
+}
+
+IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest,
+                       SubResourceHitWithMainFrameBrowserInitiatedSlowLoad) {
+  GURL first_url = embedded_test_server()->GetURL(kEmptyPage);
+  GURL second_url = embedded_test_server()->GetURL(kMalwareDelayedLoadsPage);
+  GURL third_url = embedded_test_server()->GetURL(kNeverCompletesPath);
+  GURL bad_url = embedded_test_server()->GetURL(kMalwareImg);
+
+  SBFullHashResult malware_full_hash;
+  GenUrlFullhashResult(bad_url, MALWARE, &malware_full_hash);
+  SetupResponseForUrl(bad_url, malware_full_hash);
+
+  // Navigate to first, safe page.
+  ui_test_utils::NavigateToURL(browser(), first_url);
+  EXPECT_FALSE(ShowingInterstitialPage());
+  EXPECT_FALSE(got_hit_report());
+  Mock::VerifyAndClear(&observer_);
+
+  // Navigate to malware page. The malware subresources haven't loaded yet, so
+  // no interstitial should show yet.
+  chrome::NavigateParams params(browser(), second_url,
+                                ui::PAGE_TRANSITION_LINK);
+  params.referrer.url = first_url;
+  ui_test_utils::NavigateToURL(&params);
+
+  EXPECT_FALSE(ShowingInterstitialPage());
+  EXPECT_FALSE(got_hit_report());
+  Mock::VerifyAndClear(&observer_);
+
+  EXPECT_CALL(observer_, OnSafeBrowsingHit(IsUnsafeResourceFor(bad_url)))
+      .Times(1);
+
+  WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::RenderFrameHost* rfh = contents->GetMainFrame();
+  content::WindowedNotificationObserver load_stop_observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::Source<content::NavigationController>(
+           &contents->GetController()));
+  // Start a browser initiated top-level navigation to a site that does not
+  // respond.
+  ui_test_utils::NavigateToURLWithDisposition(browser(), third_url, CURRENT_TAB,
+                                              ui_test_utils::BROWSER_TEST_NONE);
+
+  // While the top-level navigation is pending, run javascript
+  // function in the page which loads the malware image.
+  rfh->ExecuteJavaScriptForTests(base::ASCIIToUTF16("loadMalwareImage()"));
+
+  // Wait for interstitial to show.
+  load_stop_observer.Wait();
+
+  EXPECT_TRUE(ShowingInterstitialPage());
+  EXPECT_TRUE(got_hit_report());
+  // Report URLs should be for the current page, not the pending load.
+  EXPECT_EQ(bad_url, hit_report().malicious_url);
+  EXPECT_EQ(second_url, hit_report().page_url);
+  EXPECT_EQ(first_url, hit_report().referrer_url);
+  EXPECT_TRUE(hit_report().is_subresource);
+}
+
+IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, SubResourceHitOnFreshTab) {
+  // Allow popups.
+  HostContentSettingsMapFactory::GetForProfile(browser()->profile())
+      ->SetContentSetting(ContentSettingsPattern::Wildcard(),
+                          ContentSettingsPattern::Wildcard(),
+                          CONTENT_SETTINGS_TYPE_POPUPS, std::string(),
+                          CONTENT_SETTING_ALLOW);
+
+  // Add |kMalwareImg| to fake safebrowsing db.
+  GURL img_url = embedded_test_server()->GetURL(kMalwareImg);
+  SBFullHashResult img_full_hash;
+  GenUrlFullhashResult(img_url, MALWARE, &img_full_hash);
+  SetupResponseForUrl(img_url, img_full_hash);
+
+  // Have the current tab open a new tab with window.open().
+  WebContents* main_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::RenderFrameHost* main_rfh = main_contents->GetMainFrame();
+
+  content::WebContentsAddedObserver web_contents_added_observer;
+  main_rfh->ExecuteJavaScriptForTests(
+      base::ASCIIToUTF16("w=window.open();"));
+  WebContents* new_tab_contents = web_contents_added_observer.GetWebContents();
+  content::RenderFrameHost* new_tab_rfh = new_tab_contents->GetMainFrame();
+  // A fresh WebContents should not have any NavigationEntries yet. (See
+  // https://crbug.com/524208.)
+  EXPECT_EQ(nullptr, new_tab_contents->GetController().GetLastCommittedEntry());
+  EXPECT_EQ(nullptr, new_tab_contents->GetController().GetPendingEntry());
+
+  // Run javascript in the blank new tab to load the malware image.
+  EXPECT_CALL(observer_, OnSafeBrowsingHit(IsUnsafeResourceFor(img_url)))
+      .Times(1);
+  new_tab_rfh->ExecuteJavaScriptForTests(
+      base::ASCIIToUTF16("var img=new Image();"
+                         "img.src=\"" + img_url.spec() + "\";"
+                         "document.body.appendChild(img);"));
+
+  // Wait for interstitial to show.
+  content::WaitForInterstitialAttach(new_tab_contents);
+  Mock::VerifyAndClearExpectations(&observer_);
+  EXPECT_TRUE(ShowingInterstitialPage());
+  EXPECT_TRUE(got_hit_report());
+  EXPECT_EQ(img_url, hit_report().malicious_url);
+  EXPECT_TRUE(hit_report().is_subresource);
+  // Page report URLs should be empty, since there is no URL for this page.
+  EXPECT_EQ(GURL(), hit_report().page_url);
+  EXPECT_EQ(GURL(), hit_report().referrer_url);
+
+  // Proceed through it.
+  InterstitialPage* interstitial_page = new_tab_contents->GetInterstitialPage();
+  ASSERT_TRUE(interstitial_page);
+  interstitial_page->Proceed();
+
+  content::WaitForInterstitialDetach(new_tab_contents);
+  EXPECT_FALSE(ShowingInterstitialPage());
+}
+
+namespace {
 
 class TestSBClient : public base::RefCountedThreadSafe<TestSBClient>,
                      public SafeBrowsingDatabaseManager::Client {
@@ -847,10 +1165,10 @@ class TestSBClient : public base::RefCountedThreadSafe<TestSBClient>,
   DISALLOW_COPY_AND_ASSIGN(TestSBClient);
 };
 
+}  // namespace
+
 // These tests use SafeBrowsingService::Client to directly interact with
 // SafeBrowsingService.
-namespace {
-
 IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, CheckDownloadUrl) {
   GURL badbin_url = embedded_test_server()->GetURL(kMalwareFile);
   std::vector<GURL> badbin_urls(1, badbin_url);
@@ -1084,8 +1402,6 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, StartAndStop) {
   EXPECT_FALSE(csd_service->enabled());
 }
 
-}  // namespace
-
 class SafeBrowsingServiceShutdownTest : public SafeBrowsingServiceTest {
  public:
   void TearDown() override {