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,
+};
+
+// Used to send UMA information about missing logs upload result in the registry
+// for the reporter. Replicated in the histograms.xml file, so the order
+// MUST NOT CHANGE.
+enum SwReporterLogsUploadResultRegistryError {
+  REPORTER_LOGS_UPLOAD_RESULT_ERROR_NO_ERROR = 0,
+  REPORTER_LOGS_UPLOAD_RESULT_ERROR_REGISTRY_KEY_INVALID = 1,
+  REPORTER_LOGS_UPLOAD_RESULT_ERROR_VALUE_NOT_FOUND = 2,
+  REPORTER_LOGS_UPLOAD_RESULT_ERROR_VALUE_OUT_OF_BOUNDS = 3,
+  REPORTER_LOGS_UPLOAD_RESULT_ERROR_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";
+const char kLogsUploadResultRegistryErrorMetricName[] =
+    "SoftwareReporter.LogsUploadResultRegistryError";
+
+// 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 = 30;

[Jialiu Lin, 2016/09/21 17:41:18]

You changed this constant from 7 to 30. A little bit confusing. Are you trying
to reserve more histogram buckets for future use? I think you can simply
increase this const whenever you modify SoftwareReporter.LogsUploadResult in
histogram.xml.

So maybe keep 7 here, add note in the description of
SoftwareReporter.LogsUploadResult in histogram.xml, to remind others update this
const if they try to modify SoftwareReporter.LogsUploadResult?

[ftirelo, 2016/09/21 18:45:18]

The idea of moving from 7 to 30 was to reserve more histograms for future use
without the dependency on Chrome release.

This is particularly important in this case, because if we need to change the
Software Reporter to emit a new result value, we only need to change
histograms.xml, and the change will take effect immediately everywhere. On the
other hand, if the constant is hard-coded here, then such changes in the
reporter will need to wait for the whole Chrome release process. Please also
notice that this doesn't affect Chrome, which is only used for reporting
whatever is sent by the reporter.

[Jialiu Lin, 2016/09/21 18:48:31]

Thanks for clarifying this. SGTM. 

 
 // 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);

[Jialiu Lin, 2016/09/21 17:41:18]

nit: Maybe make "SoftwareReporter.Step" a const as well?

[ftirelo, 2016/09/21 18:45:18]

Done.

   }
 
+  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) {
+      RecordEnumerationHistogram(
+          kLogsUploadResultRegistryErrorMetricName,
+          REPORTER_LOGS_UPLOAD_RESULT_ERROR_REGISTRY_KEY_INVALID,
+          REPORTER_LOGS_UPLOAD_RESULT_ERROR_MAX);
+      return;
+    }
+
+    if (reporter_key.ReadValueDW(kLogsUploadResultValueName,
+                                 &logs_upload_result) != ERROR_SUCCESS) {
+      RecordEnumerationHistogram(
+          kLogsUploadResultRegistryErrorMetricName,
+          REPORTER_LOGS_UPLOAD_RESULT_ERROR_VALUE_NOT_FOUND,
+          REPORTER_LOGS_UPLOAD_RESULT_ERROR_MAX);
+      return;
+    }
+
+    if (logs_upload_result >= kSwReporterLogsUploadResultMax) {
+      RecordEnumerationHistogram(
+          kLogsUploadResultRegistryErrorMetricName,
+          REPORTER_LOGS_UPLOAD_RESULT_ERROR_VALUE_OUT_OF_BOUNDS,
+          REPORTER_LOGS_UPLOAD_RESULT_ERROR_MAX);
+      return;
+    }
+
+    RecordEnumerationHistogram(kLogsUploadResultMetricName,
+                               static_cast<Sample>(logs_upload_result),
+                               kSwReporterLogsUploadResultMax);
+    reporter_key.DeleteValue(kLogsUploadResultValueName);
+    RecordEnumerationHistogram(kLogsUploadResultRegistryErrorMetricName,
+                               REPORTER_LOGS_UPLOAD_RESULT_ERROR_NO_ERROR,
+                               REPORTER_LOGS_UPLOAD_RESULT_ERROR_MAX);
+  }
+
  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 336 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 (next_invocation.BehaviourIsSupported(
+            SwReporterInvocation::BEHAVIOUR_ALLOW_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(finished_invocation.BehaviourIsSupported(
+        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 (finished_invocation.BehaviourIsSupported(
+              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 (!finished_invocation.BehaviourIsSupported(
+            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;
+}
+
+bool SwReporterInvocation::BehaviourIsSupported(
+    SwReporterInvocation::Behaviours intended_behaviour) const {
+  return (supported_behaviours & intended_behaviour) != 0;
 }
 
 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