Add event handlers to SWAN trigger.

The trigger now listens to page load events, suspicious site list
hits, and a report delay callback. A simple state model reconciles
these events and creates reports at the right time.

Also some basic unit tests.

This is a no-op since the trigger is disabled by default.
Next CL will add metrics to the trigger and more tests.

Bug: 817377
Change-Id: Ica12e87debf39fb4f2e47a4e92d1f9cf1d40491e
Reviewed-on: https://chromium-review.googlesource.com/998532
Commit-Queue: Luke Z <lpz@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549208}

components/safe_browsing/triggers/BUILD.gn

@@ -71,11 +71,15 @@ source_set("unit_tests") {
   testonly = true
   sources = [
     "ad_sampler_trigger_unittest.cc",
+    "mock_trigger_manager.cc",
+    "mock_trigger_manager.h",
+    "suspicious_site_trigger_unittest.cc",
     "trigger_manager_unittest.cc",
     "trigger_throttler_unittest.cc",
   ]
   deps = [
     ":ad_sampler_trigger",
+    ":suspicious_site_trigger",
     ":trigger_throttler",
     ":triggers",
     "//base",

components/safe_browsing/triggers/ad_sampler_trigger_unittest.cc

@@ -10,7 +10,7 @@
 #include "base/test/test_simple_task_runner.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/safe_browsing/features.h"
-#include "components/safe_browsing/triggers/trigger_manager.h"
+#include "components/safe_browsing/triggers/mock_trigger_manager.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/navigation_simulator.h"
@@ -36,27 +36,6 @@ const char kAdName[] = "google_ads_iframe_1";
 const char kNonAdName[] = "foo";
 }  // namespace
 
-class MockTriggerManager : public TriggerManager {
- public:
-  MockTriggerManager() : TriggerManager(nullptr) {}
-
-  MOCK_METHOD6(StartCollectingThreatDetails,
-               bool(TriggerType trigger_type,
-                    content::WebContents* web_contents,
-                    const security_interstitials::UnsafeResource& resource,
-                    net::URLRequestContextGetter* request_context_getter,
-                    history::HistoryService* history_service,
-                    const SBErrorOptions& error_display_options));
-
-  MOCK_METHOD6(FinishCollectingThreatDetails,
-               bool(TriggerType trigger_type,
-                    content::WebContents* web_contents,
-                    const base::TimeDelta& delay,
-                    bool did_proceed,
-                    int num_visits,
-                    const SBErrorOptions& error_display_options));
-};
-
 class AdSamplerTriggerTest : public content::RenderViewHostTestHarness {
  public:
   AdSamplerTriggerTest() : task_runner_(new base::TestSimpleTaskRunner) {}

components/safe_browsing/triggers/mock_trigger_manager.cc

+// Copyright 2018 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 "components/safe_browsing/triggers/mock_trigger_manager.h"
+
+namespace safe_browsing {
+
+MockTriggerManager::MockTriggerManager() : TriggerManager(nullptr) {}
+
+MockTriggerManager::~MockTriggerManager() {}
+
+}  // namespace safe_browsing
\ No newline at end of file

components/safe_browsing/triggers/mock_trigger_manager.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_SAFE_BROWSING_TRIGGERS_MOCK_TRIGGER_MANAGER_H_
+#define COMPONENTS_SAFE_BROWSING_TRIGGERS_MOCK_TRIGGER_MANAGER_H_
+
+#include "base/macros.h"
+#include "components/safe_browsing/triggers/trigger_manager.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace safe_browsing {
+
+class MockTriggerManager : public TriggerManager {
+ public:
+  MockTriggerManager();
+  virtual ~MockTriggerManager();
+
+  MOCK_METHOD6(StartCollectingThreatDetails,
+               bool(TriggerType trigger_type,
+                    content::WebContents* web_contents,
+                    const security_interstitials::UnsafeResource& resource,
+                    net::URLRequestContextGetter* request_context_getter,
+                    history::HistoryService* history_service,
+                    const SBErrorOptions& error_display_options));
+
+  MOCK_METHOD6(FinishCollectingThreatDetails,
+               bool(TriggerType trigger_type,
+                    content::WebContents* web_contents,
+                    const base::TimeDelta& delay,
+                    bool did_proceed,
+                    int num_visits,
+                    const SBErrorOptions& error_display_options));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockTriggerManager);
+};
+
+}  // namespace safe_browsing
+
+#endif  // MOCK_TRIGGER_MANAGER_H_
\ No newline at end of file

