Adds histograms for tracking Software Reporter logs uploads in SRT Fetcher.

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>
@@ skipping 38 matching lines @@
 #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 {
 
+// TODO(b/647763) Change the registry key to properly handle cases when the user
+// runs Google Chrome stable alongside Google Chrome SxS.
 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";
@@ skipping 36 matching lines @@
   SW_REPORTER_NO_LOCAL_STATE = 9,
   SW_REPORTER_NO_PROMPT_NEEDED = 10,
   SW_REPORTER_NO_PROMPT_FIELD_TRIAL = 11,
   SW_REPORTER_ALREADY_PROMPTED = 12,
   SW_REPORTER_RAN_DAILY = 13,
   SW_REPORTER_ADDED_TO_MENU = 14,
 
   SW_REPORTER_MAX,
 };
 
+// Used to send UMA information showing whether uploading of Software Reporter
+// logs is enabled, or the reason why not.
+// Replicated in the histograms.xml file, so the order MUST NOT CHANGE.
+enum SwReporterLogsUploadsEnabled {
+  REPORTER_LOGS_UPLOADS_ENABLED = 0,
+  REPORTER_LOGS_UPLOADS_SBER_DISABLED = 1,
+  REPORTER_LOGS_UPLOADS_RECENTLY_SENT_LOGS = 2,
+  REPORTER_LOGS_UPLOADS_MAX,
+};
+
 const char kRunningTimeErrorMetricName[] =
     "SoftwareReporter.RunningTimeRegistryError";
 
 SwReporterTestingDelegate* g_testing_delegate_ = nullptr;
 
 const wchar_t kScanTimesSubKey[] = L"ScanTimes";
 const wchar_t kFoundUwsValueName[] = L"FoundUws";
 const wchar_t kMemoryUsedValueName[] = L"MemoryUsed";
+const wchar_t kLogsUploadResultValueName[] = L"LogsUploadResult";
 
 const char kFoundUwsMetricName[] = "SoftwareReporter.FoundUwS";
 const char kFoundUwsReadErrorMetricName[] =
     "SoftwareReporter.FoundUwSReadError";
 const char kScanTimesMetricName[] = "SoftwareReporter.UwSScanTimes";
 const char kMemoryUsedMetricName[] = "SoftwareReporter.MemoryUsed";
