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

chrome/browser/safe_browsing/srt_fetcher_win.cc

 // Copyright 2015 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 "chrome/browser/safe_browsing/srt_fetcher_win.h"
 
 #include <stdint.h>
 
+#include <algorithm>
 #include <memory>
 #include <vector>
 
 #include "base/bind.h"
 #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"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/time/time.h"
 #include "base/win/registry.h"
 #include "chrome/browser/browser_process.h"
 #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"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
 
 using content::BrowserThread;
 
 namespace safe_browsing {
 
 const wchar_t kSoftwareRemovalToolRegistryKey[] =
     L"Software\\Google\\Software Removal Tool";
 
 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
 // values for the reporter. Replicated in the histograms.xml file, so the order
 // MUST NOT CHANGE.
 enum SwReporterRunningTimeRegistryError {
   REPORTER_RUNNING_TIME_ERROR_NO_ERROR = 0,
   REPORTER_RUNNING_TIME_ERROR_REGISTRY_KEY_INVALID = 1,
   REPORTER_RUNNING_TIME_ERROR_MISSING_START_TIME = 2,
   REPORTER_RUNNING_TIME_ERROR_MISSING_END_TIME = 3,
@@ skipping 37 matching lines @@
 const wchar_t kScanTimesSubKey[] = L"ScanTimes";
 const wchar_t kFoundUwsValueName[] = L"FoundUws";
 const wchar_t kMemoryUsedValueName[] = L"MemoryUsed";
 
 const char kFoundUwsMetricName[] = "SoftwareReporter.FoundUwS";
 const char kFoundUwsReadErrorMetricName[] =
     "SoftwareReporter.FoundUwSReadError";
 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:
   // Reports the software reporter tool's version via UMA.
   void ReportVersion(const base::Version& version) const {
     DCHECK(!version.components().empty());
     // The minor version is the 2nd last component of the version,
     // or just the first component if there is only 1.
     uint32_t minor_version = 0;
     if (version.components().size() > 1)
@@ skipping 219 matching lines @@
       if (error->GetBubbleView()) {
         show_bubble = false;
         break;
       }
     }
   }
   if (show_bubble)
     global_error->ShowBubbleView(browser);
 }
 
