Enforces order for settings reset

This fixes a weird interaction between to tasks involving settings reset:
 - Post-cleanup settings reset, which can happen at start up after the
   Chrome Cleanup Tool successfully completed (for example, it may have
   required a reboot);
 - Settings reset prompt, which will check if user settings have been
   hijacked and offer the user to reset it to its default values.

In case both features are enabled, we should only check the latter after
completing the former, so that we will not prompt users to reset their
settings if they have already agreed on resetting them post-cleanup.

BUG=690020

Review-Url: https://codereview.chromium.org/2971183002
Cr-Commit-Position: refs/heads/master@{#485274}

chrome/browser/chrome_browser_main_win.cc

@@ -231,6 +231,16 @@ void SetupModuleDatabase(
                  base::Unretained(module_database)));
 }
 
+void MaybePostSettingsResetPrompt() {
+  if (base::FeatureList::IsEnabled(safe_browsing::kSettingsResetPrompt)) {
+    content::BrowserThread::PostAfterStartupTask(
+        FROM_HERE,
+        content::BrowserThread::GetTaskRunnerForThread(
+            content::BrowserThread::UI),
+        base::Bind(safe_browsing::MaybeShowSettingsResetPromptWithDelay));
+  }
+}
+
 }  // namespace
 
 void ShowCloseBrowserFirstMessageBox() {
@@ -376,24 +386,20 @@ void ChromeBrowserMainPartsWin::PostBrowserStart() {
   InitializeChromeElf();
 
   // Reset settings for the current profile if it's tagged to be reset after a
-  // complete run of the Chrome Cleanup tool.
+  // complete run of the Chrome Cleanup tool. If post-cleanup settings reset is
+  // enabled, we delay checks for settings reset prompt until the scheduled
+  // reset is finished.
   if (safe_browsing::PostCleanupSettingsResetter::IsEnabled()) {
     // Using last opened profiles, because we want to find reset the profile
     // that was open in the last Chrome run, which may not be open yet in
     // the current run.
     safe_browsing::PostCleanupSettingsResetter().ResetTaggedProfiles(
         g_browser_process->profile_manager()->GetLastOpenedProfiles(),
-        base::BindOnce(&base::DoNothing),
+        base::BindOnce(&MaybePostSettingsResetPrompt),
         base::MakeUnique<
             safe_browsing::PostCleanupSettingsResetter::Delegate>());
-  }
-
-  if (base::FeatureList::IsEnabled(safe_browsing::kSettingsResetPrompt)) {
-    content::BrowserThread::PostAfterStartupTask(
-        FROM_HERE,
-        content::BrowserThread::GetTaskRunnerForThread(
-            content::BrowserThread::UI),
-        base::Bind(safe_browsing::MaybeShowSettingsResetPromptWithDelay));
+  } else {
+    MaybePostSettingsResetPrompt();
   }
 
   // Record UMA data about whether the fault-tolerant heap is enabled.

chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_dependency_browsertest_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 <memory>
+#include <tuple>
+
+#include "base/strings/string_util.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
+#include "chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.h"
+#include "chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_test_utils.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace safe_browsing {
+namespace {
+
+using ::testing::InvokeWithoutArgs;
+using ::testing::StrictMock;
+
+class MockSettingsResetPromptDelegate : public SettingsResetPromptDelegate {
+ public:
+  MOCK_CONST_METHOD0(ShowSettingsResetPromptWithDelay, void());
+};
+
+// Test params: in_browser_cleaner_ui_enabled, settings_reset_prompt_enabled,
+//              representing if each feature is enabled.
+class SettingsResetDependencyTest
+    : public InProcessBrowserTest,
+      public ::testing::WithParamInterface<std::tuple<bool, bool>> {
+ public:
+  void SetUpInProcessBrowserTestFixture() override {
+    SetSettingsResetPromptDelegate(&delegate_);
+
+    std::tie(in_browser_cleaner_ui_enabled_, settings_reset_prompt_enabled_) =
+        GetParam();
+
+    std::vector<base::StringPiece> enabled_features;
+    std::vector<base::StringPiece> disabled_features;
+    if (in_browser_cleaner_ui_enabled_) {
+      enabled_features.push_back(kInBrowserCleanerUIFeature.name);
+    } else {
+      disabled_features.push_back(kInBrowserCleanerUIFeature.name);
+    }
+
+    if (settings_reset_prompt_enabled_) {
+      enabled_features.push_back(kSettingsResetPrompt.name);
+      EXPECT_CALL(delegate_, ShowSettingsResetPromptWithDelay())
+          .WillOnce(
+              InvokeWithoutArgs([this] { reset_prompt_checked_ = true; }));
+    } else {
+      disabled_features.push_back(kSettingsResetPrompt.name);
+    }
+
+    scoped_feature_list_.InitFromCommandLine(
+        base::JoinString(enabled_features, ","),
+        base::JoinString(disabled_features, ","));
+  }
+
+  void TearDownInProcessBrowserTestFixture() override {
+    SetSettingsResetPromptDelegate(nullptr);
+  }
+
+ protected:
+  bool in_browser_cleaner_ui_enabled_;
+  bool settings_reset_prompt_enabled_;
+
+  bool reset_prompt_checked_ = false;
+
+  StrictMock<MockSettingsResetPromptDelegate> delegate_;
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_P(SettingsResetDependencyTest,
+                       PromptAfterPostCleanupReset) {
+  if (settings_reset_prompt_enabled_) {
+    while (!reset_prompt_checked_)
+      base::RunLoop().RunUntilIdle();
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(Default,
+                        SettingsResetDependencyTest,
+                        ::testing::Combine(::testing::Bool(),
+                                           ::testing::Bool()));
+
+}  // namespace
+}  // namespace safe_browsing

chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc

@@ -107,6 +107,36 @@ void MaybeShowSettingsResetPrompt(
       base::Bind(&TryToShowSettingsResetPrompt, base::Passed(&model)));
 }
 
+class SettingsResetPromptDelegateImpl : public SettingsResetPromptDelegate {
+ public:
+  SettingsResetPromptDelegateImpl();
+  ~SettingsResetPromptDelegateImpl() override;
+
+  void ShowSettingsResetPromptWithDelay() const override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SettingsResetPromptDelegateImpl);
+};
+
+SettingsResetPromptDelegateImpl::SettingsResetPromptDelegateImpl() = default;
+
+SettingsResetPromptDelegateImpl::~SettingsResetPromptDelegateImpl() = default;
+
+void SettingsResetPromptDelegateImpl::ShowSettingsResetPromptWithDelay() const {
+  std::unique_ptr<SettingsResetPromptConfig> config =
+      SettingsResetPromptConfig::Create();
+  if (!config)
+    return;
+
+  base::TimeDelta delay = config->delay_before_prompt();
+  content::BrowserThread::PostDelayedTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::BindOnce(MaybeShowSettingsResetPrompt, base::Passed(&config)),
+      delay);
+}
+
+SettingsResetPromptDelegate* g_settings_reset_prompt_delegate = nullptr;
+
 }  // namespace.
 
 SettingsResetPromptController::SettingsResetPromptController(
@@ -257,16 +287,19 @@ void SettingsResetPromptController::OnInteractionDone() {
 }
 
 void MaybeShowSettingsResetPromptWithDelay() {
-  std::unique_ptr<SettingsResetPromptConfig> config =
-      SettingsResetPromptConfig::Create();
-  if (!config)
-    return;
+  if (g_settings_reset_prompt_delegate) {
+    g_settings_reset_prompt_delegate->ShowSettingsResetPromptWithDelay();
+  } else {
+    SettingsResetPromptDelegateImpl().ShowSettingsResetPromptWithDelay();
+  }
+}
 
-  base::TimeDelta delay = config->delay_before_prompt();
-  content::BrowserThread::PostDelayedTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(MaybeShowSettingsResetPrompt, base::Passed(&config)),
-      delay);
+SettingsResetPromptDelegate::SettingsResetPromptDelegate() = default;
+
+SettingsResetPromptDelegate::~SettingsResetPromptDelegate() = default;
+
+void SetSettingsResetPromptDelegate(SettingsResetPromptDelegate* delegate) {
+  g_settings_reset_prompt_delegate = delegate;
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.h

@@ -83,6 +83,23 @@ class SettingsResetPromptController {
 // feature parameters.
 void MaybeShowSettingsResetPromptWithDelay();
 
+// Delegate for MaybeShowSettingsResetPromptWithDelay() that can be overriden
+// by tests that only want to check if the flow for the settings reset prompt
+// will be initiated.
+class SettingsResetPromptDelegate {
+ public:
+  SettingsResetPromptDelegate();
+  virtual ~SettingsResetPromptDelegate();
+
+  virtual void ShowSettingsResetPromptWithDelay() const = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SettingsResetPromptDelegate);
+};
+
+// Sets the global SettingsResetPromptDelegate, usually for testing.
+void SetSettingsResetPromptDelegate(SettingsResetPromptDelegate* delegate);
+
 }  // namespace safe_browsing
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SETTINGS_RESET_PROMPT_SETTINGS_RESET_PROMPT_CONTROLLER_H_

chrome/test/BUILD.gn

@@ -2001,6 +2001,7 @@ test("browser_tests") {
         "../browser/extensions/window_open_apitest.cc",
         "../browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc",
         "../browser/safe_browsing/settings_reset_prompt/default_settings_fetcher_browsertest.cc",
+        "../browser/safe_browsing/settings_reset_prompt/settings_reset_dependency_browsertest_win.cc",
         "../browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model_browsertest_win.cc",
         "../browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_test_utils.cc",
         "../browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_test_utils.h",