Elevated install of recovery component when needed

chrome/browser/component_updater/recovery_component_installer.cc

Index: chrome/browser/component_updater/recovery_component_installer.cc
diff --git a/chrome/browser/component_updater/recovery_component_installer.cc b/chrome/browser/component_updater/recovery_component_installer.cc
index 9aadd24c0c7364422689bdcdcfbee828ce331041..4fc2231aed724753072b760e0f16729e2c4ba76d 100644
--- a/chrome/browser/component_updater/recovery_component_installer.cc
+++ b/chrome/browser/component_updater/recovery_component_installer.cc
@@ -13,13 +13,15 @@
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/json/json_file_value_serializer.h"
 #include "base/logging.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/strings/string_util.h"
-#include "base/values.h"
+#include "chrome/common/chrome_switches.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/pref_names.h"
@@ -47,13 +49,90 @@ const base::FilePath::CharType kRecoveryFileName[] =
 
 const char kRecoveryManifestName[] = "ChromeRecovery";
 
+// This is the contract between ChromeRecovery and this installer for the
+// meaning of ChromeRecovery process exit code.
+enum ChromeRecvoeryExitCode {

[waffles, 2014/12/05 00:59:48]

typo: "Recvoery"

[Sorin Jianu, 2014/12/05 02:10:36]

Just simply write:
// ChromeRecovery process exit codes.

Also consider documenting the values if it is appropriate.

[xiaoling, 2014/12/05 19:45:56]

Done.

[xiaoling, 2014/12/05 19:45:56]

Done.

+  EXIT_CODE_RECOVERY_SUCCEEDED = 0,
+  EXIT_CODE_RECOVERY_SKIPPED = 1,
+  EXIT_CODE_ELEVATION_NEEDED = 2,
+};
+
+// Checks if elevated recovery simulation switch was present on the command
+// line. This is for testing purpose.
+bool SimulatingElevatedRecovery() {
+  const CommandLine& cmd_line = *CommandLine::ForCurrentProcess();

[Sorin Jianu, 2014/12/05 02:10:36]

we could use a pointer type here, no need to dereference.

return CommandLine::ForCurrentProcess()->HasSwitch(...

[xiaoling, 2014/12/05 19:45:56]

Done.

+  return cmd_line.HasSwitch(switches::kSimulateElevatedRecovery);
+}
+
+scoped_ptr<base::DictionaryValue> ReadManifest(const base::FilePath& manifest) {
+  JSONFileValueSerializer serializer(manifest);
+  std::string error;
+  scoped_ptr<base::Value> root(serializer.Deserialize(NULL, &error));

[Sorin Jianu, 2014/12/05 02:10:36]

Are we including "base/memory/scoped_ptr.h" anywhere?

[xiaoling, 2014/12/05 19:45:56]

Done.

+  if (!root.get())

[Sorin Jianu, 2014/12/05 02:10:36]

this is mostly a matter of preference but you could do:

if (root.get() && root->IsType(base::Value::TYPE_DICTIONARY)) 
  return scoped_ptr<base::DictionaryValue>(
               static_cast<base::DictionaryValue*>(root.release())).Pass()

return scoped_ptr<base::DictionaryValue>();

or you could do:

if (!root.get() || !root->IsType(base::Value::TYPE_DICTIONARY)))
  return scoped_ptr<base::DictionaryValue>();

return scoped_ptr<base::DictionaryValue>(
            static_cast<base::DictionaryValue*>(root.release())).Pass();

The call to Pass may not be needed, see @50 in base/memory/scoped_ptr.h.

[xiaoling, 2014/12/05 19:45:56]

Done.

+    return scoped_ptr<base::DictionaryValue>();
+  if (!root->IsType(base::Value::TYPE_DICTIONARY))
+    return scoped_ptr<base::DictionaryValue>();
+  return scoped_ptr<base::DictionaryValue>(
+             static_cast<base::DictionaryValue*>(root.release())).Pass();
+}
+
+void DoElevatedInstallRecoveryComponent(const base::FilePath& path) {
+  base::FilePath main_file = path.Append(kRecoveryFileName);

[Sorin Jianu, 2014/12/05 02:10:36]

Can any of these locals in this function be declared const?

[xiaoling, 2014/12/05 19:45:56]

Done.

+  base::FilePath manifest_file =
+      path.Append(FILE_PATH_LITERAL("manifest.json"));
+  if (!base::PathExists(main_file) || !base::PathExists(manifest_file))
+    return;
+
+  scoped_ptr<base::DictionaryValue> manifest(ReadManifest(manifest_file));
+  std::string name;
+  manifest->GetStringASCII("name", &name);
+  if (name != kRecoveryManifestName)
+    return;
+  std::string proposed_version;
+  manifest->GetStringASCII("version", &proposed_version);
+  Version version(proposed_version.c_str());
+  if (!version.IsValid())
+    return;
+
+  CommandLine cmdline(main_file);
+  std::string arguments;
+  if (manifest->GetStringASCII("x-recovery-args", &arguments))
+    cmdline.AppendArg(arguments);
+  std::string add_version;
+  if (manifest->GetStringASCII("x-recovery-add-version", &add_version) &&
+      add_version == "yes")
+    cmdline.AppendSwitchASCII("version", version.GetString());

[Sorin Jianu, 2014/12/05 02:10:36]

I always use {} in cases where the conditional and its condition do not fit in
one line.

"In general, curly braces are not required for single-line statements, but they
are allowed if you like them; conditional or loop statements with complex
conditions or statements may be more readable with curly braces."

[xiaoling, 2014/12/05 19:45:56]

Done.

+
+  base::LaunchOptions options;
+#if defined(OS_WIN)
+  options.start_hidden = true;
+  base::LaunchElevatedProcess(cmdline, options);
+#else
+  NOTREACHED();
+#endif
+}
+
+void ElevatedInstallRecoveryComponent(const base::FilePath& installer_path) {
+  BrowserThread::PostTask(

[Sorin Jianu, 2014/12/05 02:10:36]

In general, we wanted to get rid of executing code on the FILE thread, does it
make sense to get a thread from the blocking pool, like this?

content::BrowserThread::GetBlockingPool()
      ->GetSequencedTaskRunnerWithShutdownBehavior(
          content::BrowserThread::GetBlockingPool()->GetSequenceToken(),
          base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);

[xiaoling, 2014/12/05 19:45:56]

Done. Used WorkerPool thread to avoid hassle of maintaining ref counter of the
task runner.

+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&DoElevatedInstallRecoveryComponent, installer_path));
+}
+
 }  // namespace
 
