Reset user data directory and disk cache directory after downgrade.

chrome/browser/user_profiles_cleanup.cc

+// Copyright 2016 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/user_profiles_cleanup.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/files/file_enumerator.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
+#include "base/version.h"
+#include "base/win/registry.h"
+#include "chrome/browser/policy/policy_path_parser.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/chrome_version.h"
+#include "chrome/installer/util/google_update_constants.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace {

[grt (UTC plus 2), 2016/05/19 03:24:18]

nit: blank line after this

[zmin, 2016/05/19 23:08:02]

Done.

+base::FilePath::StringType g_delete_suffix = L".CHROME_DELETE";

[grt (UTC plus 2), 2016/05/19 03:24:18]

static objects are banned since they introduce exit-time dtors. please use:

constexpr base::FilePath::CharType kDeleteSuffix[] = FILE_PATH_LITERAL("...");

[zmin, 2016/05/19 23:08:02]

Done.

+
+// Get disk cache dir override value if exists. Return empty path for default

[grt (UTC plus 2), 2016/05/19 15:37:28]

"Returns the..."

[zmin, 2016/05/19 23:08:01]

Done.

+// disk cache dir.
+base::FilePath GetDiskCacheDir() {
+  base::FilePath disk_cache_dir;
+  policy::path_parser::CheckDiskCacheDirPolicy(&disk_cache_dir);
+  if (disk_cache_dir.empty()) {
+    base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+    disk_cache_dir = command_line->GetSwitchValuePath(switches::kDiskCacheDir);
+  }
+  if (disk_cache_dir.ReferencesParent())
+    return base::MakeAbsoluteFilePath(disk_cache_dir);
+  return disk_cache_dir;
+}
+
+base::Version GetDowngradeVersion() {

[grt (UTC plus 2), 2016/05/19 15:37:27]

this function belongs in chrome/installer/util somewhere. for the sake of
keeping related code together (and unittested), i propose that you put all
"interact with the downgrade version in the registry" functions together in
install_util.{cc,h} with tests in install_util_unittest.cc.

[zmin, 2016/05/19 23:08:02]

Done. Unit test will be uploaded later as I need to find a way to change the
state_path .
Right now I get the state path by
BrowserDistribution::GetDistribution()->GetStateKey() as you suggested below
which is a static function.

+  base::win::RegKey key;
+  base::string16 downgrade_version;
+  base::string16 key_path =

[grt (UTC plus 2), 2016/05/19 15:37:28]

BrowserDistribution::GetDistribution()->GetStateKey()

[zmin, 2016/05/19 23:08:02]

Done.

+      base::string16(L"Software\\Google\\Update\\ClientState\\") +
+      base::string16(google_update::kChromeUpgradeCode);
+  if (key.Open(HKEY_LOCAL_MACHINE, key_path.c_str(), KEY_QUERY_VALUE) !=

[grt (UTC plus 2), 2016/05/19 15:37:27]

Google Update interactions are always in the 32-bit hive:
KEY_QUERY_VALUE | KEY_WOW64_32KEY

[grt (UTC plus 2), 2016/05/19 15:37:28]

use InstallUtil::IsPerUserInstall to determine whether to look in HKLM or HKCU
rather than looking in both.

[zmin, 2016/05/19 23:08:02]

Done.

[zmin, 2016/05/19 23:08:02]

Done.

+          ERROR_SUCCESS ||
+      key.ReadValue(google_update::kRegDowngradeVersion, &downgrade_version) !=
+          ERROR_SUCCESS) {
+    key.Close();
+    if (key.Open(HKEY_CURRENT_USER, key_path.c_str(), KEY_QUERY_VALUE) !=
+            ERROR_SUCCESS ||
+        key.ReadValue(google_update::kRegDowngradeVersion,
+                      &downgrade_version) != ERROR_SUCCESS) {
+      return base::Version();
+    }
+  }
+  return base::Version(base::UTF16ToASCII(downgrade_version));
+}
+
+base::FilePath GetLastChromeVersionFile(const base::FilePath& user_data_dir) {
+  return user_data_dir.Append(L"LastChromeVersion");

[grt (UTC plus 2), 2016/05/19 15:37:28]

L"..." -> FILE_PATH_LITERAL("...")

[zmin, 2016/05/19 23:08:02]

Done.

+}
+
+base::Version GetLastChromeVersion(const base::FilePath& user_data_dir) {
+  std::string last_chrome_version_str;
+  if (base::ReadFileToString(GetLastChromeVersionFile(user_data_dir),
+                             &last_chrome_version_str))
+    return base::Version(
+        base::CollapseWhitespaceASCII(last_chrome_version_str, false));

[grt (UTC plus 2), 2016/05/19 15:37:28]

why not TrimWhitespaceASCII?

[zmin, 2016/05/19 23:08:02]

Done.

+  return base::Version();
+}
+
+base::FilePath GetTempDirNameForDelete(const base::FilePath& source_path) {

[grt (UTC plus 2), 2016/05/19 15:37:28]

doc comment

[zmin, 2016/05/19 23:08:02]

Done.

+  if (source_path.empty())
+    return base::FilePath();
+
+  base::FilePath target_path(source_path.value() + g_delete_suffix);

[grt (UTC plus 2), 2016/05/19 15:37:27]

use FilePath methods whenever possible. is this equivalent to the above:
  base::FilePath target_path(source_path.AddExtension(g_delete_suffix));
?

[zmin, 2016/05/19 23:08:03]

Done.

+  int uniquifier = base::GetUniquePathNumber(source_path, g_delete_suffix);
+  if (uniquifier > 0)

[grt (UTC plus 2), 2016/05/19 15:37:27]

nit: add braces for multi-line body

[zmin, 2016/05/19 23:08:02]

Done.

+    return target_path.InsertBeforeExtensionASCII(
+        base::StringPrintf(" (%d)", uniquifier));
+

[grt (UTC plus 2), 2016/05/19 15:37:27]

what if uniquifier < 0?

[zmin, 2016/05/19 23:08:02]

Good question. <0 means the dir have been copied  100 times without deletion. We
can return an empty path and do not rename dir if that happened.

+  return target_path;
+}
+
+void RenameDirectory(const base::FilePath& source,

[grt (UTC plus 2), 2016/05/19 15:37:28]

add a doc comment

[zmin, 2016/05/19 23:08:02]

Done.

+                     const base::FilePath& target,
+                     bool recreate) {
+  if (!source.empty() && base::Move(source, target) && recreate)
+    base::CreateDirectory(source);
+}
+
+void DeleteDirectoryWithSuffix(base::FilePath path) {

[grt (UTC plus 2), 2016/05/19 15:37:27]

const-ref

[grt (UTC plus 2), 2016/05/19 15:37:27]

DeleteAllRenamedUserDirectories?

[zmin, 2016/05/19 23:08:01]

Done.

[zmin, 2016/05/19 23:08:02]

Done.

+  if (path.empty())
+    return;
+  base::FilePath dir_name = path.DirName();
+  // Does not support root directory
+  if (dir_name == path)
+    return;
+
+  base::FilePath::StringType pattern =
+      path.BaseName().value() + L"*" + g_delete_suffix;
+  base::FileEnumerator enumerator(dir_name, false,
+                                  base::FileEnumerator::DIRECTORIES, pattern);
+  for (base::FilePath file = enumerator.Next(); !file.empty();

[grt (UTC plus 2), 2016/05/19 15:37:28]

file -> dir

[zmin, 2016/05/19 23:08:03]

Done.

+       file = enumerator.Next())

[grt (UTC plus 2), 2016/05/19 15:37:27]

nit: braces since the for() spans multiple lines

[zmin, 2016/05/19 23:08:02]

Done.

+    base::DeleteFile(file, true);
+}
+
+void RemoveUnusedUserProfiles() {
+  base::FilePath user_data_dir;
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  DeleteDirectoryWithSuffix(user_data_dir);
+  DeleteDirectoryWithSuffix(GetDiskCacheDir());
+}
+
+}  // namespace
+
+void ResetUserProfilesAfterDowngrade() {
+  base::FilePath user_data_dir;
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  base::Version current_version(CHROME_VERSION_STRING);

[grt (UTC plus 2), 2016/05/19 15:37:28]

CHROME_VERSION_STRING -> chrome::kChromeVersion

[zmin, 2016/05/19 23:08:02]

Done.

+
+  if (GetDowngradeVersion() > current_version) {

[grt (UTC plus 2), 2016/05/19 15:37:27]

  if (GetDowngradeVersion() > current_version &&
      GetLastChromeVersion(user_data_dir) > current_version) {

+    base::FilePath disk_cache_dir(GetDiskCacheDir());

[grt (UTC plus 2), 2016/05/19 15:37:28]

keep locals in the narrowest scope possible
(https://google.github.io/styleguide/cppguide.html#Local_Variables)

[zmin, 2016/05/19 23:08:02]

More comments added

+    // Without the browser process singleton protection, the directory may be
+    // copied twice.
+    base::FilePath temp_disk_cache_dir(GetTempDirNameForDelete(disk_cache_dir));
+    base::FilePath temp_user_data_dir(GetTempDirNameForDelete(user_data_dir));
+    if (GetLastChromeVersion(user_data_dir) > current_version) {
+      // We don't have to recreate |disk_cache_dir| because it'll be initialized

[grt (UTC plus 2), 2016/05/19 15:37:27]

while use of personal pronouns isn't banned (it was discussed here:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/-wTxR...),
i am of the opinion that comments are usually more clear without "we" and such.
for example:
      // Do not recreate the disk cache dir as it will be initialized later.

[zmin, 2016/05/19 23:08:02]

Done.

+      // later.
+      RenameDirectory(disk_cache_dir, temp_disk_cache_dir, false);
+      RenameDirectory(user_data_dir, temp_user_data_dir, true);
+    }
+  }
+  base::WriteFile(GetLastChromeVersionFile(user_data_dir),
+                  current_version.GetString().c_str(),

[grt (UTC plus 2), 2016/05/19 15:37:28]

stash current_version.GetString() in a local var so it isn't created twice for
this call.

better yet, use the original string:
WriteFile(..., chrome::kChromeVersion, strlen(chrome::kChromeVersion));

[grt (UTC plus 2), 2016/05/19 15:37:28]

c_str() -> data() since you don't write the string terminator

[zmin, 2016/05/19 23:08:02]

Done.

+                  current_version.GetString().size());
+}
+
+void RemoveUnusedUserProfilesInBlockingPool() {
+  content::BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownBehavior(
+      FROM_HERE, base::Bind(&RemoveUnusedUserProfiles),
+      base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+}