components/safe_browsing/triggers/suspicious_site_trigger.cc

@@ -7,12 +7,25 @@
 #include "components/history/core/browser/history_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/triggers/trigger_manager.h"
+#include "components/safe_browsing/triggers/trigger_throttler.h"
+#include "components/security_interstitials/content/unsafe_resource.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/url_request/url_request_context_getter.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(safe_browsing::SuspiciousSiteTrigger);
 
 namespace safe_browsing {
+
+namespace {
+// Number of milliseconds to allow data collection to run before sending a
+// report (since this trigger runs in the background).
+const int64_t kSuspiciousSiteCollectionPeriodMilliseconds = 5000;
+}  // namespace
+
 SuspiciousSiteTrigger::SuspiciousSiteTrigger(
     content::WebContents* web_contents,
     TriggerManager* trigger_manager,
@@ -20,10 +33,15 @@ SuspiciousSiteTrigger::SuspiciousSiteTrigger(
     net::URLRequestContextGetter* request_context,
     history::HistoryService* history_service)
     : content::WebContentsObserver(web_contents),
+      finish_report_delay_ms_(kSuspiciousSiteCollectionPeriodMilliseconds),
+      current_state_(TriggerState::IDLE),
       trigger_manager_(trigger_manager),
       prefs_(prefs),
       request_context_(request_context),
-      history_service_(history_service) {}
+      history_service_(history_service),
+      task_runner_(content::BrowserThread::GetTaskRunnerForThread(
+          content::BrowserThread::UI)),
+      weak_ptr_factory_(this) {}
 
 SuspiciousSiteTrigger::~SuspiciousSiteTrigger() {}
 
@@ -42,4 +60,148 @@ void SuspiciousSiteTrigger::CreateForWebContents(
   }
 }
 
+bool SuspiciousSiteTrigger::MaybeStartReport() {
+  SBErrorOptions error_options =
+      TriggerManager::GetSBErrorDisplayOptions(*prefs_, *web_contents());
+
+  security_interstitials::UnsafeResource resource;
+  resource.threat_type = SB_THREAT_TYPE_SUSPICIOUS_SITE;
+  resource.url = web_contents()->GetURL();
+  resource.web_contents_getter = resource.GetWebContentsGetter(
+      web_contents()->GetMainFrame()->GetProcess()->GetID(),
+      web_contents()->GetMainFrame()->GetRoutingID());
+
+  if (!trigger_manager_->StartCollectingThreatDetails(
+          TriggerType::SUSPICIOUS_SITE, web_contents(), resource,
+          request_context_, history_service_, error_options)) {
+    return false;
+  }
+
+  // Call back into the trigger after a short delay, allowing the report
+  // to complete.
+  task_runner_->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&SuspiciousSiteTrigger::FinishReport,
+                     weak_ptr_factory_.GetWeakPtr()),
+      base::TimeDelta::FromMilliseconds(finish_report_delay_ms_));
+
+  return true;
+}
+
+void SuspiciousSiteTrigger::FinishReport() {
+  SBErrorOptions error_options =
+      TriggerManager::GetSBErrorDisplayOptions(*prefs_, *web_contents());
+  trigger_manager_->FinishCollectingThreatDetails(
+      TriggerType::SUSPICIOUS_SITE, web_contents(), base::TimeDelta(),
+      /*did_proceed=*/false, /*num_visits=*/0, error_options);
+}
+
+void SuspiciousSiteTrigger::DidStartLoading() {
+  switch (current_state_) {
+    case TriggerState::IDLE:
+      // Load started, move to loading state.
+      current_state_ = TriggerState::LOADING;
+      return;
+
+    case TriggerState::LOADING:
+      // No-op, still loading.
+      return;
+
+    case TriggerState::LOADING_WILL_REPORT:
+      // This happens if the user leaves the suspicious page before it
+      // finishes loading. A report can't be created in this case since the
+      // page is now gone.
+      current_state_ = TriggerState::LOADING;
+      return;
+
+    case TriggerState::REPORT_STARTED:
+      // A new page load has started while creating the current report.
+      // Finish the report immediately with whatever data has been captured
+      // so far. A report timer will have already started, but it will be
+      // ignored when it fires.
+      current_state_ = TriggerState::LOADING;
+      FinishReport();
+      return;
+  }
+}
+
+void SuspiciousSiteTrigger::DidStopLoading() {
+  switch (current_state_) {
+    case TriggerState::IDLE:
+      // No-op, load stopped and we're already idle.
+      return;
+
+    case TriggerState::LOADING:
+      // Load finished, return to Idle state.
+      current_state_ = TriggerState::IDLE;
+      return;
+
+    case TriggerState::LOADING_WILL_REPORT:
+      // Suspicious site detected mid-load and the page has now
+      // finished loading, so try starting a report now.
+      // If we fail to start a report for whatever reason, return to Idle.
+      if (MaybeStartReport()) {
+        current_state_ = TriggerState::REPORT_STARTED;
+      } else {
+        current_state_ = TriggerState::IDLE;
+      }
+      return;
+
+    case TriggerState::REPORT_STARTED:
+      // No-op. Let the report continue running.
+      return;
+  }
+}
+
+void SuspiciousSiteTrigger::SuspiciousSiteDetected() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  switch (current_state_) {
+    case TriggerState::IDLE:
+      // Suspicious site detected while idle, start a report immediately.
+      // If we fail to start a report for whatever reason, remain Idle.
+      if (MaybeStartReport()) {
+        current_state_ = TriggerState::REPORT_STARTED;
+      }
+      return;
+
+    case TriggerState::LOADING:
+      // Suspicious site detected in the middle of the load, remember this
+      // and let the page finish loading. The report will be started after
+      // the page has loaded.
+      current_state_ = TriggerState::LOADING_WILL_REPORT;
+      return;
+
+    case TriggerState::LOADING_WILL_REPORT:
+      // No-op. Current page has multiple suspicious URLs in it, remain in
+      // the LOADING_WILL_REPORT state. A report will begin when the page
+      // finishes loading.
+      return;
+
+    case TriggerState::REPORT_STARTED:
+      // No-op. The current report should capture all suspicious sites.
+      return;
+  }
+}
+
+void SuspiciousSiteTrigger::ReportDelayTimerFired() {
+  switch (current_state_) {
+    case TriggerState::IDLE:
+    case TriggerState::LOADING:
+    case TriggerState::LOADING_WILL_REPORT:
+      // Invalid, expecting to be in REPORT_STARTED state.
+      return;
+
+    case TriggerState::REPORT_STARTED:
+      // The delay timer has fired so complete the current report.
+      current_state_ = TriggerState::IDLE;
+      FinishReport();
+      return;
+  }
+}
+
+void SuspiciousSiteTrigger::SetTaskRunnerForTest(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+  task_runner_ = task_runner;
+}
+
 }  // namespace safe_browsing

