[Passwords] Fix a crash in HttpCredentialCleaner

HttpCredentialCleaner tried to use the PrefService in a posted task.
This caused a use-after-free when the DestroyProfileOnBrowserClose
flag was enabled.

Make CredentialsCleanerRunner a KeyedService, so its lifetime (and its
attached HttpCredentialCleaner) is tied to the Profile's lifetime,
letting us avoid the crash by using WeakPtrs.

Bug: 88586, 1141055
Change-Id: I77ebd7daa87257d562c9c12bf63681f16ffc2d10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2511073
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826437}

chrome/browser/BUILD.gn

@@ -1059,6 +1059,8 @@ static_library("browser") {
     "password_manager/chrome_biometric_authenticator.h",
     "password_manager/chrome_password_manager_client.cc",
     "password_manager/chrome_password_manager_client.h",
+    "password_manager/credentials_cleaner_runner_factory.cc",
+    "password_manager/credentials_cleaner_runner_factory.h",
     "password_manager/field_info_manager_factory.cc",
     "password_manager/field_info_manager_factory.h",
     "password_manager/password_store_factory.cc",

chrome/browser/password_manager/account_password_store_factory.cc

@@ -13,6 +13,8 @@
 #include "base/memory/weak_ptr.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/password_manager/credentials_cleaner_runner_factory.h"
+#include "chrome/browser/password_manager/password_store_utils.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -202,8 +204,10 @@ AccountPasswordStoreFactory::BuildServiceInstanceFor(
             ->GetNetworkContext();
       },
       profile);
-  password_manager_util::RemoveUselessCredentials(ps, profile->GetPrefs(), 60,
-                                                  network_context_getter);
+  password_manager_util::RemoveUselessCredentials(
+      CredentialsCleanerRunnerFactory::GetForProfile(profile), ps,
+      profile->GetPrefs(), base::TimeDelta::FromSeconds(60),
+      network_context_getter);
 
 #if !defined(OS_ANDROID)
   ps->SetUnsyncedCredentialsDeletionNotifier(

chrome/browser/password_manager/credentials_cleaner_runner_factory.cc

+// Copyright 2020 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/password_manager/credentials_cleaner_runner_factory.h"
+
+#include "chrome/browser/profiles/profile.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/password_manager/core/browser/credentials_cleaner_runner.h"
+#include "content/public/browser/browser_context.h"
+
+CredentialsCleanerRunnerFactory::CredentialsCleanerRunnerFactory()
+    : BrowserContextKeyedServiceFactory(
+          "CredentialsCleanerRunner",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+CredentialsCleanerRunnerFactory::~CredentialsCleanerRunnerFactory() = default;
+
+CredentialsCleanerRunnerFactory*
+CredentialsCleanerRunnerFactory::GetInstance() {
+  static base::NoDestructor<CredentialsCleanerRunnerFactory> instance;
+  return instance.get();
+}
+
+password_manager::CredentialsCleanerRunner*
+CredentialsCleanerRunnerFactory::GetForProfile(Profile* profile) {
+  return static_cast<password_manager::CredentialsCleanerRunner*>(
+      GetInstance()->GetServiceForBrowserContext(profile, true));
+}
+
+KeyedService* CredentialsCleanerRunnerFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  return new password_manager::CredentialsCleanerRunner();
+}

chrome/browser/password_manager/credentials_cleaner_runner_factory.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_PASSWORD_MANAGER_CREDENTIALS_CLEANER_RUNNER_FACTORY_H_
+#define CHROME_BROWSER_PASSWORD_MANAGER_CREDENTIALS_CLEANER_RUNNER_FACTORY_H_
+
+#include "base/no_destructor.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+
+namespace password_manager {
+class CredentialsCleanerRunner;
+}  // namespace password_manager
+
+namespace content {
+class BrowserContext;
+}  // namespace content
+
+class Profile;
+
+// Creates instances of CredentialsCleanerRunner per Profile.
+class CredentialsCleanerRunnerFactory
+    : public BrowserContextKeyedServiceFactory {
+ public:
+  static CredentialsCleanerRunnerFactory* GetInstance();
+  static password_manager::CredentialsCleanerRunner* GetForProfile(
+      Profile* profile);
+
+ private:
+  friend class base::NoDestructor<CredentialsCleanerRunnerFactory>;
+
+  CredentialsCleanerRunnerFactory();
+  ~CredentialsCleanerRunnerFactory() override;
+
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const override;
+};
+
+#endif  // CHROME_BROWSER_PASSWORD_MANAGER_CREDENTIALS_CLEANER_RUNNER_FACTORY_H_

chrome/browser/password_manager/password_store_factory.cc

@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/password_manager/credentials_cleaner_runner_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -164,8 +165,10 @@ PasswordStoreFactory::BuildServiceInstanceFor(
             ->GetNetworkContext();
       },
       profile);
