Primary histograms for InBrowserCleanerUI experiment

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc

 // Copyright 2017 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/chrome_cleaner/chrome_cleaner_controller_win.h"
 
 #include <windows.h>
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/task_scheduler/task_traits.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.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_manager.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_client_info_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
 #include "chrome/installer/util/scoped_token_privilege.h"
 #include "components/chrome_cleaner/public/constants/constants.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/http/http_status_code.h"
 
 namespace safe_browsing {
 
 namespace {
 
@@ skipping 53 matching lines @@
 ChromeCleanerController::IdleReason IdleReasonWhenConnectionClosedTooSoon(
     ChromeCleanerController::State current_state) {
   DCHECK(current_state == ChromeCleanerController::State::kScanning ||
          current_state == ChromeCleanerController::State::kInfected);
 
   return current_state == ChromeCleanerController::State::kScanning
              ? ChromeCleanerController::IdleReason::kScanningFailed
              : ChromeCleanerController::IdleReason::kConnectionLost;
 }
 
+void RecordCleanerLogsAcceptanceHistogram(bool value) {

[csharp, 2017/07/07 17:32:51]

what about logs_uploaded instead of value?

[ftirelo, 2017/07/07 20:27:52]

Used logs_accepted instead.

+  UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.CleanerLogsAcceptance", value);
+}
+
 }  // namespace
 
+void RecordCleanupStartedHistogram(CleanupStartedHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.CleanupStarted", value,
+                            CLEANUP_STARTED_MAX);
+}
+
 ChromeCleanerControllerDelegate::ChromeCleanerControllerDelegate() = default;
 
 ChromeCleanerControllerDelegate::~ChromeCleanerControllerDelegate() = default;
 
 void ChromeCleanerControllerDelegate::FetchAndVerifyChromeCleaner(
     FetchedCallback fetched_callback) {
   FetchChromeCleaner(
       base::BindOnce(&OnChromeCleanerFetched, base::Passed(&fetched_callback)));
 }
 
@@ skipping 109 matching lines @@
     return;
 
   DCHECK(prompt_user_callback_);
 
   PromptAcceptance acceptance = PromptAcceptance::DENIED;
   State new_state = State::kIdle;
   switch (user_response) {
     case UserResponse::kAcceptedWithLogs:
       acceptance = PromptAcceptance::ACCEPTED_WITH_LOGS;
       SetLogsEnabled(true);
+      RecordCleanerLogsAcceptanceHistogram(true);
       new_state = State::kCleaning;
       delegate_->TagForResetting(profile);
       break;
     case UserResponse::kAcceptedWithoutLogs:
       acceptance = PromptAcceptance::ACCEPTED_WITHOUT_LOGS;
       SetLogsEnabled(false);
+      RecordCleanerLogsAcceptanceHistogram(false);
       new_state = State::kCleaning;
       delegate_->TagForResetting(profile);
       break;
     case UserResponse::kDenied:  // Fallthrough
     case UserResponse::kDismissed:
       acceptance = PromptAcceptance::DENIED;
       idle_reason_ = IdleReason::kUserDeclinedCleanup;
       new_state = State::kIdle;
       break;
   }
 
   BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
       ->PostTask(FROM_HERE,
                  base::BindOnce(std::move(prompt_user_callback_), acceptance));
 
   // The transition to a new state should happen only after the response has
   // been posted on the UI thread so that if we transition to the kIdle state,
   // the response callback is not cleared before it has been posted.
   SetStateAndNotifyObservers(new_state);
 }
 
 void ChromeCleanerController::Reboot() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
   if (state() != State::kRebootRequired)
     return;
 
+  UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.RebootInitiated", true);
   InitiateReboot();
 }
 
 ChromeCleanerController::ChromeCleanerController()
     : real_delegate_(base::MakeUnique<ChromeCleanerControllerDelegate>()),
       delegate_(real_delegate_.get()),
       weak_factory_(this) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 }
 
@@ skipping 45 matching lines @@
 
 void ChromeCleanerController::OnChromeCleanerFetchedAndVerified(
     base::FilePath executable_path) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK_EQ(State::kScanning, state());
   DCHECK(reporter_invocation_);
 
   if (executable_path.empty()) {
     idle_reason_ = IdleReason::kScanningFailed;
     SetStateAndNotifyObservers(State::kIdle);
+    RecordPromptNotShownWithReasonHistogram(
+        NO_PROMPT_REASON_CLEANER_DOWNLOAD_FAILED);
     return;
   }
 
   DCHECK(executable_path.MatchesExtension(FILE_PATH_LITERAL(".exe")));
 
   ChromeCleanerRunner::ChromeMetricsStatus metrics_status =
       delegate_->IsMetricsAndCrashReportingEnabled()
           ? ChromeCleanerRunner::ChromeMetricsStatus::kEnabled
           : ChromeCleanerRunner::ChromeMetricsStatus::kDisabled;
 