components/safe_browsing/triggers/suspicious_site_trigger.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_SAFE_BROWSING_TRIGGERS_SUSPICIOUS_SITE_TRIGGER_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
@@ -40,8 +41,30 @@ class SuspiciousSiteTrigger
       net::URLRequestContextGetter* request_context,
       history::HistoryService* history_service);
 
+  // content::WebContentsObserver implementations.
+  void DidStartLoading() override;
+  void DidStopLoading() override;
+
+  // Called when a suspicious site has been detected on the tab that this
+  // trigger is running on.
+  void SuspiciousSiteDetected();
+
  private:
   friend class content::WebContentsUserData<SuspiciousSiteTrigger>;
+  friend class SuspiciousSiteTriggerTest;
+
+  // The different states the trigger could be in.
+  enum class TriggerState {
+    // Trigger is idle, page is not loading, no report requested.
+    IDLE,
+    // Page load has started, no report requested.
+    LOADING,
+    // Page load has started and a report is requested. The report will be
+    // created when the page load finishes.
+    LOADING_WILL_REPORT,
+    // A page load finished and a report for the page has started.
+    REPORT_STARTED,
+  };
 
   SuspiciousSiteTrigger(content::WebContents* web_contents,
                         TriggerManager* trigger_manager,
@@ -49,6 +72,31 @@ class SuspiciousSiteTrigger
                         net::URLRequestContextGetter* request_context,
                         history::HistoryService* history_service);
 
