Compare non-whitelisted off-domain inclusions against the browsing history.

chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc

Index: chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
diff --git a/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
index a1a0eb005638a3688af1de8e62381088aded4955..bda17ca41466dfb1aed42a529d2092c7d7a41034 100644
--- a/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
+++ b/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
@@ -3,16 +3,35 @@
 // found in the LICENSE file.
 
 #include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
+#include "chrome/browser/history/chrome_history_client.h"
+#include "chrome/browser/history/chrome_history_client_factory.h"
+#include "chrome/browser/history/history_service.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/history/web_history_service_factory.h"
+#include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/safe_browsing/database_manager.h"
 #include "chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_pref_service_syncable.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/testing_profile_manager.h"
+#include "components/history/core/browser/history_constants.h"
+#include "components/history/core/browser/history_types.h"
+#include "components/keyed_service/core/service_access_type.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/resource_type.h"
 #include "content/public/test/test_browser_thread_bundle.h"
-#include "ipc/ipc_message.h"
 #include "net/base/request_priority.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_test_util.h"
@@ -25,6 +44,38 @@ using testing::NiceMock;
 using testing::Return;
 using testing::Values;
 
+namespace {
+
+const int kFakeRenderProcessId = 123;
+const int kFakeRenderFrameId = 456;
+
+// A BrowserContextKeyedServiceFactory::TestingFactoryFunction that creates a
+// HistoryService for a TestingProfile.
+KeyedService* BuildHistoryService(content::BrowserContext* context) {
+  TestingProfile* profile = static_cast<TestingProfile*>(context);
+
+  // Delete the file before creating the service.
+  base::FilePath history_path(
+      profile->GetPath().Append(history::kHistoryFilename));
+  if (!base::DeleteFile(history_path, false) ||
+      base::PathExists(history_path)) {
+    ADD_FAILURE() << "failed to delete history db file "
+                  << history_path.value();
+    return NULL;
+  }
+
+  HistoryService* history_service = new HistoryService(

[Alexei Svitkine (slow), 2015/01/20 17:48:13]

Should this be a scoped_ptr? If you're returning ownership to the caller, return
value should be a scoped_ptr too.

[gab, 2015/01/20 18:10:48]

No, ProfileKeyedServices are owned by the profiles, the factories only hand out
raw pointers to them (and initially build them if none is associated with a
given profile).

[Alexei Svitkine (slow), 2015/01/20 18:17:48]

If so, why is it safe to delete it when Init() returns false? I didn't see
anything in the documentation for Init() that talks about ownership...

[gab, 2015/01/20 18:51:57]

My bad, I responded too quick, somehow I thought the discussion was on line 209
below where we get a raw HistoryService*.

I agree with you that here the return type should be scoped_ptr<KeyedService>
but this is unfortunately not how BrowserContextKeyedService's and their
factories functions are designed (see
https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...).

This is using the same approach as
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...
perhaps a common helper could be built, I was hoping to have grt@ weigh in on
his opinion for that given he's the one who implemented the copied instance.

+      ChromeHistoryClientFactory::GetForProfile(profile), profile);
+  if (history_service->Init(profile->GetPath()))
+    return history_service;
+
+  ADD_FAILURE() << "failed to initialize history service";
+  delete history_service;
+  return NULL;
+}
+
+}  // namespace
+
 namespace safe_browsing {
 
 using AnalysisEvent = OffDomainInclusionDetector::AnalysisEvent;
@@ -63,6 +114,8 @@ static_assert(
 // A set of test cases to run each parametrized test case below through.
 enum class OffDomainInclusionTestCases {
   OFF_DOMAIN_INCLUSION_WHITELISTED,
+  OFF_DOMAIN_INCLUSION_IN_HISTORY,
+  OFF_DOMAIN_INCLUSION_IN_HISTORY_AND_WHITELISTED,
   OFF_DOMAIN_INCLUSION_UNKNOWN,
 };
 
@@ -81,32 +134,58 @@ class MockSafeBrowsingDatabaseManager : public SafeBrowsingDatabaseManager {
   DISALLOW_COPY_AND_ASSIGN(MockSafeBrowsingDatabaseManager);
 };
 
+// Adds |url| to |history_service| upon being constructed and removes it upon
+// being destructed.
+class ScopedHistoryEntry {
+ public:
+  ScopedHistoryEntry(HistoryService* history_service, const GURL& url)
+      : history_service_(history_service), url_(url) {
+    base::RunLoop run_loop;
+
+    history_service_->AddPage(url_, base::Time::Now(), history::SOURCE_BROWSED);
+
+    // Flush tasks posted on the history thread.
+    history_service_->FlushForTest(run_loop.QuitClosure());
+    run_loop.Run();
+    // Make sure anything bounced back to the main thread has been handled.
+    base::RunLoop().RunUntilIdle();
+  }
+
+  ~ScopedHistoryEntry() {
+    base::RunLoop run_loop;
+
+    history_service_->DeleteURL(url_);
+
+    history_service_->FlushForTest(run_loop.QuitClosure());
+    run_loop.Run();
+    base::RunLoop().RunUntilIdle();
+  }
+
+ private:
+  HistoryService* history_service_;
+  const GURL url_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedHistoryEntry);
+};
+
 class OffDomainInclusionDetectorTest
     : public testing::TestWithParam<OffDomainInclusionTestCases> {
  protected:
   OffDomainInclusionDetectorTest()
-      : observed_analysis_event_(AnalysisEvent::NO_EVENT) {}
+      : testing_profile_(nullptr),
+        observed_analysis_event_(AnalysisEvent::NO_EVENT) {}
 
   void SetUp() override {
-    // Only used for initializing MockSafeBrowsingDatabaseManager.
-    scoped_refptr<SafeBrowsingService> sb_service =
-        SafeBrowsingService::CreateSafeBrowsingService();
-
-    scoped_refptr<MockSafeBrowsingDatabaseManager>
-        mock_safe_browsing_database_manager =
-            new NiceMock<MockSafeBrowsingDatabaseManager>(sb_service);
-    ON_CALL(*mock_safe_browsing_database_manager, MatchInclusionWhitelistUrl(_))
-        .WillByDefault(Return(
-            GetParam() ==
-            OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED));
-
-    off_domain_inclusion_detector_.reset(new OffDomainInclusionDetector(
-        mock_safe_browsing_database_manager,
-        base::Bind(&OffDomainInclusionDetectorTest::OnOffDomainInclusionEvent,
-                   base::Unretained(this))));
+    InitTestProfile();
+    InitOffDomainInclusionDetector();
   }
 
   void TearDown() override {
+    // Shut down the history service.
+    testing_profile_->AsTestingProfile()->DestroyHistoryService();
+    profile_manager_.reset();
+    TestingBrowserProcess::DeleteInstance();
+
     // The SafeBrowsingService held by the MockSafeBrowsingDatabaseManager in
     // |off_domain_inclusion_detector_| is deleted asynchronously and we thus
     // need to cleanup explicitly here or the test will leak.
@@ -120,13 +199,23 @@ class OffDomainInclusionDetectorTest
     return last_event;
   }
 
-  void SimulateTestURLRequest(const std::string& url,
+  void SimulateTestURLRequest(const std::string& url_str,
                               const std::string& referrer,
                               content::ResourceType resource_type,
                               bool is_main_frame,
                               bool parent_is_main_frame) const {
+    const GURL url(url_str);
+
+    HistoryService* history_service = HistoryServiceFactory::GetForProfile(
+        testing_profile_, ServiceAccessType::EXPLICIT_ACCESS);
+    scoped_ptr<ScopedHistoryEntry> scoped_history_entry;
+    if (ShouldAddSimulatedURLsToHistory()) {
+      scoped_history_entry.reset(
+          new ScopedHistoryEntry(history_service, url));
+    }
+
     scoped_ptr<net::URLRequest> url_request(
-        context_.CreateRequest(GURL(url), net::DEFAULT_PRIORITY, NULL, NULL));
+        context_.CreateRequest(url, net::DEFAULT_PRIORITY, NULL, NULL));
 
     if (!referrer.empty())
       url_request->SetReferrer(referrer);
@@ -135,9 +224,9 @@ class OffDomainInclusionDetectorTest
         url_request.get(),
         resource_type,
         NULL,                  // resource_context
-        0,                     // render_process_id
+        kFakeRenderProcessId,  // render_process_id
         0,                     // render_view_id
-        MSG_ROUTING_NONE,      // render_frame_id
+        kFakeRenderFrameId,    // render_frame_id
         is_main_frame,         // is_main_frame
         parent_is_main_frame,  // parent_is_main_frame
         true,                  // allow_download
@@ -145,29 +234,129 @@ class OffDomainInclusionDetectorTest
 
     off_domain_inclusion_detector_->OnResourceRequest(url_request.get());
 
-    // OffDomainInclusionDetector::OnResourceRequest() sometimes completes
+    // OffDomainInclusionDetector::OnResourceRequest() may complete
     // asynchronously, run all message loops (i.e. this message loop in unit
     // tests) until idle.
     base::RunLoop().RunUntilIdle();
+
+    // Make sure any task posted by the OffDomainInclusionDetector to the
+    // history thread has occurred.
+    base::RunLoop run_loop;
+    history_service->FlushForTest(run_loop.QuitClosure());
+    run_loop.Run();
+
+    // Finally, finish anything the history thread may have bounced back to the
+    // main thread.
+    base::RunLoop().RunUntilIdle();
   }
 
   // Returns the expected AnalysisEvent produced when facing an off-domain
   // inclusion in the current test configuration.
   AnalysisEvent GetExpectedOffDomainInclusionEventType() {
+    switch (GetParam()) {
+      case OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED:
+        return AnalysisEvent::OFF_DOMAIN_INCLUSION_WHITELISTED;
+      case OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_IN_HISTORY:
+        return AnalysisEvent::OFF_DOMAIN_INCLUSION_IN_HISTORY;
+      case OffDomainInclusionTestCases::
+          OFF_DOMAIN_INCLUSION_IN_HISTORY_AND_WHITELISTED:
+        return AnalysisEvent::OFF_DOMAIN_INCLUSION_WHITELISTED;
+      case OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_UNKNOWN:
+        return AnalysisEvent::OFF_DOMAIN_INCLUSION_SUSPICIOUS;
+    }
+    NOTREACHED();
+    return AnalysisEvent::NO_EVENT;
+  }
+
+ private:
+  // Returns true if the inclusion whitelist should be mocked to return a match
+  // on all requests.
+  bool ShouldWhitelistEverything() const {
     return GetParam() ==
-                   OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED
-               ? AnalysisEvent::OFF_DOMAIN_INCLUSION_WHITELISTED
-               : AnalysisEvent::OFF_DOMAIN_INCLUSION_SUSPICIOUS;
+               OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED ||
+           GetParam() == OffDomainInclusionTestCases::
+                             OFF_DOMAIN_INCLUSION_IN_HISTORY_AND_WHITELISTED;
   }
 
+  // Returns true if simulated URLs should be added to history.
+  bool ShouldAddSimulatedURLsToHistory() const {
+    return GetParam() ==
+               OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_IN_HISTORY ||
+           GetParam() == OffDomainInclusionTestCases::
+                             OFF_DOMAIN_INCLUSION_IN_HISTORY_AND_WHITELISTED;
+  }
 
- private:
   void OnOffDomainInclusionEvent(AnalysisEvent event) {
     // Make sure no other AnalysisEvent was previously reported.
     EXPECT_EQ(AnalysisEvent::NO_EVENT, observed_analysis_event_);
     observed_analysis_event_ = event;
   }
 
+  Profile* GetTestProfileForTestRenderFrameId(int render_process_id,
+                                              int render_frame_id) {
+    EXPECT_EQ(kFakeRenderProcessId, render_process_id);
+    EXPECT_EQ(kFakeRenderFrameId, render_frame_id);
+    return testing_profile_;
+  }
+
+  void InitTestProfile() {
+    ASSERT_FALSE(profile_manager_);
+    ASSERT_FALSE(testing_profile_);
+
+    profile_manager_.reset(
+        new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
+    ASSERT_TRUE(profile_manager_->SetUp());
+
+    // Set up a mock keyed service factory for the HistoryService.
+    TestingProfile::TestingFactories factories;
+    factories.push_back(std::make_pair(HistoryServiceFactory::GetInstance(),
+                                       &BuildHistoryService));
+    // Suppress WebHistoryService since it makes network requests.
+    factories.push_back(std::make_pair(
+        WebHistoryServiceFactory::GetInstance(),
+        static_cast<BrowserContextKeyedServiceFactory::TestingFactoryFunction>(
+            NULL)));
+
+    // Create default prefs for the test profile.
+    scoped_ptr<TestingPrefServiceSyncable> prefs(
+        new TestingPrefServiceSyncable);
+    chrome::RegisterUserProfilePrefs(prefs->registry());
+
+    // |testing_profile_| is owned by |profile_manager_|.
+    testing_profile_ = profile_manager_->CreateTestingProfile(
+        "profile",                  // profile_name
+        prefs.Pass(),
+        base::UTF8ToUTF16("user"),  // user_name
+        0,                          // avatar_id
+        std::string(),              // supervised_user_id
+        factories);
+  }
+
+  void InitOffDomainInclusionDetector() {
+    ASSERT_FALSE(off_domain_inclusion_detector_);
+
+    // Only used for initializing MockSafeBrowsingDatabaseManager.
+    scoped_refptr<SafeBrowsingService> sb_service =
+        SafeBrowsingService::CreateSafeBrowsingService();
+
+    scoped_refptr<MockSafeBrowsingDatabaseManager>
+        mock_safe_browsing_database_manager =
+            new NiceMock<MockSafeBrowsingDatabaseManager>(sb_service);
+    ON_CALL(*mock_safe_browsing_database_manager, MatchInclusionWhitelistUrl(_))
+        .WillByDefault(Return(ShouldWhitelistEverything()));
+
+    off_domain_inclusion_detector_.reset(new OffDomainInclusionDetector(
+        mock_safe_browsing_database_manager,
+        base::Bind(&OffDomainInclusionDetectorTest::OnOffDomainInclusionEvent,
+                   base::Unretained(this)),
+        base::Bind(
+            &OffDomainInclusionDetectorTest::GetTestProfileForTestRenderFrameId,
+            base::Unretained(this))));
+  }
+
+  Profile* testing_profile_;
+  scoped_ptr<TestingProfileManager> profile_manager_;
+
   content::TestBrowserThreadBundle thread_bundle_;
   net::TestURLRequestContext context_;
 
@@ -342,7 +531,8 @@ TEST_P(OffDomainInclusionDetectorTest,
                            true,    // is_main_frame
                            false);  // parent_is_main_frame
 
-    EXPECT_EQ(AnalysisEvent::EMPTY_MAIN_FRAME_URL, GetLastEventAndReset());
+    EXPECT_EQ(AnalysisEvent::ABORT_EMPTY_MAIN_FRAME_URL,
+              GetLastEventAndReset());
   }
 }
 
@@ -350,6 +540,9 @@ INSTANTIATE_TEST_CASE_P(
     OffDomainInclusionDetectorTestInstance,
     OffDomainInclusionDetectorTest,
     Values(OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED,
+           OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_IN_HISTORY,
+           OffDomainInclusionTestCases::
+               OFF_DOMAIN_INCLUSION_IN_HISTORY_AND_WHITELISTED,
            OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_UNKNOWN));
 
 }  // namespace safe_browsing