@@ skipping 42 matching lines @@
   DCHECK_EQ(State::kScanning, state());
   DCHECK(!files_to_delete_);
   DCHECK(!prompt_user_callback_);
 
   if (files_to_delete->empty()) {
     BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
         ->PostTask(FROM_HERE, base::BindOnce(std::move(prompt_user_callback),
                                              PromptAcceptance::DENIED));
     idle_reason_ = IdleReason::kScanningFoundNothing;
     SetStateAndNotifyObservers(State::kIdle);
+    RecordPromptNotShownWithReasonHistogram(NO_PROMPT_REASON_NOTHING_FOUND);
     return;
   }
 
+  UMA_HISTOGRAM_COUNTS_1000("SoftwareReporter.NumberOfFilesToDelete",
+                            files_to_delete->size());

[csharp, 2017/07/07 17:32:51]

Is there any problem if files_to_delete->size() > 1000? I vaguely remember
hearing about problems on the back end if we gave values a lot higher than what
the count should have been

[ftirelo, 2017/07/07 20:27:52]

There is an overflow buffer for counts > 1000. Since less than 0.5% of file
lists are larger than that, we should be fine.

   files_to_delete_ = std::move(files_to_delete);
   prompt_user_callback_ = std::move(prompt_user_callback);
   SetStateAndNotifyObservers(State::kInfected);
 }
 
 void ChromeCleanerController::OnConnectionClosed() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK_NE(State::kIdle, state());
   DCHECK_NE(State::kRebootRequired, state());
 
   if (state() == State::kScanning || state() == State::kInfected) {
     idle_reason_ = IdleReasonWhenConnectionClosedTooSoon(state());
     SetStateAndNotifyObservers(State::kIdle);
+    RecordPromptNotShownWithReasonHistogram(
+        NO_PROMPT_REASON_IPC_CONNECTION_BROKEN);

[csharp, 2017/07/07 17:32:51]

Is it possible for the connect to be closed after the user has seen the prompt?

[ftirelo, 2017/07/07 20:27:52]

Yes, if the cleaner binary crashes.

[csharp, 2017/07/07 20:53:33]

Should this be counted then, since in that case wouldn't we have already
recorded the prompt as being shown?

[ftirelo, 2017/07/07 21:22:07]

Added a new histogram for moment when IPC connection was lost and changed this
to only register non-prompt if state is kScanning.

     return;
   }
   // Nothing to do if OnConnectionClosed() is called in other states:
   // - This function will not be called in the kIdle and kRebootRequired
   //   states since we invalidate all weak pointers when we enter those states.
   // - In the kCleaning state, we don't care about the connection to the Chrome
   //   Cleaner process since communication via Mojo is complete and only the
   //   exit code of the process is of any use to us (for deciding whether we
   //   need to reboot).
 }
@@ skipping 13 matching lines @@
             process_status.launch_status);
 
   if (process_status.launch_status !=
       ChromeCleanerRunner::LaunchStatus::kSuccess) {
     idle_reason_ = IdleReason::kCleaningFailed;
     SetStateAndNotifyObservers(State::kIdle);
     return;
   }
 
   if (process_status.exit_code == kRebootRequiredExitCode) {
+    UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.RebootRequired", true);
     SetStateAndNotifyObservers(State::kRebootRequired);
     return;
   }
 
   if (process_status.exit_code == kRebootNotRequiredExitCode) {
     delegate_->ResetTaggedProfiles(
         g_browser_process->profile_manager()->GetLoadedProfiles(),
         base::BindOnce(&ChromeCleanerController::OnSettingsResetCompleted,
                        base::Unretained(this)));
     ResetCleanerDataAndInvalidateWeakPtrs();
@@ skipping 17 matching lines @@
       observer.OnRebootFailed();
   }
 }
 
 void ChromeCleanerController::OnSettingsResetCompleted() {
   idle_reason_ = IdleReason::kCleaningSucceeded;
   SetStateAndNotifyObservers(State::kIdle);
 }
 
 }  // namespace safe_browsing