Initial script request detector.

chrome/browser/safe_browsing/incident_reporting/script_request_detector.h

+// Copyright 2014 The Chromium Authors. All rights reserved.

[grt (UTC plus 2), 2015/01/28 14:49:05]

So this is the new year? I don't feel any different.

[robertshield, 2015/01/28 22:20:32]

I find your assumption that I too am bound to the Gregorian calendar
inconsiderate.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_SCRIPT_REQUEST_DETECTOR_H_
+#define CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_SCRIPT_REQUEST_DETECTOR_H_
+
+#include "base/callback.h"

[grt (UTC plus 2), 2015/01/28 14:49:05]

unused?

[robertshield, 2015/01/28 22:20:32]

Done.

+#include "base/containers/hash_tables.h"
+#include "base/macros.h"
+#include "chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h"
+#include "chrome/common/safe_browsing/csd.pb.h"

[grt (UTC plus 2), 2015/01/28 14:49:05]

replace this with a forward decl of
ClientIncidentReport_IncidentData_ScriptRequestIncident

[robertshield, 2015/01/28 22:20:33]

Done.

+#include "content/public/common/resource_type.h"

[grt (UTC plus 2), 2015/01/28 14:49:05]

unused?

[robertshield, 2015/01/28 22:20:33]

Done.

+
+namespace net {
+class URLRequest;
+}
+
+namespace safe_browsing {
+
+// Observes network requests and reports suspicious activity.
+class ScriptRequestDetector {
+ public:
+  // |incident_reporting_service| MUST outlive this class.
+  explicit ScriptRequestDetector(
+      IncidentReportingService* incident_reporting_service);

[grt (UTC plus 2), 2015/01/28 14:49:05]

Passing in a ptr to the service defeats the whole purpose of the
AddIncidentCallback. I'll have to think about this a bit.

[robertshield, 2015/01/28 22:20:33]

Agreed, at the same time with the current API this is the only way that comes to
mind to do it.

For musing: 
AddIncidentCallbacks currently require a Profile to conjure up and I don't have
one until the request comes in. Perhaps a callback that also took a profile
parameter could be handed out?

[grt (UTC plus 2), 2015/01/30 16:15:23]

The more I think about it, the less comfortable I am with this. I'll get started
right away on allowing the caller to pass the Profile in at the time of running
the callback. It looks like it won't be at all intrusive. Please address other
comments, and maybe I'll finish before you. I'll reconsider my position if it
looks hairy.

[robertshield, 2015/02/01 04:25:57]

Sure, happy to merge either way, let me know.

[grt (UTC plus 2), 2015/02/02 18:45:22]

The latest work-in-progress is out for review here:
https://codereview.chromium.org/845663004/.

[robertshield, 2015/02/06 02:47:47]

Rebased on top of the latest hotness.

+  virtual ~ScriptRequestDetector();
+
+  // Analyzes the |request| and triggers an incident report on suspicious
+  // script inclusion.
+  void OnResourceRequest(const net::URLRequest* request);
+
+  // Add a custom script hash. Used only for testing.
+  void AddScriptHashForTesting(const std::string& raw_hash);

[grt (UTC plus 2), 2015/01/28 14:49:05]

unused

[robertshield, 2015/01/28 22:20:32]

Done.

[grt (UTC plus 2), 2015/01/30 16:15:23]

Hard to kill, it seems.

[robertshield, 2015/02/01 04:26:00]

Huh, I stabbed it repeatedly this time.

+
+  // Add a custom script hash. Used only for testing.
+  bool ContainsScriptHashForTesting(const std::string& raw_hash);

[grt (UTC plus 2), 2015/01/28 14:49:05]

unused

[robertshield, 2015/01/28 22:20:33]

Done.

[grt (UTC plus 2), 2015/01/30 16:15:23]

I don't think this word means what you think it means.

[robertshield, 2015/02/01 04:25:57]

It would seem I fell victim to one of the classic blunders.

+
+ protected:
+  // Testing hook.
+  virtual bool AllowNullProfileForTesting();

[grt (UTC plus 2), 2015/01/28 14:49:05]

interesting. gab@ chose to do GetProfileForRenderProcessId via virtual dispatch
so that tests could make it return the proper testing profile. i wonder if one
approach is better/worse than the other.

[robertshield, 2015/01/28 22:20:32]

Interesting, I don't believe one is better than the other. I did the other thing
first, but then didn't like the amount of code that was overridden for testing.
This is an explicit testing hook, which I felt was cleaner. 

[grt (UTC plus 2), 2015/01/30 16:15:23]

How do you feel about getting rid of virtual dispatch and just having a bool
member that is modified by a protected setter (which can be made public in a
subclass-for-test)?

[robertshield, 2015/02/01 04:25:59]

Equally ok. I am chronically unopinionated about such things. Doing as you
suggest.

+
+ private:
+  void InitializeScriptSet();
+
+  void ReportIncidentOnUIThread(
+      int render_process_id,
+      scoped_ptr<ClientIncidentReport_IncidentData_ScriptRequestIncident>
+          incident_data);
+
+  IncidentReportingService* incident_reporting_service_;
+  base::hash_set<std::string> script_set_;
+
+  base::WeakPtrFactory<ScriptRequestDetector> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScriptRequestDetector);
+};
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_SCRIPT_REQUEST_DETECTOR_H_