+// Component installer that is responsible to repair the chrome installation
+// or repair the Google update installation. This is a last resort safety
+// mechanism.
+// For user Chrome, recovery component just installs silently. For machine
+// Chrome, elevation may be needed. If that happens, the installer will set
+// preference flag prefs::kRecoveryComponentNeedsElevation to request that.
+// There is a global error service monitors this flag and will pop up
+// bubble if the flag is set to true.
+// See chrome/browser/recovery/recovery_install_global_error.cc for details.
 class RecoveryComponentInstaller : public ComponentInstaller {
  public:
-  explicit RecoveryComponentInstaller(const Version& version,
-                                      PrefService* prefs);
-
+  RecoveryComponentInstaller(const Version& version, PrefService* prefs);
   ~RecoveryComponentInstaller() override {}
 
   void OnUpdateError(int error) override;
@@ -69,6 +148,10 @@ class RecoveryComponentInstaller : public ComponentInstaller {
   PrefService* prefs_;
 };
 
+void SimulateElevatedRecoveryHelper(PrefService* prefs) {
+  prefs->SetBoolean(prefs::kRecoveryComponentNeedsElevation, true);
+}
+
 void RecoveryRegisterHelper(ComponentUpdateService* cus, PrefService* prefs) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   Version version(prefs->GetString(prefs::kRecoveryComponentVersion));
@@ -92,6 +175,17 @@ void RecoveryUpdateVersionHelper(const Version& version, PrefService* prefs) {
   prefs->SetString(prefs::kRecoveryComponentVersion, version.GetString());
 }
 
+void RecoveryUpdateElevationHelper(bool elevation_needed, PrefService* prefs) {

[Sorin Jianu, 2014/12/05 02:10:36]

We can be more specific here and rename this function to:
SetRecoveryComponentNeedsElevationPrefs.

[xiaoling, 2014/12/05 19:45:56]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  prefs->SetBoolean(prefs::kRecoveryComponentNeedsElevation, elevation_needed);
+}
+
+void RecoveryUpdateUnpackPathHelper(const base::FilePath& unpack_path,
+                                    PrefService* prefs) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  prefs->SetFilePath(prefs::kRecoveryComponentUnpackPath, unpack_path);

[Sorin Jianu, 2014/12/05 02:10:36]

Same as above. 
SetRecoveryComponentNeedsUnpackPath.

The reason is that the code @252 becomes easier to read.

[xiaoling, 2014/12/05 19:45:56]

Done.

+}
+
 RecoveryComponentInstaller::RecoveryComponentInstaller(const Version& version,
                                                        PrefService* prefs)
     : current_version_(version), prefs_(prefs) {
@@ -120,11 +214,8 @@ bool RecoveryComponentInstaller::Install(const base::DictionaryValue& manifest,
   base::FilePath path;
   if (!PathService::Get(DIR_RECOVERY_BASE, &path))
     return false;
-  if (!base::PathExists(path)) {
-    if (!base::CreateDirectory(path)) {
+  if (!base::PathExists(path) && !base::CreateDirectory(path))
       return false;
-    }
-  }
   path = path.AppendASCII(version.GetString());
   if (base::PathExists(path) && !base::DeleteFile(path, true))
     return false;
@@ -142,18 +233,48 @@ bool RecoveryComponentInstaller::Install(const base::DictionaryValue& manifest,
   if (manifest.GetStringASCII("x-recovery-args", &arguments))
     cmdline.AppendArg(arguments);
   std::string add_version;
-  if (manifest.GetStringASCII("x-recovery-add-version", &add_version)) {
-    if (add_version == "yes")
-      cmdline.AppendSwitchASCII("version", current_version_.GetString());
+  if (manifest.GetStringASCII("x-recovery-add-version", &add_version) &&
+      add_version == "yes")
+    cmdline.AppendSwitchASCII("version", current_version_.GetString());
+
+  bool install_result = false;
+#if defined(OS_WIN)

[Sorin Jianu, 2014/12/05 02:10:36]

Refactor as discussed.

[xiaoling, 2014/12/05 19:45:56]

Done.

+  base::ProcessHandle process_handle;
+  base::LaunchOptions options;
+  options.start_hidden = true;
+  if (base::LaunchProcess(cmdline, options, &process_handle)) {
+    int installer_exit_code = 0;
+    const base::TimeDelta kMaxWaitTime = base::TimeDelta::FromSeconds(600);
+    if (base::WaitForExitCodeWithTimeout(process_handle,
+                                         &installer_exit_code,
+                                         kMaxWaitTime)) {
+      if (installer_exit_code == EXIT_CODE_ELEVATION_NEEDED) {
+        BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+            base::Bind(&RecoveryUpdateUnpackPathHelper, path, prefs_));
+        BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+            base::Bind(&RecoveryUpdateElevationHelper, true, prefs_));
+      }
+
+      // Mark install result as true regardless of exit code. We may need to do
+      // elevated install further, but from updater service perspective, the
+      // install is completed.
+      install_result = true;
+    } else {
+      // Ensure that the process terminates.
+      base::KillProcess(process_handle, -1, true);
+    }
   }
-  current_version_ = version;
-  if (prefs_) {
-    BrowserThread::PostTask(
-        BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(&RecoveryUpdateVersionHelper, version, prefs_));
+#else
+  install_result = base::LaunchProcess(cmdline, base::LaunchOptions(), NULL);
+#endif
+
+  if (install_result) {
+    current_version_ = version;
+    if (prefs_)
+      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+          base::Bind(&RecoveryUpdateVersionHelper, version, prefs_));
   }
-  return base::LaunchProcess(cmdline, base::LaunchOptions(), NULL);
+  return install_result;
 }
 
 bool RecoveryComponentInstaller::GetInstalledFile(
@@ -164,6 +285,13 @@ bool RecoveryComponentInstaller::GetInstalledFile(
 
 void RegisterRecoveryComponent(ComponentUpdateService* cus,
                                PrefService* prefs) {
+  if (SimulatingElevatedRecovery()) {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&SimulateElevatedRecoveryHelper, prefs));
+  }
+
 #if !defined(OS_CHROMEOS)
   // We delay execute the registration because we are not required in
   // the critical path during browser startup.
@@ -177,14 +305,22 @@ void RegisterRecoveryComponent(ComponentUpdateService* cus,
 
 void RegisterPrefsForRecoveryComponent(PrefRegistrySimple* registry) {
   registry->RegisterStringPref(prefs::kRecoveryComponentVersion, "0.0.0.0");
+  registry->RegisterFilePathPref(prefs::kRecoveryComponentUnpackPath,
+                                 base::FilePath());
+  registry->RegisterBooleanPref(prefs::kRecoveryComponentNeedsElevation, false);
 }
 
 void AcceptedElevatedRecoveryInstall(PrefService* prefs) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  ElevatedInstallRecoveryComponent(
+      prefs->GetFilePath(prefs::kRecoveryComponentUnpackPath));
+  prefs->SetBoolean(prefs::kRecoveryComponentNeedsElevation, false);
 }
 
 void DeclinedElevatedRecoveryInstall(PrefService* prefs) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  prefs->SetBoolean(prefs::kRecoveryComponentNeedsElevation, false);
 }
 
 }  // namespace component_updater