[Autofill Assistant] Use base::ObserverList for navigation listeners.

Before this patch, the observer/listener for navigation state changes
were implemented using a std::vector. This would lead to null pointer
dereferencing when adding and removing listeners while handling an event
due to std::vector::erase reordering elements (pointers). This patch
uses a checked observer and ObserverList instead to properly handling
adding and removing observers/listeners.

This bug was uncovered while working on the bug linked in this patch.

R=arbesser@google.com

Bug: b/152724265
Change-Id: Ia59bfca5bec5ce326394668eb7d8d62004fc16fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2139691
Commit-Queue: Mathias Carlen <mcarlen@chromium.org>
Reviewed-by: Clemens Arbesser <arbesser@google.com>
Cr-Commit-Position: refs/heads/master@{#757021}

components/autofill_assistant/browser/controller.cc

@@ -315,16 +315,12 @@ void Controller::ClearGenericUi() {
   }
 }
 
-void Controller::AddListener(ScriptExecutorDelegate::Listener* listener) {
-  auto found = std::find(listeners_.begin(), listeners_.end(), listener);
-  if (found == listeners_.end())
-    listeners_.emplace_back(listener);
+void Controller::AddListener(NavigationListener* listener) {
+  navigation_listeners_.AddObserver(listener);
 }
 
-void Controller::RemoveListener(ScriptExecutorDelegate::Listener* listener) {
-  auto found = std::find(listeners_.begin(), listeners_.end(), listener);
-  if (found != listeners_.end())
-    listeners_.erase(found);
+void Controller::RemoveListener(NavigationListener* listener) {
+  navigation_listeners_.RemoveObserver(listener);
 }
 
 void Controller::SetExpandSheetForPromptAction(bool expand) {
@@ -575,9 +571,8 @@ const ClientSettings& Controller::GetClientSettings() const {
 }
 
 void Controller::ReportNavigationStateChanged() {
-  // Listeners are called in the same order they were added.
-  for (auto* listener : listeners_) {
-    listener->OnNavigationStateChanged();
+  for (auto& listener : navigation_listeners_) {
+    listener.OnNavigationStateChanged();
   }
 }
 

components/autofill_assistant/browser/controller.h

@@ -142,8 +142,8 @@ class Controller : public ScriptExecutorDelegate,
   // states where showing the UI is optional, such as RUNNING, in tracking mode.
   void RequireUI() override;
 
-  void AddListener(ScriptExecutorDelegate::Listener* listener) override;
-  void RemoveListener(ScriptExecutorDelegate::Listener* listener) override;
+  void AddListener(NavigationListener* listener) override;
+  void RemoveListener(NavigationListener* listener) override;
 
   void SetExpandSheetForPromptAction(bool expand) override;
 
@@ -406,7 +406,7 @@ class Controller : public ScriptExecutorDelegate,
 
   // Value for ScriptExecutorDelegate::HasNavigationError()
   bool navigation_error_ = false;
-  std::vector<ScriptExecutorDelegate::Listener*> listeners_;
+  base::ObserverList<NavigationListener> navigation_listeners_;
 
   // Tracks scripts and script execution. It's kept at the end, as it tend to
   // depend on everything the controller support, through script and script

components/autofill_assistant/browser/controller_unittest.cc

@@ -246,11 +246,12 @@ std::ostream& operator<<(std::ostream& out, const NavigationState& state) {
 
 // A Listener that keeps track of the reported state of the delegate captured
 // from OnNavigationStateChanged.
-class NavigationStateChangeListener : public ScriptExecutorDelegate::Listener {
+class NavigationStateChangeListener
+    : public ScriptExecutorDelegate::NavigationListener {
  public:
   explicit NavigationStateChangeListener(ScriptExecutorDelegate* delegate)
       : delegate_(delegate) {}
-  ~NavigationStateChangeListener() = default;
+  ~NavigationStateChangeListener() override;
   void OnNavigationStateChanged() override;
 
   std::vector<NavigationState> events;
@@ -259,6 +260,8 @@ class NavigationStateChangeListener : public ScriptExecutorDelegate::Listener {
   ScriptExecutorDelegate* const delegate_;
 };
 
+NavigationStateChangeListener::~NavigationStateChangeListener() {}
+
 void NavigationStateChangeListener::OnNavigationStateChanged() {
   NavigationState state;
   state.navigating = delegate_->IsNavigatingToNewDocument();

components/autofill_assistant/browser/fake_script_executor_delegate.cc

@@ -161,11 +161,11 @@ void FakeScriptExecutorDelegate::RequireUI() {
   require_ui_ = true;
 }
 
-void FakeScriptExecutorDelegate::AddListener(Listener* listener) {
+void FakeScriptExecutorDelegate::AddListener(NavigationListener* listener) {
   listeners_.insert(listener);
 }
 
-void FakeScriptExecutorDelegate::RemoveListener(Listener* listener) {
+void FakeScriptExecutorDelegate::RemoveListener(NavigationListener* listener) {
   listeners_.erase(listener);
 }
 

components/autofill_assistant/browser/fake_script_executor_delegate.h

@@ -66,8 +66,8 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
   bool HasNavigationError() override;
   bool IsNavigatingToNewDocument() override;
   void RequireUI() override;
-  void AddListener(Listener* listener) override;
-  void RemoveListener(Listener* listener) override;
+  void AddListener(NavigationListener* listener) override;
+  void RemoveListener(NavigationListener* listener) override;
   void SetExpandSheetForPromptAction(bool expand) override;
   void SetGenericUi(
       std::unique_ptr<GenericUserInterfaceProto> generic_ui,
@@ -136,7 +136,7 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
   std::unique_ptr<UserData> payment_request_info_;
   bool navigating_to_new_document_ = false;
   bool navigation_error_ = false;
-  std::set<ScriptExecutorDelegate::Listener*> listeners_;
+  std::set<ScriptExecutorDelegate::NavigationListener*> listeners_;
   ViewportMode viewport_mode_ = ViewportMode::NO_RESIZE;
   ConfigureBottomSheetProto::PeekMode peek_mode_ =
       ConfigureBottomSheetProto::HANDLE;

components/autofill_assistant/browser/script_executor.h

@@ -32,7 +32,7 @@ class UserModel;
 
 // Class to execute an assistant script.
 class ScriptExecutor : public ActionDelegate,
-                       public ScriptExecutorDelegate::Listener {
+                       public ScriptExecutorDelegate::NavigationListener {
  public:
   // Listens to events on ScriptExecutor.
   // TODO(b/806868): Make global_payload a part of callback instead of the
@@ -237,7 +237,7 @@ class ScriptExecutor : public ActionDelegate,
   // Helper for WaitForElementVisible that keeps track of the state required to
   // run interrupts while waiting for a specific element.
   class WaitForDomOperation : public ScriptExecutor::Listener,
-                              ScriptExecutorDelegate::Listener {
+                              ScriptExecutorDelegate::NavigationListener {
    public:
     // Let the caller know about either the result of looking for the element or
     // of an abnormal result from an interrupt.
@@ -267,7 +267,7 @@ class ScriptExecutor : public ActionDelegate,
     void Pause();
     void Continue();
 
-    // Implements ScriptExecutorDelegate::Listener
+    // Implements ScriptExecutorDelegate::NavigationListener
     void OnNavigationStateChanged() override;
 
     // Implements ScriptExecutor::Listener

components/autofill_assistant/browser/script_executor_delegate.h

@@ -10,6 +10,7 @@
 #include <string>
 #include <vector>
 
+#include "base/observer_list_types.h"
 #include "components/autofill_assistant/browser/client_status.h"
 #include "components/autofill_assistant/browser/details.h"
 #include "components/autofill_assistant/browser/info_box.h"
@@ -39,7 +40,7 @@ class UserModel;
 
 class ScriptExecutorDelegate {
  public:
-  class Listener {
+  class NavigationListener : public base::CheckedObserver {
    public:
     // The values returned by IsNavigatingToNewDocument() or
     // HasNavigationError() might have changed.
@@ -128,11 +129,11 @@ class ScriptExecutorDelegate {
 
   // Register a listener that can be told about changes. Duplicate calls are
   // ignored.
-  virtual void AddListener(Listener* listener) = 0;
+  virtual void AddListener(NavigationListener* listener) = 0;
 
   // Removes a previously registered listener. Does nothing if no such listeners
   // exists.
-  virtual void RemoveListener(Listener* listener) = 0;
+  virtual void RemoveListener(NavigationListener* listener) = 0;
 
   // Set how the sheet should behave when entering a prompt state.
   virtual void SetExpandSheetForPromptAction(bool expand) = 0;