+const char kLogsUploadEnabledMetricName[] =
+    "SoftwareReporter.LogsUploadEnabled";
+const char kLogsUploadResultMetricName[] = "SoftwareReporter.LogsUploadResult";
+
+// The max value for histogram SoftwareReporter.LogsUploadResult, which is used
+// to send UMA information about the result of Software Reporter's attempt to
+// upload logs, when logs are enabled. This value must be consistent with the
+// SoftwareReporterLogsUploadResult enum defined in the histograms.xml file.
+const int kSwReporterLogsUploadResultMax = 7;
 
 // Reports metrics about the software reporter via UMA (and sometimes Rappor).
 class UMAHistogramReporter {
  public:
   UMAHistogramReporter() : UMAHistogramReporter(std::string()) {}
 
   explicit UMAHistogramReporter(const std::string& suffix)
       : suffix_(suffix),
         registry_key_(suffix.empty() ? kSoftwareRemovalToolRegistryKey
                                      : base::StringPrintf(
@@ skipping 29 matching lines @@
     }
     RecordSparseHistogram("SoftwareReporter.MajorVersion", major_version);
   }
 
   void ReportExitCode(int exit_code) const {
     RecordSparseHistogram("SoftwareReporter.ExitCode", exit_code);
   }
 
   // Reports UwS found by the software reporter tool via UMA and RAPPOR.
   void ReportFoundUwS(bool use_rappor) const {
-    base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
-                                   KEY_QUERY_VALUE | KEY_SET_VALUE);
+    base::win::RegKey reporter_key;
     std::vector<base::string16> found_uws_strings;
-    if (reporter_key.Valid() &&
-        reporter_key.ReadValues(kFoundUwsValueName, &found_uws_strings) ==
+    if (reporter_key.Open(HKEY_CURRENT_USER, registry_key_.c_str(),
+                          KEY_QUERY_VALUE | KEY_SET_VALUE) != ERROR_SUCCESS ||
+        reporter_key.ReadValues(kFoundUwsValueName, &found_uws_strings) !=
             ERROR_SUCCESS) {
-      rappor::RapporService* rappor_service = nullptr;
-      if (use_rappor)
-        rappor_service = g_browser_process->rappor_service();
+      return;
+    }
 
-      bool parse_error = false;
-      for (const base::string16& uws_string : found_uws_strings) {
-        // All UwS ids are expected to be integers.
-        uint32_t uws_id = 0;
-        if (base::StringToUint(uws_string, &uws_id)) {
-          RecordSparseHistogram(kFoundUwsMetricName, uws_id);
-          if (rappor_service) {
-            rappor_service->RecordSample(kFoundUwsMetricName,
-                                         rappor::COARSE_RAPPOR_TYPE,
-                                         base::UTF16ToUTF8(uws_string));
-          }
-        } else {
-          parse_error = true;
+    rappor::RapporService* rappor_service = nullptr;
+    if (use_rappor)
+      rappor_service = g_browser_process->rappor_service();
+
+    bool parse_error = false;
+    for (const base::string16& uws_string : found_uws_strings) {
+      // All UwS ids are expected to be integers.
+      uint32_t uws_id = 0;
+      if (base::StringToUint(uws_string, &uws_id)) {
+        RecordSparseHistogram(kFoundUwsMetricName, uws_id);
+        if (rappor_service) {
+          rappor_service->RecordSample(kFoundUwsMetricName,
+                                       rappor::COARSE_RAPPOR_TYPE,
+                                       base::UTF16ToUTF8(uws_string));
         }
+      } else {
+        parse_error = true;
       }
 
       // Clean up the old value.
       reporter_key.DeleteValue(kFoundUwsValueName);
 
       RecordBooleanHistogram(kFoundUwsReadErrorMetricName, parse_error);
     }
   }
 
   // Reports to UMA the memory usage of the software reporter tool as reported
   // by the tool itself in the Windows registry.
   void ReportMemoryUsage() const {
-    base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
-                                   KEY_QUERY_VALUE | KEY_SET_VALUE);
+    base::win::RegKey reporter_key;
     DWORD memory_used = 0;
-    if (reporter_key.Valid() &&
-        reporter_key.ReadValueDW(kMemoryUsedValueName, &memory_used) ==
+    if (reporter_key.Open(HKEY_CURRENT_USER, registry_key_.c_str(),
+                          KEY_QUERY_VALUE | KEY_SET_VALUE) != ERROR_SUCCESS ||
+        reporter_key.ReadValueDW(kMemoryUsedValueName, &memory_used) !=
             ERROR_SUCCESS) {
-      RecordMemoryKBHistogram(kMemoryUsedMetricName, memory_used);
-      reporter_key.DeleteValue(kMemoryUsedValueName);
+      return;
     }
+    RecordMemoryKBHistogram(kMemoryUsedMetricName, memory_used);
+    reporter_key.DeleteValue(kMemoryUsedValueName);
   }
 
   // Reports the SwReporter run time with UMA both as reported by the tool via
   // the registry and as measured by |ReporterRunner|.
   void ReportRuntime(const base::TimeDelta& reporter_running_time) const {
     RecordLongTimesHistogram("SoftwareReporter.RunningTimeAccordingToChrome",
                              reporter_running_time);
 
-    // TODO(b/641081): This should only have KEY_QUERY_VALUE and KEY_SET_VALUE,
-    // and use Open to avoid creating the key if it doesn't already exist.
-    base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
-                                   KEY_ALL_ACCESS);
-    if (!reporter_key.Valid()) {
+    // TODO(b/641081): This should only have KEY_QUERY_VALUE and KEY_SET_VALUE.
+    base::win::RegKey reporter_key;
+    if (reporter_key.Open(HKEY_CURRENT_USER, registry_key_.c_str(),
+                          KEY_ALL_ACCESS) != ERROR_SUCCESS) {
       RecordEnumerationHistogram(
           kRunningTimeErrorMetricName,
           REPORTER_RUNNING_TIME_ERROR_REGISTRY_KEY_INVALID,
           REPORTER_RUNNING_TIME_ERROR_MAX);
       return;
     }
 
     bool has_start_time = false;
     int64_t start_time_value = 0;
     if (reporter_key.HasValue(kStartTimeValueName) &&
@@ skipping 34 matching lines @@
       RecordEnumerationHistogram(kRunningTimeErrorMetricName,
                                  REPORTER_RUNNING_TIME_ERROR_MISSING_END_TIME,
                                  REPORTER_RUNNING_TIME_ERROR_MAX);
     }
   }
 
   // Reports the UwS scan times of the software reporter tool via UMA.
   void ReportScanTimes() const {
     base::string16 scan_times_key_path = base::StringPrintf(
         L"%ls\\%ls", registry_key_.c_str(), kScanTimesSubKey);
-    // TODO(b/641081): This should only have KEY_QUERY_VALUE and KEY_SET_VALUE,
-    // and use Open to avoid creating the key if it doesn't already exist.
-    base::win::RegKey scan_times_key(
-        HKEY_CURRENT_USER, scan_times_key_path.c_str(), KEY_ALL_ACCESS);
-    if (!scan_times_key.Valid())
+    // TODO(b/641081): This should only have KEY_QUERY_VALUE and KEY_SET_VALUE.
+    base::win::RegKey scan_times_key;
+    if (scan_times_key.Open(HKEY_CURRENT_USER, scan_times_key_path.c_str(),
+                            KEY_ALL_ACCESS) != ERROR_SUCCESS) {
       return;
+    }
 
     base::string16 value_name;
     int uws_id = 0;
     int64_t raw_scan_time = 0;
     int num_scan_times = scan_times_key.GetValueCount();
     for (int i = 0; i < num_scan_times; ++i) {
       if (scan_times_key.GetValueNameAt(i, &value_name) == ERROR_SUCCESS &&
           base::StringToInt(value_name, &uws_id) &&
           scan_times_key.ReadInt64(value_name.c_str(), &raw_scan_time) ==
               ERROR_SUCCESS) {
         base::TimeDelta scan_time =
             base::TimeDelta::FromInternalValue(raw_scan_time);
         // We report the number of seconds plus one because it can take less
         // than one second to scan some UwS and the count passed to |AddCount|
         // must be at least one.
         RecordSparseHistogramCount(kScanTimesMetricName, uws_id,
                                    scan_time.InSeconds() + 1);
       }
     }
     // Clean up by deleting the scan times key, which is a subkey of the main
     // reporter key.
     scan_times_key.Close();
-    base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
-                                   KEY_ENUMERATE_SUB_KEYS);
-    if (reporter_key.Valid())
+    base::win::RegKey reporter_key;
+    if (reporter_key.Open(HKEY_CURRENT_USER, registry_key_.c_str(),
+                          KEY_ENUMERATE_SUB_KEYS) == ERROR_SUCCESS) {
       reporter_key.DeleteKey(kScanTimesSubKey);
+    }
   }
 
   void RecordReporterStep(SwReporterUmaValue value) {
     RecordEnumerationHistogram("SoftwareReporter.Step", value, SW_REPORTER_MAX);
   }
 
+  void RecordLogsUploadEnabled(SwReporterLogsUploadsEnabled value) {
+    RecordEnumerationHistogram(kLogsUploadEnabledMetricName, value,
+                               REPORTER_LOGS_UPLOADS_MAX);
+  }
+
+  void RecordLogsUploadResult() {
+    base::win::RegKey reporter_key;
+    DWORD logs_upload_result = 0;
+    if (reporter_key.Open(HKEY_CURRENT_USER, registry_key_.c_str(),
+                          KEY_QUERY_VALUE | KEY_SET_VALUE) != ERROR_SUCCESS ||
+        reporter_key.ReadValueDW(kLogsUploadResultValueName,
+                                 &logs_upload_result) != ERROR_SUCCESS) {
+      return;
+    }
+

[csharp, 2016/09/20 14:29:54]

Before recording logs_upload_result, could you force it to have a value we
recognize? Some of the Finch dashboards don't like the ExitCode histogram
because it report values outside of the expected bounds

(Although I'm not quite sure what value an out of bound value should be
converted to)

[csharp, 2016/09/20 15:19:28]

Maybe limit it to 30 to allow possible values to grow without needing to change
Chrome.

And maybe instead of converting to a different value, just use another histogram
to report the error

[ftirelo, 2016/09/20 16:41:42]

Done.

[ftirelo, 2016/09/20 16:41:42]

Done.

+    RecordEnumerationHistogram(kLogsUploadResultMetricName,
+                               static_cast<Sample>(logs_upload_result),
+                               kSwReporterLogsUploadResultMax);
+    reporter_key.DeleteValue(kLogsUploadResultValueName);
+  }
+
  private:
   using Sample = base::HistogramBase::Sample;
 
   static constexpr base::HistogramBase::Flags kUmaHistogramFlag =
       base::HistogramBase::kUmaTargetedHistogramFlag;
 
   // Helper functions to record histograms with an optional suffix added to the
   // histogram name. The UMA_HISTOGRAM macros can't be used because they
   // require a constant string.
 
@@ skipping 270 matching lines @@
   prefs->SetString(prefs::kSwReporterPromptVersion,
                    reporter_version.GetString());
 
   // Download the SRT.
   RecordReporterStepHistogram(SW_REPORTER_DOWNLOAD_START);
 
   // All the work happens in the self-deleting class below.
   new SRTFetcher(profile);
 }
 
+bool BehaviourIsEnabled(SwReporterInvocation::Behaviours behaviours,

[csharp, 2016/09/20 14:29:54]

What about having this as a member of SwReporterInvocation? Then you implicitly
always have the first param

[ftirelo, 2016/09/20 16:41:42]

Done.

+                        SwReporterInvocation::Behaviours intended_behaviour) {
+  return (behaviours & intended_behaviour) != 0;
+}
+
 // This class tries to run a queue of reporters and react to their exit codes.
 // It schedules subsequent runs of the queue as needed, or retries as soon as a
 // browser is available when none is on first try.
 class ReporterRunner : public chrome::BrowserListObserver {
  public:
   // Registers |invocations| to run next time |TryToRun| is scheduled. (And if
   // it's not already scheduled, call it now.)
   static void ScheduleInvocations(
       const SwReporterQueue& invocations,
       const base::Version& version,
@@ skipping 46 matching lines @@
     if (g_testing_delegate_)
       g_testing_delegate_->NotifyLaunchReady();
 
     // Add switches for users who opted into extended Safe Browsing reporting.
     // The invocation object is changed locally right before the actual process
     // is launched because user status can change 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.
     PrefService* local_state = g_browser_process->local_state();
-    if (next_invocation.flags & SwReporterInvocation::FLAG_SEND_REPORTER_LOGS &&
+    if (BehaviourIsEnabled(
+            next_invocation.supported_behaviours,
+            SwReporterInvocation::BEHAVIOUR_SEND_REPORTER_LOGS) &&
         local_state && ShouldSendReporterLogs(*local_state)) {
+      next_invocation.logs_upload_enabled = true;
       AddSwitchesForExtendedReporterUser(&next_invocation);
       // Set the local state value before the first attempt to run the
       // reporter, because we only want to upload logs once in the window
       // defined by |kDaysBetweenReporterLogsSent|. If we set with other local
       // state values after the reporter runs, we could send logs again too
       // quickly (for example, if Chrome stops before the reporter finishes).
       local_state->SetInt64(prefs::kSwReporterLastTimeSentReport,
                             base::Time::Now().ToInternalValue());
     }
 
@@ skipping 42 matching lines @@
 
     // If the reporter failed to launch, do not process the results. (The exit
     // code itself doesn't need to be logged in this case because
     // SW_REPORTER_FAILED_TO_START is logged in |LaunchAndWaitForExit|.)
     if (exit_code == kReporterFailureExitCode)
       return;
 
     UMAHistogramReporter uma(finished_invocation.suffix);
     uma.ReportVersion(version);
     uma.ReportExitCode(exit_code);
-    uma.ReportFoundUwS(finished_invocation.flags &
-                       SwReporterInvocation::FLAG_LOG_TO_RAPPOR);
+    uma.ReportFoundUwS(
+        BehaviourIsEnabled(finished_invocation.supported_behaviours,
+                           SwReporterInvocation::BEHAVIOUR_LOG_TO_RAPPOR));
 
     PrefService* local_state = g_browser_process->local_state();
     if (local_state) {
-      if (finished_invocation.flags &
-          SwReporterInvocation::FLAG_LOG_EXIT_CODE_TO_PREFS)
+      if (BehaviourIsEnabled(
+              finished_invocation.supported_behaviours,
+              SwReporterInvocation::BEHAVIOUR_LOG_EXIT_CODE_TO_PREFS)) {
         local_state->SetInteger(prefs::kSwReporterLastExitCode, exit_code);
+      }
       local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
                             base::Time::Now().ToInternalValue());
     }
     uma.ReportRuntime(reporter_running_time);
     uma.ReportScanTimes();
     uma.ReportMemoryUsage();
+    if (finished_invocation.logs_upload_enabled)
+      uma.RecordLogsUploadResult();
 
-    if (!(finished_invocation.flags &
-          SwReporterInvocation::FLAG_TRIGGER_PROMPT))
+    if (!BehaviourIsEnabled(finished_invocation.supported_behaviours,
+                            SwReporterInvocation::BEHAVIOUR_TRIGGER_PROMPT)) {
       return;
+    }
 
     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;
     }
 
     if (exit_code != kSwReporterPostRebootCleanupNeeded &&
         exit_code != kSwReporterCleanupNeeded) {
@@ skipping 57 matching lines @@
           FROM_HERE,
           base::Bind(&ReporterRunner::TryToRun, base::Unretained(this)),
           next_trigger - now);
     }
   }
 
   // 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()) {