+// Returns true if there is a profile that is not in incognito mode and the user
+// has opted into Safe Browsing extended reporting.
+bool SafeBrowsingExtendedReportingEnabled() {
+  BrowserList* browser_list = BrowserList::GetInstance();
+  return std::any_of(browser_list->begin_last_active(),
+                     browser_list->end_last_active(),
+                     [](const Browser* browser) {
+                       const Profile* profile = browser->profile();

[csharp, 2016/08/30 13:39:08]

What about converting this lambda to a helper
function,BrowserIsSafeBrowsingExtended? (or something)

(It seems just a little big for a lambda)

[ftirelo, 2016/08/30 21:21:48]

Done.

+                       return profile && !profile->IsOffTheRecord() &&
+                              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
 // external that could be shut down beforehand.
 int LaunchAndWaitForExit(const SwReporterInvocation& invocation) {
   if (g_testing_delegate_)
     return g_testing_delegate_->LaunchReporter(invocation);
   base::Process reporter_process =
       base::LaunchProcess(invocation.command_line, base::LaunchOptions());
   // This exit code is used to identify that a reporter run didn't happen, so
@@ skipping 175 matching lines @@
     DCHECK(browser);
     MaybeFetchSRT(browser, version_);
     BrowserList::RemoveObserver(this);
   }
 
   // This method is called on the UI thread when the reporter run has completed.
   // This is run as a task posted from an interruptible worker thread so should
   // be resilient to unexpected shutdown.
   void ReporterDone(const base::Time& reporter_start_time,
                     const base::Version& version,
+                    bool logging_enabled,
                     int exit_code) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
     if (g_testing_delegate_)
       g_testing_delegate_->NotifyReporterDone();
 
     base::TimeDelta reporter_running_time =
         base::Time::Now() - reporter_start_time;
     // Don't continue when the reporter process failed to launch, but still try
     // again after the regular delay. It's not worth retrying earlier, risking
     // running too often if it always fails, since not many users fail here.
     main_thread_task_runner_->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ReporterRunner::TryToRun, base::Unretained(this)),
         base::TimeDelta::FromDays(days_between_reporter_runs_));
     if (exit_code == kReporterFailureExitCode)
       return;
 
     UMAHistogramReporter uma;
     uma.ReportVersion(version);
     uma.ReportExitCode(exit_code);
     uma.ReportFoundUwS();
     PrefService* local_state = g_browser_process->local_state();
     if (local_state) {
       local_state->SetInteger(prefs::kSwReporterLastExitCode, exit_code);
       local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
                             base::Time::Now().ToInternalValue());
+      if (logging_enabled) {
+        local_state->SetInt64(prefs::kSwReporterLastTimeSentReport,

[csharp, 2016/08/30 13:39:08]

I wonder if this should be set right before the reporter is called. Setting it
here could result in a user uploading logs but Chrome stopping before this value
is set, so they'd run it again too quickly.

Although if we set it before the reporter runs some times the reporter would
stop before it uploads, so we'd miss some cases. But missing some cases seems
like it might be better than getting too many. WDYT?

[ftirelo, 2016/08/30 21:21:49]

Done.

+                              base::Time::Now().ToInternalValue());
+      }
     }
     uma.ReportRuntime(reporter_running_time);
     uma.ReportScanTimes();
     uma.ReportMemoryUsage();
 
     if (!IsInSRTPromptFieldTrialGroups()) {
       // Knowing about disabled field trial is more important than reporter not
       // finding anything to remove, so check this case first.
       RecordReporterStepHistogram(SW_REPORTER_NO_PROMPT_FIELD_TRIAL);
       return;
@@ skipping 43 matching lines @@
         last_time_triggered +
         base::TimeDelta::FromDays(days_between_reporter_runs_) -
         base::Time::Now());
     if (next_trigger_delay.ToInternalValue() <= 0 ||
         // Also make sure the kSwReporterLastTimeTriggered value is not set in
         // the future.
         last_time_triggered > base::Time::Now()) {
       if (g_testing_delegate_)
         g_testing_delegate_->NotifyLaunchReady();
 
+      // Creating a new invocation to add switches for users who opted into

[csharp, 2016/08/30 13:39:08]

Could you add some tests for this?

[ftirelo, 2016/08/30 21:21:49]

Done.

+      // extended Safe Browsing reporting. The invocation object is changed
+      // locally right before the actual process is launched because user status
+      // can between this and the next run for this ReporterRunner object. For
+      // example, the ReporterDone() callback schedules the next run for a few
+      // days later, and the user might have changed settings in the meantime.
+      SwReporterInvocation actual_invocation =

[csharp, 2016/08/30 13:39:08]

I think you want to create a copy constructor and use it here, since this will
drop all the fields but the command line.

[ftirelo, 2016/08/30 21:21:49]

Done.

+          SwReporterInvocation::FromCommandLine(invocation_.command_line);
+      bool logging_enabled = ShouldSendReporterLogs(*local_state);
+      if (logging_enabled)
+        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_));
+                     base::Time::Now(), version_, logging_enabled));
     } else {
       main_thread_task_runner_->PostDelayedTask(
           FROM_HERE,
           base::Bind(&ReporterRunner::TryToRun, base::Unretained(this)),
           next_trigger_delay);
     }
   }
 
+  // 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 |kSwReporterLastTimeSentReport| days ago.
+  bool ShouldSendReporterLogs(const PrefService& local_state) {
+    if (!base::FeatureList::IsEnabled(kSwReporterExtendedSafeBrowsingFeature) ||
+        !SafeBrowsingExtendedReportingEnabled()) {
+      return false;
+    }
+
+    const base::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.

[csharp, 2016/08/30 13:39:08]

nit: Could you explain why we care about this a bit?

[ftirelo, 2016/08/30 21:21:49]

Done.

+    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;
+  }
+
+  void AddSwitchesForExtendedReporterUser(SwReporterInvocation* invocation) {
+    invocation->command_line.AppendSwitch(kExtendedSafeBrowsingEnabledSwitch);
+    invocation->command_line.AppendSwitchASCII(
+        kChromeVersionSwitch, version_info::GetVersionNumber());
+    invocation->command_line.AppendSwitchNative(
+        kChromeChannelSwitch, base::IntToString16(ChannelAsInt()));
+  }
+
   bool first_run_ = true;
   SwReporterInvocation invocation_;
   base::Version version_;
   scoped_refptr<base::TaskRunner> main_thread_task_runner_;
   scoped_refptr<base::TaskRunner> blocking_task_runner_;
 
   // This value is used to identify how long to wait before starting a new run
   // of the reporter. It's initialized with the default value and may be changed
   // to a different value when a prompt is pending and the reporter should be
   // run before adding the global error to the Chrome menu.
@@ skipping 54 matching lines @@
                                     KEY_QUERY_VALUE);
 
   return srt_cleaner_key.Valid() && srt_cleaner_key.GetValueCount() > 0;
 }
 
 void SetSwReporterTestingDelegate(SwReporterTestingDelegate* delegate) {
   g_testing_delegate_ = delegate;
 }
 
 }  // namespace safe_browsing