Avoid calling a removed observer in PersonalDataManager

PersonalDataManager::NotifyPersonalDataObserver will call
OnPersonalDataChanged and often OnPersonalDataFinishedProfileTasks
on PersonalDataManagerObserver.

Trouble is that it does this in the same ObserverList loop.

So if PersonalDataManagerObserver implementation of OnPersonalDataChanged
calls PersonalDataManager::RemoveObserver(this) then it will still
be called on OnPersonalDataFinishedProfileTasks.

Worse, if PersonalDataManagerObserver deleted itself after removing
itself as an observer you now have a use-after-free calling a virtual
method on a destroyed object.

There are not currently any PersonalDataManagerObserver implementations
that I can find that has this problem.

Bug: 959172
Change-Id: I2bb0a625f5c3a847c5d035ccc57b5fdb349366b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1594529
Commit-Queue: Joel Klinghed <the_jk@opera.com>
Reviewed-by: Parastoo Geranmayeh <parastoog@google.com>
Cr-Commit-Position: refs/heads/master@{#656489}

components/autofill/core/browser/personal_data_manager.cc

@@ -2038,7 +2038,11 @@ void PersonalDataManager::NotifyPersonalDataObserver() {
   bool profile_changes_are_ongoing = ProfileChangesAreOngoing();
   for (PersonalDataManagerObserver& observer : observers_) {
     observer.OnPersonalDataChanged();
-    if (!profile_changes_are_ongoing) {
+  }
+  if (!profile_changes_are_ongoing) {
+    // Call OnPersonalDataFinishedProfileTasks in a separate loop as
+    // the observers might have removed themselves in OnPersonalDataChanged
+    for (PersonalDataManagerObserver& observer : observers_) {
       observer.OnPersonalDataFinishedProfileTasks();
     }
   }

components/autofill/core/browser/personal_data_manager_unittest.cc

@@ -7853,4 +7853,46 @@ TEST_F(PersonalDataManagerTest, OnUserAcceptedUpstreamOffer) {
   }
 }
 
+namespace {
+
+class OneTimeObserver : public PersonalDataManagerObserver {
+ public:
+  OneTimeObserver(PersonalDataManager* manager) : manager_(manager) {}
+
+  ~OneTimeObserver() override {
+    if (manager_)
+      manager_->RemoveObserver(this);
+  }
+
+  void OnPersonalDataChanged() override {
+    ASSERT_TRUE(manager_) << "Callback called after RemoveObserver()";
+    manager_->RemoveObserver(this);
+    manager_ = nullptr;
+  }
+
+  void OnPersonalDataFinishedProfileTasks() override {
+    EXPECT_TRUE(manager_) << "Callback called after RemoveObserver()";
+  }
+
+  bool IsConnected() { return manager_; }
+
+ private:
+  PersonalDataManager* manager_;
+};
+
+}  // namespace
+
+TEST_F(PersonalDataManagerTest, RemoveObserverInOnPersonalDataChanged) {
+  OneTimeObserver observer(personal_data_.get());
+
+  personal_data_->AddObserver(&observer);
+
+  // Do something to trigger a data change
+  personal_data_->AddProfile(test::GetFullProfile());
+
+  WaitForOnPersonalDataChanged();
+
+  EXPECT_FALSE(observer.IsConnected()) << "Observer not called";
+}
+
 }  // namespace autofill