+    if (!base::FeatureList::IsEnabled(kSwReporterExtendedSafeBrowsingFeature))
+      return false;
+
+    UMAHistogramReporter uma;
+    if (!SafeBrowsingExtendedReportingEnabled()) {
+      uma.RecordLogsUploadEnabled(REPORTER_LOGS_UPLOADS_SBER_DISABLED);
       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. This is intended as a
-    // measure for failure recovery, in case the time in local state is
-    // incorrectly set to the future.
-    if (last_time_sent_logs > now)
+    const base::Time next_time_send_logs =
+        last_time_sent_logs +
+        base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent);
+    // Send the logs if the last send is the future or if the interval has
+    // passed. The former is intended as a measure for failure recovery, in
+    // case the time in local state is incorrectly set to the future.
+    if (last_time_sent_logs > now || next_time_send_logs <= now) {
+      uma.RecordLogsUploadEnabled(REPORTER_LOGS_UPLOADS_ENABLED);
       return true;
-    // Otherwise, send them if the interval has passed.
-    return last_time_sent_logs +
-               base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent) <=
-           now;
+    }
+    uma.RecordLogsUploadEnabled(REPORTER_LOGS_UPLOADS_RECENTLY_SENT_LOGS);
+    return false;
   }
 
   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()));
   }
 
