Post-cleanup settings reset.

chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc

Index: chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc
diff --git a/chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc b/chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..246969c0d408693aff0b5b1079fcfef8d0d937fc
--- /dev/null
+++ b/chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc
@@ -0,0 +1,228 @@
+// 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/settings_resetter_win.h"
+
+#include <algorithm>
+#include <iterator>
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "base/barrier_closure.h"
+#include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "base/win/registry.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profile_resetter/profile_resetter.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/common/pref_names.h"
+#include "components/chrome_cleaner/public/constants/constants.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace safe_browsing {
+
+namespace {
+
+// Manages post-cleanup settings reset for a list of profiles.
+// An instance of this class is created by ResetPostCleanupSettingsIfTagged()
+// and will self-delete once all profiles in the list have been reset.
+class SettingsResetter : public base::RefCounted<SettingsResetter> {
+ public:
+  SettingsResetter(std::vector<Profile*> profiles_to_reset,

[alito, 2017/06/02 05:32:41]

Under what circumstances could we end up with  multiple profiles that have been
tagged for reset?

[ftirelo, 2017/06/02 21:20:57]

Added a comment to ResetPostCleanupSettingsIfTagged(), since that is the public
method. Please let me know if you think we should also document this here.

+                   std::unique_ptr<SettingsResetterDelegate> delegate,
+                   base::OnceClosure done_callback);
+  virtual ~SettingsResetter();
+
+  // Resets settings of |profile_| and invokes |done_callback_| when done.

[alito, 2017/06/02 05:32:41]

profile_ -> profiles_to_reset_

[ftirelo, 2017/06/02 21:20:57]

Done.

+  void Run();
+
+ private:
+  // Resets settings for |profile| according to default values given by
+  // |master_settings|. Used as a callback for
+  // DefaultSettingsFetcher::FetchDefaultSettings().
+  void OnFetchCompleted(
+      Profile* profile,
+      std::unique_ptr<BrandcodedDefaultSettings> master_settings);
+
+  // Removes the settings reset tag for |profile|. If there are no more
+  // profiles to reset, invokes |done_callback_| and deletes this object.
+  void OnResetCompleted(Profile* profile);
+
+  // The profile to be reset.

[alito, 2017/06/02 05:32:41]

nit: profile -> profiles

[ftirelo, 2017/06/02 21:20:57]

Done.

+  std::vector<Profile*> profiles_to_reset_;
+
+  // The number of profiles that need to be reset.
+  int num_pending_resets_;
+
+  // The callback to be invoked once settings reset completes.
+  base::OnceClosure done_callback_;
+
+  std::unique_ptr<SettingsResetterDelegate> delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(SettingsResetter);
+};
+
+SettingsResetter::SettingsResetter(
+    std::vector<Profile*> profiles_to_reset,
+    std::unique_ptr<SettingsResetterDelegate> delegate,
+    base::OnceClosure done_callback)
+    : done_callback_(std::move(done_callback)),
+      profiles_to_reset_(std::move(profiles_to_reset)),
+      num_pending_resets_(profiles_to_reset_.size()),
+      delegate_(std::move(delegate)) {
+  DCHECK_LT(0, num_pending_resets_);
+  DCHECK(done_callback_);
+  DCHECK(delegate_);
+}
+
+SettingsResetter::~SettingsResetter() {
+  DCHECK(!done_callback_);

[alito, 2017/06/02 05:32:41]

Worth adding a DCHECK also for num_pending_resets_?

[ftirelo, 2017/06/02 21:20:57]

Done.

+}
+
+void SettingsResetter::Run() {
+  for (Profile* profile : profiles_to_reset_) {
+    delegate_->FetchDefaultSettings(base::Bind(
+        &SettingsResetter::OnFetchCompleted, base::Unretained(this), profile));

[alito, 2017/06/02 05:32:41]

How does the SettingsResetter object keep itself alive beyond the call to Run()?

[ftirelo, 2017/06/02 21:20:57]

My previous implementation deleted the object once num_pending_resets reached 0.
Started using RefCounted pointer instead, because tests were crashing after the
vector got out of scope before this method completed, but forgot to update the
Unretained. Fixed now.

+  }
+}
+
+void SettingsResetter::OnFetchCompleted(
+    Profile* profile,
+    std::unique_ptr<BrandcodedDefaultSettings> master_settings) {
+  static const ProfileResetter::ResettableFlags kSettingsToReset =
+      ProfileResetter::DEFAULT_SEARCH_ENGINE | ProfileResetter::HOMEPAGE |
+      ProfileResetter::EXTENSIONS | ProfileResetter::STARTUP_PAGES |
+      ProfileResetter::SHORTCUTS;
+
+  delegate_->GetProfileResetter(profile)->Reset(
+      kSettingsToReset, std::move(master_settings),
+      base::Bind(&SettingsResetter::OnResetCompleted, base::Unretained(this),
+                 profile));
+}
+
+void SettingsResetter::OnResetCompleted(Profile* profile) {
+  DCHECK_LT(0, num_pending_resets_);
+
+  RecordPostCleanupSettingsResetPending(false, profile);
+
+  --num_pending_resets_;
+  if (!num_pending_resets_)
+    std::move(done_callback_).Run();
+}
+
+// Returns true if there is information of a completed cleanup in the registry.
+bool CleanupCompletedFromRegistry() {
+  base::string16 cleaner_key_path(
+      chrome_cleaner::kSoftwareRemovalToolRegistryKey);
+  cleaner_key_path.append(L"\\").append(chrome_cleaner::kCleanerSubKey);
+
+  base::win::RegKey srt_cleaner_key(HKEY_CURRENT_USER, cleaner_key_path.c_str(),
+                                    KEY_QUERY_VALUE);
+  int64_t ts_cleanup_started = 0L, ts_cleanup_completed = 0L;
+  return srt_cleaner_key.Valid() &&
+         srt_cleaner_key.ReadInt64(
+             chrome_cleaner::kCleanupStartedTimestampValueName,
+             &ts_cleanup_started) == ERROR_SUCCESS &&
+         srt_cleaner_key.ReadInt64(
+             chrome_cleaner::kCleanupConfirmedTimestampValueName,
+             &ts_cleanup_completed) == ERROR_SUCCESS &&
+         ts_cleanup_started < ts_cleanup_completed;
+}
+
+Profile* GetLastActiveProfile() {

[alito, 2017/06/02 05:32:41]

this doesn't seem to be used anywhere.

[ftirelo, 2017/06/02 21:20:57]

Done.

+  Browser* browser = chrome::FindLastActive();
+  return browser ? browser->profile() : nullptr;
+}
+
+}  // namespace
+
+void RegisterChromeCleanerProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  DCHECK(registry);
+  registry->RegisterBooleanPref(prefs::kChromeCleanerResetPending, false);
+}
+
+bool PostCleanupSettingsResetPending(Profile* profile) {
+  DCHECK(profile);
+  PrefService* prefs = profile->GetPrefs();
+  DCHECK(prefs);
+  return prefs->GetBoolean(prefs::kChromeCleanerResetPending);
+}
+
+void RecordPostCleanupSettingsResetPending(bool value, Profile* profile) {
+  DCHECK(profile);
+  PrefService* prefs = profile->GetPrefs();
+  DCHECK(prefs);
+  prefs->SetBoolean(prefs::kChromeCleanerResetPending, value);
+}
+
+void TagProfileForResetting(Profile* profile) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile);
+
+  if (base::FeatureList::IsEnabled(safe_browsing::kInBrowserCleanerUIFeature))