+  // Tries to start a report. Returns whether a report started successfully.
+  // If a report is started, a delayed callback will also begin to notify
+  // the trigger when the report should be completed and sent.
+  bool MaybeStartReport();
+
+  // Calls into the trigger manager to finish the active report and send it.
+  void FinishReport();
+
+  // Called when the report delay timer fires, indicating that the active
+  // report should be completed and sent.
+  void ReportDelayTimerFired();
+
+  // Sets a task runner to use for tests.
+  void SetTaskRunnerForTest(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+
+  // The delay (in milliseconds) to wait before finishing a report. Can be
+  // overwritten for tests.
+  int64_t finish_report_delay_ms_;
+
+  // Current state of the trigger. Used to synchronize page load events with
+  // suspicious site list hit events so that reports can be generated at the
+  // right time.
+  TriggerState current_state_;
+
   // TriggerManager gets called if this trigger detects a suspicious site and
   // wants to collect data abou tit. Not owned.
   TriggerManager* trigger_manager_;
@@ -57,6 +105,12 @@ class SuspiciousSiteTrigger
   net::URLRequestContextGetter* request_context_;
   history::HistoryService* history_service_;
 
+  // Task runner for posting delayed tasks. Normally set to the runner for the
+  // UI thread, but can be overwritten for tests.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  base::WeakPtrFactory<SuspiciousSiteTrigger> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(SuspiciousSiteTrigger);
 };
 

components/safe_browsing/triggers/suspicious_site_trigger_unittest.cc

