SRT Bubble

chrome/browser/component_updater/sw_reporter_installer_win.cc

 // Copyright (c) 2014 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/component_updater/sw_reporter_installer_win.h"
 
 #include <string>
 #include <vector>
 
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/process/kill.h"
 #include "base/process/launch.h"
 #include "base/task_runner_util.h"
 #include "base/threading/worker_pool.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/safe_browsing/srt_global_error.h"
+#include "chrome/browser/ui/browser_finder.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/component_updater_paths.h"
 #include "components/component_updater/component_updater_service.h"
 #include "components/component_updater/component_updater_utils.h"
 #include "components/component_updater/default_component_installer.h"
 #include "components/component_updater/pref_names.h"
+#include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
 namespace component_updater {
 
 namespace {
 
 // These values are used to send UMA information and are replicated in the
 // histograms.xml file, so the order MUST NOT CHANGE.
@@ skipping 23 matching lines @@
                              0x5f, 0xea, 0xf0, 0x88, 0xf6, 0x97, 0x9b, 0xc7};
 
 const base::FilePath::CharType kSwReporterExeName[] =
     FILE_PATH_LITERAL("software_reporter_tool.exe");
 
 // Where to fetch the reporter exit code in the registry.
 const wchar_t kSoftwareRemovalToolRegistryKey[] =
     L"Software\\Google\\Software Removal Tool";
 const wchar_t kExitCodeRegistryValueName[] = L"ExitCode";
 
+// Exit codes that identify that a cleanup is needed.
+const int kCleanupNeeded = 0;
+const int kPostRebootCleanupNeeded = 4;
+
 void ReportUmaStep(SwReporterUmaValue value) {
   UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.Step", value, SW_REPORTER_MAX);
 }
 
 // This function is called on the UI thread to report the SwReporter exit code
 // and then clear it from the registry as well as clear the execution state
 // from the local state. This could be called from an interruptible worker
 // thread so should be resilient to unexpected shutdown.
-void ReportAndClearExitCode(int exit_code) {
+void ReportAndClearExitCode(int exit_code, const std::string& version) {

[gab, 2014/09/24 16:22:36]

Augment method comment with |version|.

[MAD, 2014/09/24 20:40:35]

Done.

   UMA_HISTOGRAM_SPARSE_SLOWLY("SoftwareReporter.ExitCode", exit_code);
 
+  if (exit_code == kPostRebootCleanupNeeded || exit_code == kCleanupNeeded) {
+    Browser* browser =
+        chrome::FindLastActiveWithHostDesktopType(chrome::GetActiveDesktop());

[gab, 2014/09/24 16:22:36]

I don't think this is what you want. This may be NULL in numerous scenarios (see
method comment) and can also be a non-tabbed browser.

The comment @
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
hints that you want to use a ScopedTabbedBrowserDisplayer for this use case
(which will even take care of creating one for you if none exist).

[MAD, 2014/09/24 20:40:35]

The problem is that I don't have a profile here, so I can't use
ScopedTabbedBrowserDisplayer. I think it's OK if in some edge cases we don't get
a browser, we will simply try again when we do have one. But your comment about
the potential of getting a non tabbed browser is interesting, indeed we see a
weird bubble if we launch with a --app= switch... Thanks! Fixed...

On 2014/09/24 16:22:36, gab wrote:

+    if (browser) {
+      Profile* profile = browser->profile();
+      DCHECK(profile);
+      const std::string prompt_state =
+          profile->GetPrefs()->GetString(prefs::kSwReporterPromptState);
+      // Don't show the prompt again if it's been shown before.
+      if (prompt_state.empty()) {
+        profile->GetPrefs()->SetString(prefs::kSwReporterPromptState, version);

[gab, 2014/09/24 16:22:36]

Only set this pref if we actually end up showing the bubble below.

[MAD, 2014/09/24 20:40:35]

No, because we still create the global error which makes the bubble available
via the menu, and will show up as a bubble as soon as the other bubbles get
dismissed.

+        GlobalErrorService* global_error_service =
+            GlobalErrorServiceFactory::GetForProfile(profile);
+        SRTGlobalError* global_error = new SRTGlobalError(global_error_service);
+        global_error_service->AddGlobalError(global_error);

[gab, 2014/09/24 16:22:36]

This gives ownership of |global_error|, make this explicit by putting it in a
scoped_ptr above and releasing it here.

[MAD, 2014/09/24 20:40:35]

If I release it here, I won't be able to use it below...

+
+        // Do not try to show bubble if another GlobalError is already showing
+        // one.
+        const GlobalErrorService::GlobalErrorList& global_errors(
+            global_error_service->errors());
+        GlobalErrorService::GlobalErrorList::const_iterator it;
+        for (it = global_errors.begin(); it != global_errors.end(); ++it) {
+          if ((*it)->GetBubbleView())

[robertshield, 2014/09/24 13:02:12]

Just to double check: GetBubbleView() should return NULL for the |global_error|
just added line 114 since it hasn't been shown yet, right?

[MAD, 2014/09/24 20:40:35]

Yes.

+            break;
+        }
+        if (it == global_errors.end())
+          global_error->ShowBubbleView(browser);

[gab, 2014/09/24 16:22:36]

This is not safe as you gave up ownership of |global_error| above and thus
depend on the internal implementation of GlobalErrorService to have kept it
alive (the scoped_ptr syntax suggested above specifically prevents this type of
mistake).

[MAD, 2014/09/24 20:40:35]

But then if we release it above, we still can't use it here... :-/
Added comments to clarify the lifespan and ownership...

+      }
+    }
+  }
+
   base::win::RegKey srt_key(
       HKEY_CURRENT_USER, kSoftwareRemovalToolRegistryKey, KEY_WRITE);
   srt_key.DeleteValue(kExitCodeRegistryValueName);
 }
 
 // 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 anytime, so it shouldn't depend on anything external that
 // could be shutdown beforehand.
-void LaunchAndWaitForExit(const base::FilePath& exe_path) {
+void LaunchAndWaitForExit(const base::FilePath& exe_path,
+                          const std::string& version) {
   const base::CommandLine reporter_command_line(exe_path);
   base::ProcessHandle scan_reporter_process = base::kNullProcessHandle;
   if (!base::LaunchProcess(reporter_command_line,
                            base::LaunchOptions(),
                            &scan_reporter_process)) {
     ReportUmaStep(SW_REPORTER_FAILED_TO_START);
     return;
   }
   ReportUmaStep(SW_REPORTER_START_EXECUTION);
 
   int exit_code = -1;
   bool success = base::WaitForExitCode(scan_reporter_process, &exit_code);
   DCHECK(success);
   base::CloseProcessHandle(scan_reporter_process);
   scan_reporter_process = base::kNullProcessHandle;
   // It's OK if this doesn't complete, the work will continue on next startup.
-  BrowserThread::PostTask(BrowserThread::UI,
-                          FROM_HERE,
-                          base::Bind(&ReportAndClearExitCode, exit_code));
-}
-
-void ExecuteReporter(const base::FilePath& install_dir) {
-  base::WorkerPool::PostTask(
+  BrowserThread::PostTask(
+      BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&LaunchAndWaitForExit, install_dir.Append(kSwReporterExeName)),
-      true);
+      base::Bind(&ReportAndClearExitCode, exit_code, version));
 }
 
 class SwReporterInstallerTraits : public ComponentInstallerTraits {
  public:
   explicit SwReporterInstallerTraits(PrefService* prefs) : prefs_(prefs) {}
 
   virtual ~SwReporterInstallerTraits() {}
 
   virtual bool VerifyInstallation(const base::FilePath& dir) const {
     return base::PathExists(dir.Append(kSwReporterExeName));
@@ skipping 18 matching lines @@
 
     // A previous run may have results in the registry, so check and report
     // them if present.
     base::win::RegKey srt_key(
         HKEY_CURRENT_USER, kSoftwareRemovalToolRegistryKey, KEY_READ);
     DWORD exit_code;
     if (srt_key.Valid() &&
         srt_key.ReadValueDW(kExitCodeRegistryValueName, &exit_code) ==
             ERROR_SUCCESS) {
       ReportUmaStep(SW_REPORTER_REGISTRY_EXIT_CODE);
-      ReportAndClearExitCode(exit_code);
+      ReportAndClearExitCode(exit_code, version.GetString());
     }
 
     // If we can't access local state, we can't see when we last ran, so
     // just exit without running.
     if (!g_browser_process || !g_browser_process->local_state())
       return;
 
     // Run the reporter if it hasn't been triggered in the
     // kDaysBetweenSwReporterRuns days.
     const base::Time last_time_triggered = base::Time::FromInternalValue(
         g_browser_process->local_state()->GetInt64(
             prefs::kSwReporterLastTimeTriggered));
     if ((base::Time::Now() - last_time_triggered).InDays() >=
         kDaysBetweenSwReporterRuns) {
       g_browser_process->local_state()->SetInt64(
           prefs::kSwReporterLastTimeTriggered,
           base::Time::Now().ToInternalValue());
 
-      ExecuteReporter(install_dir);
+      base::WorkerPool::PostTask(
+          FROM_HERE,
+          base::Bind(&LaunchAndWaitForExit,
+                     install_dir.Append(kSwReporterExeName),
+                     version.GetString()),
+          true);
     }
   }
 
   virtual base::FilePath GetBaseDirectory() const { return install_dir(); }
 
   virtual void GetHash(std::vector<uint8>* hash) const { GetPkHash(hash); }
 
   virtual std::string GetName() const { return "Software Reporter Tool"; }
 
   static base::FilePath install_dir() {
@@ skipping 22 matching lines @@
   PrefService* prefs_;
   static wchar_t version_dir_[_MAX_PATH];
 };
 
 wchar_t SwReporterInstallerTraits::version_dir_[] = {};
 
 }  // namespace
 
 void RegisterSwReporterComponent(ComponentUpdateService* cus,
                                  PrefService* prefs) {
-  // The Sw reporter shouldn't run if the user isn't reporting metrics.
-  if (!ChromeMetricsServiceAccessor::IsMetricsReportingEnabled())
-    return;
-
   // Install the component.
   scoped_ptr<ComponentInstallerTraits> traits(
       new SwReporterInstallerTraits(prefs));
   // |cus| will take ownership of |installer| during installer->Register(cus).
   DefaultComponentInstaller* installer =
       new DefaultComponentInstaller(traits.Pass());
   installer->Register(cus);
 }
 
 void RegisterPrefsForSwReporter(PrefRegistrySimple* registry) {
   registry->RegisterInt64Pref(prefs::kSwReporterLastTimeTriggered, 0);
 }
 
+void RegisterProfilePrefsForSwReporter(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterStringPref(
+      prefs::kSwReporterPromptState,
+      "",
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+}
+
 }  // namespace component_updater