@@ skipping 37 matching lines @@
 
 SwReporterInvocation SwReporterInvocation::FromCommandLine(
     const base::CommandLine& command_line) {
   SwReporterInvocation invocation;
   invocation.command_line = command_line;
   return invocation;
 }
 
 bool SwReporterInvocation::operator==(const SwReporterInvocation& other) const {
   return command_line.argv() == other.command_line.argv() &&
-         suffix == other.suffix && flags == other.flags;
+         suffix == other.suffix &&
+         supported_behaviours == other.supported_behaviours &&
+         logs_upload_enabled == other.logs_upload_enabled;
 }
 
 void RunSwReporters(const SwReporterQueue& invocations,
                     const base::Version& version,
                     scoped_refptr<base::TaskRunner> main_thread_task_runner,
                     scoped_refptr<base::TaskRunner> blocking_task_runner) {
   DCHECK(!invocations.empty());
   DCHECK(version.IsValid());
   ReporterRunner::ScheduleInvocations(invocations, version,
                                       std::move(main_thread_task_runner),
                                       std::move(blocking_task_runner));
 }
 
 bool ReporterFoundUws() {
   PrefService* local_state = g_browser_process->local_state();
   if (!local_state)
     return false;
   int exit_code = local_state->GetInteger(prefs::kSwReporterLastExitCode);
   return exit_code == kSwReporterCleanupNeeded;
 }
 
 bool UserHasRunCleaner() {
   base::string16 cleaner_key_path(kSoftwareRemovalToolRegistryKey);
   cleaner_key_path.append(L"\\").append(kCleanerSubKey);
 
-  base::win::RegKey srt_cleaner_key(HKEY_CURRENT_USER, cleaner_key_path.c_str(),
-                                    KEY_QUERY_VALUE);
-
-  return srt_cleaner_key.Valid() && srt_cleaner_key.GetValueCount() > 0;
+  base::win::RegKey srt_cleaner_key;
+  return srt_cleaner_key.Open(HKEY_CURRENT_USER, cleaner_key_path.c_str(),
+                              KEY_QUERY_VALUE) == ERROR_SUCCESS &&
+         srt_cleaner_key.GetValueCount() > 0;
 }
 
 void SetSwReporterTestingDelegate(SwReporterTestingDelegate* delegate) {
   g_testing_delegate_ = delegate;
 }
 
 }  // namespace safe_browsing