+// Copyright 2018 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 "components/safe_browsing/triggers/suspicious_site_trigger.h"
+
+#include "base/test/test_simple_task_runner.h"
+#include "components/prefs/testing_pref_service.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
+#include "components/safe_browsing/triggers/mock_trigger_manager.h"
+#include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_renderer_host.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::NavigationSimulator;
+using content::RenderFrameHost;
+using content::RenderFrameHostTester;
+
+using testing::_;
+using testing::Return;
+
+namespace safe_browsing {
+
+namespace {
+const char kSuspiciousUrl[] = "https://suspicious.com/";
+const char kCleanUrl[] = "https://foo.com/";
+}  // namespace
+
+class SuspiciousSiteTriggerTest : public content::RenderViewHostTestHarness {
+ public:
+  SuspiciousSiteTriggerTest() : task_runner_(new base::TestSimpleTaskRunner) {}
+  ~SuspiciousSiteTriggerTest() override {}
+
+  void SetUp() override {
+    content::RenderViewHostTestHarness::SetUp();
+
+    // Enable any prefs required for the trigger to run.
+    safe_browsing::RegisterProfilePrefs(prefs_.registry());
+    prefs_.SetBoolean(prefs::kSafeBrowsingExtendedReportingOptInAllowed, true);
+    prefs_.SetBoolean(prefs::kSafeBrowsingScoutReportingEnabled, true);
+    prefs_.SetBoolean(prefs::kSafeBrowsingScoutGroupSelected, true);
+  }
+
+  void CreateTrigger() {
+    safe_browsing::SuspiciousSiteTrigger::CreateForWebContents(
+        web_contents(), &trigger_manager_, &prefs_, nullptr, nullptr);
+    safe_browsing::SuspiciousSiteTrigger* trigger =
+        safe_browsing::SuspiciousSiteTrigger::FromWebContents(web_contents());
+    // Give the trigger a test task runner that we can synchronize on.
+    trigger->SetTaskRunnerForTest(task_runner_);
+  }
+
+  // Returns the final RenderFrameHost after navigation commits.
+  RenderFrameHost* NavigateFrame(const std::string& url,
+                                 RenderFrameHost* frame) {
+    GURL gurl(url);
+    auto navigation_simulator =
+        NavigationSimulator::CreateRendererInitiated(gurl, frame);
+    navigation_simulator->Commit();
+    RenderFrameHost* final_frame_host =
+        navigation_simulator->GetFinalRenderFrameHost();
+    return final_frame_host;
+  }
+
+  // Returns the final RenderFrameHost after navigation commits.
+  RenderFrameHost* NavigateMainFrame(const std::string& url) {
+    return NavigateFrame(url, web_contents()->GetMainFrame());
+  }
+
+  // Returns the final RenderFrameHost after navigation commits.
+  RenderFrameHost* CreateAndNavigateSubFrame(const std::string& url,
+                                             RenderFrameHost* parent) {
+    RenderFrameHost* subframe =
+        RenderFrameHostTester::For(parent)->AppendChild("subframe");
+    return NavigateFrame(url, subframe);
+  }
+
+  void FinishAllNavigations() {
+    // Call the trigger's DidStopLoading event handler directly since it is not
+    // called as part of the navigating individual frames.
+    safe_browsing::SuspiciousSiteTrigger::FromWebContents(web_contents())
+        ->DidStopLoading();
+  }
+
+  void TriggerSuspiciousSite() {
+    // Notify the trigger that a suspicious site was detected.
+    safe_browsing::SuspiciousSiteTrigger::FromWebContents(web_contents())
+        ->SuspiciousSiteDetected();
+  }
+
+  void WaitForTaskRunnerIdle() {
+    task_runner_->RunUntilIdle();
+    base::RunLoop().RunUntilIdle();
+  }
+
+  MockTriggerManager* get_trigger_manager() { return &trigger_manager_; }
+
+ private:
+  TestingPrefServiceSimple prefs_;
+  MockTriggerManager trigger_manager_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+};
+
+TEST_F(SuspiciousSiteTriggerTest, RegularPageNonSuspicious) {
+  // In a normal case where there are no suspicious URLs on the page, the
+  // trigger should not fire.
+  CreateTrigger();
+
+  EXPECT_CALL(*get_trigger_manager(),
+              StartCollectingThreatDetails(_, _, _, _, _, _))
+      .Times(0);
+  EXPECT_CALL(*get_trigger_manager(),
+              FinishCollectingThreatDetails(_, _, _, _, _, _))
+      .Times(0);
+
+  RenderFrameHost* main_frame = NavigateMainFrame(kCleanUrl);
+  CreateAndNavigateSubFrame(kCleanUrl, main_frame);
+  CreateAndNavigateSubFrame(kCleanUrl, main_frame);
+  FinishAllNavigations();
+}
+
+TEST_F(SuspiciousSiteTriggerTest, SuspiciousHitDuringLoad) {
+  // When a suspicious site is detected in the middle of a page load, a report
+  // is created after the page load has finished.
+  CreateTrigger();
+
+  EXPECT_CALL(*get_trigger_manager(),
+              StartCollectingThreatDetails(_, _, _, _, _, _))
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(*get_trigger_manager(),
+              FinishCollectingThreatDetails(_, _, _, _, _, _))
+      .Times(1);
+
+  RenderFrameHost* main_frame = NavigateMainFrame(kCleanUrl);
+  CreateAndNavigateSubFrame(kSuspiciousUrl, main_frame);
+  TriggerSuspiciousSite();
+  CreateAndNavigateSubFrame(kCleanUrl, main_frame);
+  FinishAllNavigations();
+
+  WaitForTaskRunnerIdle();
+}
+
+TEST_F(SuspiciousSiteTriggerTest, SuspiciousHitAfterLoad) {
+  // When a suspicious site is detected in after a page load, a report is
+  // created immediately.
+  CreateTrigger();
+
+  EXPECT_CALL(*get_trigger_manager(),
+              StartCollectingThreatDetails(_, _, _, _, _, _))
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(*get_trigger_manager(),
+              FinishCollectingThreatDetails(_, _, _, _, _, _))
+      .Times(1);
+
+  RenderFrameHost* main_frame = NavigateMainFrame(kCleanUrl);
+  CreateAndNavigateSubFrame(kSuspiciousUrl, main_frame);
+  CreateAndNavigateSubFrame(kCleanUrl, main_frame);
+  FinishAllNavigations();
+  TriggerSuspiciousSite();
+
+  WaitForTaskRunnerIdle();
+}
+
+}  // namespace safe_browsing
\ No newline at end of file