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

chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h

Index: chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
diff --git a/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h b/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
index 94064c76f1ce2231af524c3a18e0e94ef15d6949..42afe8383368f2c37bcc7e8dc9aebd5e933a5099 100644
--- a/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
+++ b/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
@@ -5,16 +5,19 @@
 #ifndef CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_OFF_DOMAIN_INCLUSION_DETECTOR_H_
 #define CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_OFF_DOMAIN_INCLUSION_DETECTOR_H_
 
-#include "base/callback.h"
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/task/cancelable_task_tracker.h"
 #include "content/public/common/resource_type.h"
 
+class Profile;
 class SafeBrowsingDatabaseManager;
 
 namespace base {
 class SingleThreadTaskRunner;
+class Time;

[grt (UTC plus 2), 2015/01/21 01:58:24]

line 103 passes a base::Time by value, so i don't think a forward decl is
sufficient.

[gab, 2015/01/23 21:35:33]

Hmm I guess that's true since the caller needs to know how to copy it. Good
catch, done.

 }
 
 namespace net {
@@ -28,16 +31,26 @@ class OffDomainInclusionDetector {
  public:
   enum class AnalysisEvent {
     NO_EVENT,
-    EMPTY_MAIN_FRAME_URL,
-    INVALID_MAIN_FRAME_URL,
+    ABORT_EMPTY_MAIN_FRAME_URL,
+    ABORT_INVALID_MAIN_FRAME_URL,
+    ABORT_NO_PROFILE,
+    ABORT_INCOGNITO,
+    ABORT_NO_HISTORY_SERVICE,
+    ABORT_HISTORY_LOOKUP_FAILED,
     OFF_DOMAIN_INCLUSION_WHITELISTED,
+    OFF_DOMAIN_INCLUSION_IN_HISTORY,
     OFF_DOMAIN_INCLUSION_SUSPICIOUS,
   };
 
   // TODO(gab): Hook the OffDomainInclusionDetector to the
   // IncidentReportingService and use an AddIncidentCallback instead of this
   // custom callback type.
-  typedef base::Callback<void(AnalysisEvent event)> ReportAnalysisEventCallback;
+  using ReportAnalysisEventCallback = base::Callback<void(AnalysisEvent event)>;
+
+  // Returns the Profile* corresponding to |render_process_id| and
+  // |render_frame_id| or NULL if there is no such match.
+  using GetProfileFromRenderFrameIdCallback =
+      base::Callback<Profile*(int render_process_id, int render_frame_id)>;
 
   // Constructs an OffDomainInclusionDetector which will use |database_manager|
   // over the course of its lifetime. The OffDomainInclusionDetector will use
@@ -46,11 +59,14 @@ class OffDomainInclusionDetector {
       const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager);
 
   // Constructs an OffDomainInclusionDetector as above with an additional
-  // ReportAnalysisEventCallback to get feedback from detection events, used
-  // only in tests.
+  // ReportAnalysisEventCallback to get feedback from detection events and
+  // GetProfileFromRenderFrameIdCallback to control which profile is used when
+  // looking up history, used only in tests.
   OffDomainInclusionDetector(
       const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager,
-      const ReportAnalysisEventCallback& report_analysis_event_callback);
+      const ReportAnalysisEventCallback& report_analysis_event_callback,
+      const GetProfileFromRenderFrameIdCallback&

[grt (UTC plus 2), 2015/01/21 01:58:24]

it looks like this callback is here so that tests can provide their own
convenience implementation. calling through a protected virtual method (that is
overridden in the test) has considerably less overhead. i recall reading on
chromium-dev that running a callback is relatively expensive on android in
particular. i think it's simpler to use a virtual method, unless there some
subtlety in the use of the callback that i'm missing.

[gab, 2015/01/23 21:35:33]

Done.

+          get_profile_from_render_frame_id_callback);
 
   ~OffDomainInclusionDetector();
 
@@ -70,12 +86,21 @@ class OffDomainInclusionDetector {
   // chain of checks on the main thread as described below:
   //   1) Check if it is indeed an off-domain inclusion and, if so: check the
   //      inclusion against the safe browsing inclusion whitelist.
-  //   2) Obtain the whitelist result and report accordingly.
+  //   2) Upon receiving the whitelist result, either: stop if it's positive or
+  //      query the HistoryService if it's not.
+  //   3) Upon receiving the HistoryService's result: report accordingly.
   void BeginAnalysis(
       scoped_ptr<OffDomainInclusionInfo> off_domain_inclusion_info);
   void ContinueAnalysisOnWhitelistResult(
       scoped_ptr<const OffDomainInclusionInfo> off_domain_inclusion_info,
       bool request_url_is_on_inclusion_whitelist);
+  void ContinueAnalysisWithHistoryCheck(
+      scoped_ptr<const OffDomainInclusionInfo> off_domain_inclusion_info);
+  void ContinueAnalysisOnHistoryResult(
+      scoped_ptr<const OffDomainInclusionInfo> off_domain_inclusion_info,
+      bool success,
+      int num_visits,
+      base::Time first_visit_time);
 
   // Reports the result of an off-domain inclusion analysis via UMA (as well
   // as via |report_analysis_event_callback_| if it is set). May be called from
@@ -87,6 +112,12 @@ class OffDomainInclusionDetector {
   scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
 
   const ReportAnalysisEventCallback report_analysis_event_callback_;
+  const GetProfileFromRenderFrameIdCallback
+      get_profile_from_render_frame_id_callback_;
+
+  // Tracks pending tasks posted to the HistoryService and cancels them if this
+  // class is destroyed before they complete.
+  base::CancelableTaskTracker history_task_tracker_;
 
   // This class may post tasks to its main thread (construction thread) via
   // |main_thread_task_runner_|, such tasks should be weakly bound via WeakPtrs