[Passwords] Use base::ObserverList in FormFetcher

In order to be more robust against changes to the observer list, this
change modifies FormFetcher to use a base::ObserverList instead of a
simple set and makes its consumers inherit from base::CheckedObserver.

This allows consumers to delete the form fetcher while being notified,
which previously would cause a use-after-free. A corresponding test is
added.

Bug: 1007556
Change-Id: I645526027e35be222479009199af6545de6a11f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1855985
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707783}

components/password_manager/core/browser/credential_manager_password_form_manager.cc

@@ -42,15 +42,7 @@ void CredentialManagerPasswordFormManager::OnFetchCompleted() {
   PasswordFormManager::OnFetchCompleted();
 
   CreatePendingCredentials();
-
-  // Notify the delegate. This might result in deleting |this|, while
-  // OnFetchCompleted is being called from FormFetcherImpl, owned by |this|. If
-  // done directly, once OnFetchCompleted returns, the FormFetcherImpl will be
-  // used after free. Therefore the call is posted to a separate task.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&CredentialManagerPasswordFormManager::NotifyDelegate,
-                     weak_factory_.GetWeakPtr()));
+  NotifyDelegate();
 }
 
 metrics_util::CredentialSourceType

components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc

@@ -106,34 +106,6 @@ class CredentialManagerPasswordFormManagerTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(CredentialManagerPasswordFormManagerTest);
 };
 