-  password_manager_util::RemoveUselessCredentials(ps, profile->GetPrefs(), 60,
-                                                  network_context_getter);
+  password_manager_util::RemoveUselessCredentials(
+      CredentialsCleanerRunnerFactory::GetForProfile(profile), ps,
+      profile->GetPrefs(), base::TimeDelta::FromSeconds(60),
+      network_context_getter);
 
 #if defined(OS_WIN) || defined(OS_MAC) || \
     (defined(OS_LINUX) && !defined(OS_CHROMEOS))

components/password_manager/core/browser/credentials_cleaner_runner.cc

@@ -23,8 +23,8 @@ bool CredentialsCleanerRunner::HasPendingTasks() const {
 }
 
 void CredentialsCleanerRunner::StartCleaning() {
-  DCHECK(!cleaning_started_);
-  cleaning_started_ = true;
+  if (cleaning_in_progress_)
+    return;
   StartCleaningTask();
 }
 
@@ -36,13 +36,15 @@ void CredentialsCleanerRunner::CleaningCompleted() {
 }
 
 void CredentialsCleanerRunner::StartCleaningTask() {
-  if (!HasPendingTasks()) {
-    // Delete the runner if no more clean-up is scheduled.
-    delete this;
+  cleaning_in_progress_ = HasPendingTasks();
+  if (!cleaning_in_progress_)
     return;
-  }
 
   cleaning_tasks_queue_.front()->StartCleaning(this);
 }
 
+base::WeakPtr<CredentialsCleanerRunner> CredentialsCleanerRunner::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/credentials_cleaner_runner.h