[alito, 2017/06/02 05:32:41]

why is this check performed here but not in
RecordPostCleanupSettingsResetPending (since that one is also publicly
available)?

[ftirelo, 2017/06/02 21:20:57]

Done.

+    RecordPostCleanupSettingsResetPending(true, profile);
+}
+
+bool CopyProfilesToReset(const std::vector<Profile*>& profiles,
+                         std::vector<Profile*>* profiles_to_reset) {
+  std::copy_if(profiles.begin(), profiles.end(),
+               std::back_inserter(*profiles_to_reset),
+               [](Profile* profile) -> bool {
+                 return PostCleanupSettingsResetPending(profile);
+               });
+  return profiles_to_reset->size() > 0;
+}
+
+void ResetPostCleanupSettingsIfTagged(
+    std::vector<Profile*> profiles,
+    base::OnceClosure continuation,
+    std::unique_ptr<SettingsResetterDelegate> delegate) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(continuation);
+
+  std::vector<Profile*> profiles_to_reset;
+  if (!base::FeatureList::IsEnabled(
+          safe_browsing::kInBrowserCleanerUIFeature) ||
+      !CopyProfilesToReset(profiles, &profiles_to_reset) ||
+      !CleanupCompletedFromRegistry()) {
+    std::move(continuation).Run();
+    return;
+  }
+
+  // This object will self-delete once |continuation| is invoked.
+  make_scoped_refptr(
+      new SettingsResetter(std::move(profiles_to_reset),
+                           delegate
+                               ? std::move(delegate)
+                               : base::MakeUnique<SettingsResetterDelegate>(),
+                           std::move(continuation)))
+      ->Run();
+}
+
+SettingsResetterDelegate::SettingsResetterDelegate() {}
+
+SettingsResetterDelegate::~SettingsResetterDelegate() {}
+
+void SettingsResetterDelegate::FetchDefaultSettings(
+    DefaultSettingsFetcher::SettingsCallback callback) {
+  DefaultSettingsFetcher::FetchDefaultSettings(std::move(callback));
+}
+
+std::unique_ptr<ProfileResetter> SettingsResetterDelegate::GetProfileResetter(
+    Profile* profile) {
+  return base::MakeUnique<ProfileResetter>(profile);
+}
+
+}  // namespace safe_browsing