-// Test that aborting early does not cause use after free.
-TEST_F(CredentialManagerPasswordFormManagerTest, AbortEarly) {
-  auto saved_form = std::make_unique<PasswordForm>();
-  saved_form->password_value = base::ASCIIToUTF16("password");
-  MockDelegate delegate;
-  auto form_manager = std::make_unique<CredentialManagerPasswordFormManager>(
-      &client_, std::move(saved_form), &delegate,
-      std::make_unique<StubFormSaver>(), std::make_unique<FakeFormFetcher>());
-
-  auto deleter = [&form_manager]() { form_manager.reset(); };
-
-  // Simulate that the PasswordStore responded to the FormFetcher. As a result,
-  // |form_manager| should call the delegate's OnProvisionalSaveComplete, which
-  // in turn should delete |form_fetcher|.
-  EXPECT_CALL(delegate, OnProvisionalSaveComplete()).WillOnce(Invoke(deleter));
-  static_cast<FakeFormFetcher*>(form_manager->GetFormFetcher())
-      ->NotifyFetchCompleted();
-  // Check that |form_manager| was not deleted yet; doing so would have caused
-  // use after free during NotifyFetchCompleted.
-  EXPECT_TRUE(form_manager);
-
-  base::RunLoop().RunUntilIdle();
-
-  // Ultimately, |form_fetcher| should have been deleted. It just should happen
-  // after it finishes executing.
-  EXPECT_FALSE(form_manager);
-}
-
 // Ensure that GetCredentialSource is actually overriden and returns the proper
 // value.
 TEST_F(CredentialManagerPasswordFormManagerTest, GetCredentialSource) {

components/password_manager/core/browser/fake_form_fetcher.cc

@@ -19,11 +19,11 @@ FakeFormFetcher::FakeFormFetcher() = default;
 FakeFormFetcher::~FakeFormFetcher() = default;
 
 void FakeFormFetcher::AddConsumer(Consumer* consumer) {
-  consumers_.insert(consumer);
+  consumers_.AddObserver(consumer);
 }
 
 void FakeFormFetcher::RemoveConsumer(Consumer* consumer) {
-  consumers_.erase(consumer);
+  consumers_.RemoveObserver(consumer);
 }
 
 FormFetcher::State FakeFormFetcher::GetState() const {
@@ -77,8 +77,8 @@ void FakeFormFetcher::SetBlacklisted(bool is_blacklisted) {
 
 void FakeFormFetcher::NotifyFetchCompleted() {
   state_ = State::NOT_WAITING;
-  for (Consumer* consumer : consumers_)
-    consumer->OnFetchCompleted();
+  for (Consumer& consumer : consumers_)
+    consumer.OnFetchCompleted();
 }
 
 void FakeFormFetcher::Fetch() {

components/password_manager/core/browser/fake_form_fetcher.h

@@ -5,10 +5,10 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_FAKE_FORM_FETCHER_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_FAKE_FORM_FETCHER_H_
 
-#include <set>
 #include <vector>
 
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/form_fetcher.h"
 #include "components/password_manager/core/browser/statistics_table.h"
@@ -87,7 +87,7 @@ class FakeFormFetcher : public FormFetcher {
   std::unique_ptr<FormFetcher> Clone() override;
 
  private:
-  std::set<Consumer*> consumers_;
+  base::ObserverList<Consumer> consumers_;
   State state_ = State::NOT_WAITING;
   autofill::PasswordForm::Scheme scheme_ =
       autofill::PasswordForm::Scheme::kHtml;

components/password_manager/core/browser/form_fetcher.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/observer_list_types.h"
 #include "base/strings/string16.h"
 
 namespace autofill {
@@ -33,10 +34,8 @@ class FormFetcher {
   enum class State { WAITING, NOT_WAITING };
 
   // API to be implemented by classes which want the results from FormFetcher.
-  class Consumer {
+  class Consumer : public base::CheckedObserver {
    public:
-    virtual ~Consumer() = default;
-
     // FormFetcher calls this method every time the state changes from WAITING
     // to NOT_WAITING. It is now safe for consumers to call the accessor
     // functions for matches.

components/password_manager/core/browser/form_fetcher_impl.cc

@@ -79,14 +79,14 @@ FormFetcherImpl::~FormFetcherImpl() = default;
 
 void FormFetcherImpl::AddConsumer(FormFetcher::Consumer* consumer) {
   DCHECK(consumer);
-  consumers_.insert(consumer);
+  consumers_.AddObserver(consumer);
   if (state_ == State::NOT_WAITING)
     consumer->OnFetchCompleted();
 }
 
 void FormFetcherImpl::RemoveConsumer(FormFetcher::Consumer* consumer) {
-  size_t removed_consumers = consumers_.erase(consumer);
-  DCHECK_EQ(1u, removed_consumers);
+  DCHECK(consumers_.HasObserver(consumer));
+  consumers_.RemoveObserver(consumer);
 }
 
 FormFetcherImpl::State FormFetcherImpl::GetState() const {
@@ -239,8 +239,8 @@ void FormFetcherImpl::ProcessPasswordStoreResults(
       sort_matches_by_date_last_used_, &non_federated_same_scheme_,
       &best_matches_, &preferred_match_);
 
-  for (auto* consumer : consumers_)
-    consumer->OnFetchCompleted();
+  for (auto& consumer : consumers_)
+    consumer.OnFetchCompleted();
 }
 
 void FormFetcherImpl::SplitResults(

components/password_manager/core/browser/form_fetcher_impl.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "components/password_manager/core/browser/form_fetcher.h"
 #include "components/password_manager/core/browser/http_password_store_migrator.h"
 #include "components/password_manager/core/browser/password_store.h"
@@ -128,8 +129,9 @@ class FormFetcherImpl : public FormFetcher,
   // Statistics for the current domain.
   std::vector<InteractionsStats> interactions_stats_;
 
-  // Consumers of the fetcher, all are assumed to outlive |this|.
-  std::set<FormFetcher::Consumer*> consumers_;
+  // Consumers of the fetcher, all are assumed to either outlive |this| or
+  // remove themselves from the list during their destruction.
+  base::ObserverList<FormFetcher::Consumer> consumers_;
 
   // Indicates whether HTTP passwords should be migrated to HTTPS.
   const bool should_migrate_http_passwords_;

components/password_manager/core/browser/form_fetcher_impl_unittest.cc

@@ -60,6 +60,18 @@ class MockConsumer : public FormFetcher::Consumer {
   MOCK_METHOD0(OnFetchCompleted, void());
 };
 
+// MockConsumer that takes ownership of the FormFetcher itself.
+class MockOwningConsumer : public FormFetcher::Consumer {
+ public:
+  explicit MockOwningConsumer(std::unique_ptr<FormFetcher> form_fetcher)
+      : form_fetcher_(std::move(form_fetcher)) {}
+
+  MOCK_METHOD0(OnFetchCompleted, void());
+
+ private:
+  std::unique_ptr<FormFetcher> form_fetcher_;
+};
+
 class NameFilter : public StubCredentialsFilter {
  public:
   // This class filters out all credentials which have |name| as
@@ -704,6 +716,29 @@ TEST_P(FormFetcherImplTest, RemoveConsumer) {
       std::vector<std::unique_ptr<PasswordForm>>());
 }
 
+// Check that destroying the fetcher while notifying its consumers is handled
+// gracefully.
+TEST_P(FormFetcherImplTest, DestroyFetcherFromConsumer) {
+  Fetch();
+
+  // Construct an owning consumer and register it and a regular consumer.
+  auto* form_fetcher = form_fetcher_.get();
+  auto owning_consumer =
+      std::make_unique<MockOwningConsumer>(std::move(form_fetcher_));
+  form_fetcher->AddConsumer(owning_consumer.get());
+  form_fetcher->AddConsumer(&consumer_);
+
+  // Destroy the form fetcher when notifying the owning consumer. Make sure the
+  // second consumer does not get notified anymore.
+  EXPECT_CALL(*owning_consumer, OnFetchCompleted).WillOnce([&owning_consumer] {
+    owning_consumer.reset();
+  });
+
+  EXPECT_CALL(consumer_, OnFetchCompleted).Times(0);
+  form_fetcher->OnGetPasswordStoreResults(
+      std::vector<std::unique_ptr<PasswordForm>>());
+}
+
 INSTANTIATE_TEST_SUITE_P(, FormFetcherImplTest, testing::Values(false, true));
 
 }  // namespace password_manager