@@ -9,6 +9,8 @@
 
 #include "base/containers/queue.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "components/keyed_service/core/keyed_service.h"
 #include "components/password_manager/core/browser/credentials_cleaner.h"
 
 namespace password_manager {
@@ -17,14 +19,15 @@ namespace password_manager {
 // order they are added. The runner is informed by the the clean-up tasks that
 // the clean-up is finished when a clean-up task calls CleaningCompleted. This
 // class will keep the clean-up object alive until the runner is notified that
-// the clean-up is finished.
+// the clean-up is finished, or until BrowserContext shutdown.
+//
 // Usage:
 // (1) Add cleaning tasks in the order the have to be executed.
 // (2) After all cleaning task are added call StartCleaning().
-// Important note: The object will delete itself once all clean-ups are done,
-// thus it should not be allocated on the stack. Having a non-public destructor
-// enforces this.
-class CredentialsCleanerRunner : public CredentialsCleaner::Observer {
+//
+// Use CredentialsCleanerRunnerFactory to create this object.
+class CredentialsCleanerRunner : public CredentialsCleaner::Observer,
+                                 public KeyedService {
  public:
   CredentialsCleanerRunner();
   ~CredentialsCleanerRunner() override;
@@ -40,13 +43,17 @@ class CredentialsCleanerRunner : public CredentialsCleaner::Observer {
   // CredentialsCleaner::Observer:
   void CleaningCompleted() override;
 
+  base::WeakPtr<CredentialsCleanerRunner> GetWeakPtr();
+
  private:
   void StartCleaningTask();
 
-  bool cleaning_started_ = false;
+  bool cleaning_in_progress_ = false;
 
   base::queue<std::unique_ptr<CredentialsCleaner>> cleaning_tasks_queue_;
 
+  base::WeakPtrFactory<CredentialsCleanerRunner> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(CredentialsCleanerRunner);
 };
 

components/password_manager/core/browser/credentials_cleaner_runner_unittest.cc

@@ -25,44 +25,20 @@ class MockCredentialsCleaner : public CredentialsCleaner {
   DISALLOW_COPY_AND_ASSIGN(MockCredentialsCleaner);
 };
 
-class DestructionCheckableCleanerRunner : public CredentialsCleanerRunner {
- public:
-  // |object_destroyed| is used to inform caller that the object destroyed
-  // itself.
-  explicit DestructionCheckableCleanerRunner(bool* object_destroyed)
-      : object_destroyed_(object_destroyed) {}
-
- private:
-  ~DestructionCheckableCleanerRunner() override { *object_destroyed_ = true; }
-
-  // The class will assign true to the pointee on destruction.
-  bool* object_destroyed_;
-};
-
 class CredentialsCleanerRunnerTest : public ::testing::Test {
  public:
   CredentialsCleanerRunnerTest() = default;
-
-  ~CredentialsCleanerRunnerTest() override { EXPECT_TRUE(runner_destroyed_); }
+  ~CredentialsCleanerRunnerTest() override = default;
 
  protected:
-  CredentialsCleanerRunner* GetRunner() { return cleaning_tasks_runner_; }
+  CredentialsCleanerRunner* GetRunner() { return &cleaning_tasks_runner_; }
 
  private:
-  bool runner_destroyed_ = false;
-
-  // This is a non-owning pointer, because the runner will delete itself.
-  DestructionCheckableCleanerRunner* cleaning_tasks_runner_ =
-      new DestructionCheckableCleanerRunner(&runner_destroyed_);
+  CredentialsCleanerRunner cleaning_tasks_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(CredentialsCleanerRunnerTest);
 };
 
-// This test would fail if the object doesn't delete itself.
-TEST_F(CredentialsCleanerRunnerTest, EmptyTasks) {
-  GetRunner()->StartCleaning();
-}
-
 // In this test we check that credential clean-ups runner executes the clean-up
 // tasks in the order they were added.
 TEST_F(CredentialsCleanerRunnerTest, NonEmptyTasks) {
@@ -84,6 +60,61 @@ TEST_F(CredentialsCleanerRunnerTest, NonEmptyTasks) {
   cleaning_tasks_runner->StartCleaning();
   for (int i = 0; i < kCleanersCount; ++i)
     cleaning_tasks_runner->CleaningCompleted();
+
+  EXPECT_FALSE(cleaning_tasks_runner->HasPendingTasks());
+}
+
+// In this test we check that StartCleaning() can be called again after the
+// first one finished.
+TEST_F(CredentialsCleanerRunnerTest, CanBeCalledAgainAfterFinished) {
+  auto* cleaning_tasks_runner = GetRunner();
+
+  auto cleaner1 = std::make_unique<MockCredentialsCleaner>();
+  EXPECT_CALL(*cleaner1, NeedsCleaning).WillOnce(::testing::Return(true));
+
+  auto cleaner2 = std::make_unique<MockCredentialsCleaner>();
+  EXPECT_CALL(*cleaner2, NeedsCleaning).WillOnce(::testing::Return(true));
+
+  ::testing::InSequence dummy;
+  EXPECT_CALL(*cleaner1, StartCleaning(cleaning_tasks_runner));
+  EXPECT_CALL(*cleaner2, StartCleaning(cleaning_tasks_runner));
+
+  cleaning_tasks_runner->MaybeAddCleaningTask(std::move(cleaner1));
+  cleaning_tasks_runner->StartCleaning();
+  cleaning_tasks_runner->CleaningCompleted();  // cleaner1
+
+  cleaning_tasks_runner->MaybeAddCleaningTask(std::move(cleaner2));
+  cleaning_tasks_runner->StartCleaning();
+  cleaning_tasks_runner->CleaningCompleted();  // cleaner2
+
+  EXPECT_FALSE(cleaning_tasks_runner->HasPendingTasks());
+}
+
+// In this test we check that StartCleaning() can be called again before the
+// first one finished.
+TEST_F(CredentialsCleanerRunnerTest, CanBeCalledAgainBeforeFinished) {
+  auto* cleaning_tasks_runner = GetRunner();
+
+  auto cleaner1 = std::make_unique<MockCredentialsCleaner>();
+  EXPECT_CALL(*cleaner1, NeedsCleaning).WillOnce(::testing::Return(true));
+
+  auto cleaner2 = std::make_unique<MockCredentialsCleaner>();
+  EXPECT_CALL(*cleaner2, NeedsCleaning).WillOnce(::testing::Return(true));
+
+  ::testing::InSequence dummy;
+  EXPECT_CALL(*cleaner1, StartCleaning(cleaning_tasks_runner));
+  EXPECT_CALL(*cleaner2, StartCleaning(cleaning_tasks_runner));
+
+  cleaning_tasks_runner->MaybeAddCleaningTask(std::move(cleaner1));
+  cleaning_tasks_runner->StartCleaning();
+
+  cleaning_tasks_runner->MaybeAddCleaningTask(std::move(cleaner2));
+  cleaning_tasks_runner->StartCleaning();
+
+  cleaning_tasks_runner->CleaningCompleted();  // cleaner1
+  cleaning_tasks_runner->CleaningCompleted();  // cleaner2
+
+  EXPECT_FALSE(cleaning_tasks_runner->HasPendingTasks());
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/http_credentials_cleaner.cc

@@ -52,7 +52,8 @@ void HttpCredentialCleaner::OnGetPasswordStoreResults(
       PostHSTSQueryForHostAndNetworkContext(
           origin, network_context_getter_.Run(),
           base::BindOnce(&HttpCredentialCleaner::OnHSTSQueryResult,
-                         base::Unretained(this), std::move(form), form_key));
+                         weak_ptr_factory_.GetWeakPtr(), std::move(form),
+                         form_key));
       ++total_http_credentials_;
     } else {  // HTTPS
       https_credentials_map_[form_key].insert(form->password_value);

components/password_manager/core/browser/http_credentials_cleaner.h

@@ -14,6 +14,7 @@
 
 #include "base/containers/flat_set.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "components/password_manager/core/browser/credentials_cleaner.h"
 #include "components/password_manager/core/browser/hsts_query.h"
 #include "components/password_manager/core/browser/password_form.h"
@@ -123,6 +124,8 @@ class HttpCredentialCleaner : public PasswordStoreConsumer,
   // credentials were processed.
   size_t total_http_credentials_ = 0;
 
+  base::WeakPtrFactory<HttpCredentialCleaner> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(HttpCredentialCleaner);
 };
 

components/password_manager/core/browser/password_manager_util.cc

@@ -172,13 +172,13 @@ void UserTriggeredManualGenerationFromContextMenu(
 // TODO(http://crbug.com/890318): Add unitests to check cleaners are correctly
 // created.
 void RemoveUselessCredentials(
+    password_manager::CredentialsCleanerRunner* cleaning_tasks_runner,
     scoped_refptr<password_manager::PasswordStore> store,
     PrefService* prefs,
-    int delay_in_seconds,
+    base::TimeDelta delay,
     base::RepeatingCallback<network::mojom::NetworkContext*()>
         network_context_getter) {
-  auto cleaning_tasks_runner =
-      std::make_unique<password_manager::CredentialsCleanerRunner>();
+  DCHECK(cleaning_tasks_runner);
 
 #if !defined(OS_IOS)
   // Can be null for some unittests.
@@ -196,8 +196,8 @@ void RemoveUselessCredentials(
         FROM_HERE,
         base::BindOnce(
             &password_manager::CredentialsCleanerRunner::StartCleaning,
-            base::Unretained(cleaning_tasks_runner.release())),
-        base::TimeDelta::FromSeconds(delay_in_seconds));
+            cleaning_tasks_runner->GetWeakPtr()),
+        delay);
   }
 }
 

components/password_manager/core/browser/password_manager_util.h

@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/strings/string16.h"
+#include "base/time/time.h"
 #include "components/password_manager/core/browser/password_form.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_store.h"
@@ -22,6 +23,7 @@ class NetworkContext;
 }  // namespace network
 
 namespace password_manager {
+class CredentialsCleanerRunner;
 class PasswordManagerDriver;
 class PasswordManagerClient;
 }  // namespace password_manager
@@ -87,9 +89,10 @@ void UserTriggeredManualGenerationFromContextMenu(
 // HSTS query is not supported. |network_context_getter| is always null for iOS
 // and it can also be null for some unittests.
 void RemoveUselessCredentials(
+    password_manager::CredentialsCleanerRunner* cleaning_tasks_runner,
     scoped_refptr<password_manager::PasswordStore> store,
     PrefService* prefs,
-    int delay_in_seconds,
+    base::TimeDelta delay,
     base::RepeatingCallback<network::mojom::NetworkContext*()>
         network_context_getter);
 

components/password_manager/core/browser/password_store.cc

@@ -681,6 +681,9 @@ void PasswordStore::ScheduleEnterprisePasswordURLUpdate() {
 
 PasswordStore::~PasswordStore() {
   DCHECK(shutdown_called_);
+  // PasswordSyncBridge should delete on the same sequence where it was created.
+  if (sync_bridge_)
+    background_task_runner_->DeleteSoon(FROM_HERE, std::move(sync_bridge_));
 }
 
 scoped_refptr<base::SequencedTaskRunner>

ios/chrome/browser/passwords/BUILD.gn

@@ -7,6 +7,8 @@ import("//ios/web/js_compile.gni")
 source_set("passwords") {
   configs += [ "//build/config/compiler:enable_arc" ]
   sources = [
+    "credentials_cleaner_runner_factory.cc",
+    "credentials_cleaner_runner_factory.h",
     "ios_chrome_bulk_leak_check_service_factory.cc",
     "ios_chrome_bulk_leak_check_service_factory.h",
     "ios_chrome_change_password_url_service_factory.cc",

ios/chrome/browser/passwords/credentials_cleaner_runner_factory.cc

+// Copyright 2020 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 "ios/chrome/browser/passwords/credentials_cleaner_runner_factory.h"
+
+#include "components/keyed_service/ios/browser_state_dependency_manager.h"
+#include "components/password_manager/core/browser/credentials_cleaner_runner.h"
+#include "ios/web/public/browser_state.h"
+
+CredentialsCleanerRunnerFactory::CredentialsCleanerRunnerFactory()
+    : BrowserStateKeyedServiceFactory(
+          "CredentialsCleanerRunner",
+          BrowserStateDependencyManager::GetInstance()) {}
+
+CredentialsCleanerRunnerFactory::~CredentialsCleanerRunnerFactory() = default;
+
+CredentialsCleanerRunnerFactory*
+CredentialsCleanerRunnerFactory::GetInstance() {
+  static base::NoDestructor<CredentialsCleanerRunnerFactory> instance;
+  return instance.get();
+}
+
+password_manager::CredentialsCleanerRunner*
+CredentialsCleanerRunnerFactory::GetForBrowserState(
+    web::BrowserState* browser_state) {
+  return static_cast<password_manager::CredentialsCleanerRunner*>(
+      GetInstance()->GetServiceForBrowserState(browser_state, true));
+}
+
+std::unique_ptr<KeyedService>
+CredentialsCleanerRunnerFactory::BuildServiceInstanceFor(
+    web::BrowserState* browser_state) const {
+  return std::make_unique<password_manager::CredentialsCleanerRunner>();
+}

ios/chrome/browser/passwords/credentials_cleaner_runner_factory.h

+// Copyright 2020 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.
+
+#ifndef IOS_CHROME_BROWSER_PASSWORDS_CREDENTIALS_CLEANER_RUNNER_FACTORY_H_
+#define IOS_CHROME_BROWSER_PASSWORDS_CREDENTIALS_CLEANER_RUNNER_FACTORY_H_
+
+#include "base/no_destructor.h"
+#include "components/keyed_service/ios/browser_state_keyed_service_factory.h"
+
+namespace password_manager {
+class CredentialsCleanerRunner;
+}  // namespace password_manager
+
+// Creates instances of CredentialsCleanerRunner per Profile.
+class CredentialsCleanerRunnerFactory : public BrowserStateKeyedServiceFactory {
+ public:
+  static CredentialsCleanerRunnerFactory* GetInstance();
+  static password_manager::CredentialsCleanerRunner* GetForBrowserState(
+      web::BrowserState* browser_state);
+
+ private:
+  friend class base::NoDestructor<CredentialsCleanerRunnerFactory>;
+
+  CredentialsCleanerRunnerFactory();
+  ~CredentialsCleanerRunnerFactory() override;
+
+  std::unique_ptr<KeyedService> BuildServiceInstanceFor(
+      web::BrowserState* browser_state) const override;
+};
+
+#endif  // IOS_CHROME_BROWSER_PASSWORDSCREDENTIALS_CLEANER_RUNNER_FACTORY_H_

ios/chrome/browser/passwords/ios_chrome_password_store_factory.cc

@@ -24,6 +24,7 @@
 #include "ios/chrome/browser/application_context.h"
 #include "ios/chrome/browser/browser_state/browser_state_otr_helper.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#include "ios/chrome/browser/passwords/credentials_cleaner_runner_factory.h"
 #include "ios/chrome/browser/sync/profile_sync_service_factory.h"
 #include "ios/chrome/browser/webdata_services/web_data_service_factory.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -99,8 +100,9 @@ IOSChromePasswordStoreFactory::BuildServiceInstanceFor(
     return nullptr;
   }
   password_manager_util::RemoveUselessCredentials(
-      store, ChromeBrowserState::FromBrowserState(context)->GetPrefs(), 60,
-      base::NullCallback());
+      CredentialsCleanerRunnerFactory::GetForBrowserState(context), store,
+      ChromeBrowserState::FromBrowserState(context)->GetPrefs(),
+      base::TimeDelta::FromSeconds(60), base::NullCallback());
   return store;
 }