Sends switches to the Software Reporter to enable matching data collection.

chrome/browser/safe_browsing/srt_fetcher_win.cc

Index: chrome/browser/safe_browsing/srt_fetcher_win.cc
diff --git a/chrome/browser/safe_browsing/srt_fetcher_win.cc b/chrome/browser/safe_browsing/srt_fetcher_win.cc
index d94c426cb6e7249db95e0c0f14d9b1b2d811906e..f502d0a269107424eb35d8b8a7c93f9738f4aad8 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_win.cc
@@ -13,6 +13,7 @@
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
@@ -29,16 +30,19 @@
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
+#include "chrome/browser/safe_browsing/srt_client_info_win.h"
 #include "chrome/browser/safe_browsing/srt_global_error_win.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/global_error/global_error_service.h"
 #include "chrome/browser/ui/global_error/global_error_service_factory.h"
+#include "chrome/common/pref_names.h"
 #include "components/component_updater/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/rappor/rappor_service.h"
 #include "components/variations/net/variations_http_headers.h"
+#include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
@@ -58,6 +62,9 @@ const wchar_t kCleanerSubKey[] = L"Cleaner";
 const wchar_t kEndTimeValueName[] = L"EndTime";
 const wchar_t kStartTimeValueName[] = L"StartTime";
 
+const char kExtendedSafeBrowsingEnabledSwitch[] =
+    "extended-safebrowsing-enabled";
+
 namespace {
 
 // Used to send UMA information about missing start and end time registry
@@ -115,6 +122,10 @@ const char kFoundUwsReadErrorMetricName[] =
 const char kScanTimesMetricName[] = "SoftwareReporter.UwSScanTimes";
 const char kMemoryUsedMetricName[] = "SoftwareReporter.MemoryUsed";
 
+const base::Feature kSwReporterExtendedSafeBrowsingFeature {
+  "SwReporterExtendedSafeBrowsingFeature", base::FEATURE_DISABLED_BY_DEFAULT
+};
+
 // Reports metrics about the software reporter via UMA (and sometimes Rappor).
 class UMAHistogramReporter {
  public:
@@ -354,6 +365,18 @@ void DisplaySRTPrompt(const base::FilePath& download_path) {
     global_error->ShowBubbleView(browser);
 }
 
+bool SafeBrowsingExtendedReportingEnabled() {
+  Browser* browser = chrome::FindLastActive();
+  if (!browser)
+    return false;
+  Profile* profile = browser->profile();
+  DCHECK(profile);
+  if (profile->IsOffTheRecord())

[robertshield, 2016/08/27 04:04:29]

Not sure this is correct as it will return false whenever a user pops an
incognito window to the foreground. 

Ward does something a little more complicated which seems more correct, it scans
all open profiles looking for one that has the right criteria: 

https://codesearch.chromium.org/chromium/src/chrome/browser/safe_browsing/inc...

[ftirelo, 2016/08/29 00:10:37]

Thanks for the suggestion. It's something that I was not comfortable with, but
didn't really know how to solve. 

We may want to also revisit this in other places. For example, in
https://chromium.googlesource.com/experimental/chromium/src/+/refs/wip/bajone....
I will investigate more thoroughly what happens if the current window is in
incognito mode and add a comment in the doc for the UI redesign.

+    return false;
+  return profile->GetPrefs()->GetBoolean(
+      prefs::kSafeBrowsingExtendedReportingEnabled);
+}
+
 // This function is called from a worker thread to launch the SwReporter and
 // wait for termination to collect its exit code. This task could be
 // interrupted by a shutdown at any time, so it shouldn't depend on anything
@@ -576,6 +599,8 @@ class ReporterRunner : public chrome::BrowserListObserver {
       local_state->SetInteger(prefs::kSwReporterLastExitCode, exit_code);
       local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
                             base::Time::Now().ToInternalValue());
+      local_state->SetInt64(prefs::kSwReporterLastTimeSentReport,

[alito, 2016/08/26 23:18:08]

Should this be set only if we actually did end up sending logs? Otherwise, as
is, it is going to simply have (almost) the same value as the
kSwReporterLastTimeTriggered pref.

[ftirelo, 2016/08/29 00:10:37]

Good catch!

+                            base::Time::Now().ToInternalValue());
     }
     uma.ReportRuntime(reporter_running_time);
     uma.ReportScanTimes();
@@ -639,13 +664,24 @@ class ReporterRunner : public chrome::BrowserListObserver {
       if (g_testing_delegate_)
         g_testing_delegate_->NotifyLaunchReady();
 
+      // Creating a new invocation to add switches for users who opted into
+      // extended Safe Browsing reporting. The invocation object is changed
+      // locally right before the actual process is launched before the current
+      // user status can change if this runs doesn't complete and we invoke the

[robertshield, 2016/08/27 04:04:29]

nit: s/runs/run/

[ftirelo, 2016/08/29 00:10:37]

Done.

+      // reporter again from the same ReporterRunner object.

[robertshield, 2016/08/27 04:04:29]

Actually, I'm not sure I fully follow the last sentence here: is this dealing
with the case where we run the reporter two times from a single instance of
ReporterRunner and a user changes their extended 
sb opt in-between those two times?

[ftirelo, 2016/08/29 00:10:36]

Yes. Changed wording to make the statement more precise. Please notice that the
ReporterDone callback schedules this method (TryToRun) to run a few days after
the current run. If I change the invocation for this object, I can potentially
send switches to the reporter that are not valid anymore.

+      SwReporterInvocation actual_invocation =
+          SwReporterInvocation::FromCommandLine(invocation_.command_line);
+      if (ShouldSendReporterLogs(*local_state)) {

[grt (UTC plus 2), 2016/08/28 19:44:22]

nit: omit braces

[ftirelo, 2016/08/29 00:10:37]

Done.

+        AddSwitchesForExtendedReporterUser(&actual_invocation);
+      }
+
       // It's OK to simply |PostTaskAndReplyWithResult| so that
       // |LaunchAndWaitForExit| doesn't need to access
       // |main_thread_task_runner_| since the callback is not delayed and the
       // test task runner won't need to force it.
       base::PostTaskAndReplyWithResult(
           blocking_task_runner_.get(), FROM_HERE,
-          base::Bind(&LaunchAndWaitForExit, invocation_),
+          base::Bind(&LaunchAndWaitForExit, actual_invocation),
           base::Bind(&ReporterRunner::ReporterDone, base::Unretained(this),
                      base::Time::Now(), version_));
     } else {
@@ -656,6 +692,36 @@ class ReporterRunner : public chrome::BrowserListObserver {
     }
   }
 
+  // Returns true if the experiment to send reporter logs is enabled, the user
+  // opted into Safe Browsing extended reporting, and logs have been sent at
+  // least |

[robertshield, 2016/08/27 04:04:29]

truncated comment?

[ftirelo, 2016/08/29 00:10:37]

Done.

+  bool ShouldSendReporterLogs(const PrefService& local_state) {
+    if (!base::FeatureList::IsEnabled(kSwReporterExtendedSafeBrowsingFeature))
+      return false;
+
+    if (!SafeBrowsingExtendedReportingEnabled())
+      return false;

[robertshield, 2016/08/27 04:04:29]

nit:

if (!a || !b)
  return false;

is a bit shorter

[ftirelo, 2016/08/29 00:10:36]

Done.

+
+    const base::Time last_time_sent_logs = base::Time::FromInternalValue(
+        local_state.GetInt64(prefs::kSwReporterLastTimeSentReport));
+    base::TimeDelta next_time_send_logs(

[robertshield, 2016/08/27 04:04:29]

nit: time_until_next_logs ?

[ftirelo, 2016/08/29 00:10:37]

Done.

+        last_time_sent_logs +
+        base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent) -
+        base::Time::Now());
+    return next_time_send_logs.ToInternalValue() <= 0 ||

[grt (UTC plus 2), 2016/08/28 19:44:22]

using the internal value for this comparison isn't so good. i think it's much
better to do something like:

    base::Time time_of_next_send(last_time_sent_logs +
base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent));
    base::Time now(base::Time::Now());
    return time_of_next_send <= now || last_time_sent_logs > now;

come to think of it, i would early-return the final part. i think it's more
clear:

    const baes::Time now = base::Time::Now();
    const base::Time last_time_sent_logs = base::Time::FromInternalValue(
        local_state.GetInt64(prefs::kSwReporterLastTimeSentReport));
    // Send the logs if the last send was in the future.
    if (last_time_sent_logs > now)
      return true;
    // Otherwise, send them if the interval has passed.
    return (last_time_sent_logs +
	    base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent)) <= now;

[ftirelo, 2016/08/29 00:10:37]

That's a good idea. Rewritten using both suggestions (aux var for next_time and
early return on future last time sent).

+        // Also make sure the kSwReporterLastTimeSentReport value is not set in
+        // the future.
+        last_time_sent_logs > base::Time::Now();
+  }
+
+  void AddSwitchesForExtendedReporterUser(SwReporterInvocation* invocation) {
+    invocation->command_line.AppendSwitch(kExtendedSafeBrowsingEnabledSwitch);
+    invocation->command_line.AppendSwitchASCII(
+        kChromeVersionSwitch, version_info::GetVersionNumber());
+    invocation->command_line.AppendSwitchASCII(

[grt (UTC plus 2), 2016/08/28 19:44:22]

nit: avoid some string conversions by using AppendSwitchNative here and
IntToString16 below

[ftirelo, 2016/08/29 00:10:37]

Done.

+        kChromeChannelSwitch, base::IntToString(ChannelAsInt()));
+  }
+
   bool first_run_ = true;
   SwReporterInvocation invocation_;
   base::Version version_;