Allow any GetActions response to change the set of scripts.

Before this change, the set of updated script from
GetActionsResponse.update_script_list was only applied at the end of a
script.

With this change, the set of updated scripts reported by
GetActionsResponse.updated_script_list is applied right away.

This limitation was put in place to avoid issues with pointers to
scripts becoming invalid at any time. To avoid this issues, this change
avoids storing Script* anywhere else than the list of scripts,
preferring to reference scripts by their path, when necessary.

This change also strictly separates scripts from interrupts, to avoid
confusion and to keep things a bit simpler. It is not possible anymore
to define a script that is both an interrupt and a normal script, that
is proposed as a runnable script on the UI. This feature was never used.

Bug: b/146314751
Change-Id: I466adb9c9ec3e79c659bac0798df46b5311739a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1968992Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726353}

components/autofill_assistant/browser/script_executor.cc

@@ -68,7 +68,7 @@ ScriptExecutor::ScriptExecutor(
     const std::string& script_payload,
     ScriptExecutor::Listener* listener,
     std::map<std::string, ScriptStatusProto>* scripts_state,
-    const std::vector<Script*>* ordered_interrupts,
+    const std::vector<std::unique_ptr<Script>>* ordered_interrupts,
     ScriptExecutorDelegate* delegate)
     : script_path_(script_path),
       additional_context_(std::move(additional_context)),
@@ -842,7 +842,8 @@ void ScriptExecutor::WaitForDomOperation::RunChecks(
                       base::BindOnce(&WaitForDomOperation::OnElementCheckDone,
                                      base::Unretained(this)));
   if (allow_interrupt_) {
-    for (const auto* interrupt : *main_script_->ordered_interrupts_) {
+    for (const std::unique_ptr<Script>& interrupt :
+         *main_script_->ordered_interrupts_) {
       if (ran_interrupts_.find(interrupt->handle.path) !=
           ran_interrupts_.end()) {
         continue;
@@ -853,7 +854,7 @@ void ScriptExecutor::WaitForDomOperation::RunChecks(
           *delegate_->GetTriggerContext(), *main_script_->scripts_state_,
           base::BindOnce(&WaitForDomOperation::OnPreconditionCheckDone,
                          weak_ptr_factory_.GetWeakPtr(),
-                         base::Unretained(interrupt)));
+                         interrupt->handle.path));
     }
   }
 
@@ -864,10 +865,10 @@ void ScriptExecutor::WaitForDomOperation::RunChecks(
 }
 
 void ScriptExecutor::WaitForDomOperation::OnPreconditionCheckDone(
-    const Script* interrupt,
+    const std::string& interrupt_path,
     bool precondition_match) {
   if (precondition_match)
-    runnable_interrupts_.insert(interrupt);
+    runnable_interrupts_.insert(interrupt_path);
 }
 
 void ScriptExecutor::WaitForDomOperation::OnElementCheckDone(
@@ -891,9 +892,11 @@ void ScriptExecutor::WaitForDomOperation::OnAllChecksDone(
   } else {
     // We must go through runnable_interrupts_ to make sure priority order is
     // respected in case more than one interrupt is ready to run.
-    for (const auto* interrupt : *main_script_->ordered_interrupts_) {
-      if (runnable_interrupts_.find(interrupt) != runnable_interrupts_.end()) {
-        RunInterrupt(interrupt);
+    for (const std::unique_ptr<Script>& interrupt :
+         *main_script_->ordered_interrupts_) {
+      const std::string& path = interrupt->handle.path;
+      if (runnable_interrupts_.find(path) != runnable_interrupts_.end()) {
+        RunInterrupt(path);
         return;
       }
     }
@@ -902,13 +905,12 @@ void ScriptExecutor::WaitForDomOperation::OnAllChecksDone(
 }
 
 void ScriptExecutor::WaitForDomOperation::RunInterrupt(
-    const Script* interrupt) {
+    const std::string& path) {
   batch_element_checker_.reset();
   SavePreInterruptState();
-  ran_interrupts_.insert(interrupt->handle.path);
+  ran_interrupts_.insert(path);
   interrupt_executor_ = std::make_unique<ScriptExecutor>(
-      interrupt->handle.path,
-      TriggerContext::Merge({main_script_->additional_context_.get()}),
+      path, TriggerContext::Merge({main_script_->additional_context_.get()}),
       main_script_->last_global_payload_, main_script_->initial_script_payload_,
       /* listener= */ this, main_script_->scripts_state_, &no_interrupts_,
       delegate_);

components/autofill_assistant/browser/script_executor.h

@@ -61,7 +61,7 @@ class ScriptExecutor : public ActionDelegate,
                  const std::string& script_payload,
                  ScriptExecutor::Listener* listener,
                  std::map<std::string, ScriptStatusProto>* scripts_state,
-                 const std::vector<Script*>* ordered_interrupts,
+                 const std::vector<std::unique_ptr<Script>>* ordered_interrupts,
                  ScriptExecutorDelegate* delegate);
   ~ScriptExecutor() override;
 
@@ -260,12 +260,12 @@ class ScriptExecutor : public ActionDelegate,
 
     void RunChecks(
         base::OnceCallback<void(const ClientStatus&)> report_attempt_result);
-    void OnPreconditionCheckDone(const Script* interrupt,
+    void OnPreconditionCheckDone(const std::string& interrupt_path,
                                  bool precondition_match);
     void OnElementCheckDone(const ClientStatus&);
     void OnAllChecksDone(
         base::OnceCallback<void(const ClientStatus&)> report_attempt_result);
-    void RunInterrupt(const Script* interrupt);
+    void RunInterrupt(const std::string& path);
     void OnInterruptDone(const ScriptExecutor::Result& result);
     void RunCallback(const ClientStatus& element_status);
     void RunCallbackWithResult(const ClientStatus& element_status,
@@ -291,12 +291,15 @@ class ScriptExecutor : public ActionDelegate,
     WaitForDomOperation::Callback callback_;
 
     std::unique_ptr<BatchElementChecker> batch_element_checker_;
-    std::set<const Script*> runnable_interrupts_;
+
+    // Path of interrupts from |ordered_interrupts_| that have been found
+    // runnable.
+    std::set<std::string> runnable_interrupts_;
     ClientStatus element_check_result_;
 
     // An empty vector of interrupts that can be passed to interrupt_executor_
     // and outlives it. Interrupts must not run interrupts.
-    const std::vector<Script*> no_interrupts_;
+    const std::vector<std::unique_ptr<Script>> no_interrupts_;
 
     // The interrupt that's currently running.
     std::unique_ptr<ScriptExecutor> interrupt_executor_;
@@ -365,10 +368,12 @@ class ScriptExecutor : public ActionDelegate,
   std::string last_script_payload_;
   ScriptExecutor::Listener* const listener_;
   ScriptExecutorDelegate* const delegate_;
-  // Set of interrupts that might run during wait for dom actions with
+  // Set of interrupts that might run during wait for dom or prompt action with
   // allow_interrupt. Sorted by priority; an interrupt that appears on the
-  // vector first should run first.
-  const std::vector<Script*>* ordered_interrupts_;
+  // vector first should run first. Note that the content of this vector can
+  // change while the script is running, as a result of OnScriptListChanged
+  // being called.
+  const std::vector<std::unique_ptr<Script>>* const ordered_interrupts_;
   std::map<std::string, ScriptStatusProto>* const scripts_state_;
   RunScriptCallback callback_;
   std::vector<std::unique_ptr<Action>> actions_;

components/autofill_assistant/browser/script_executor_unittest.cc

@@ -140,8 +140,7 @@ class ScriptExecutorTest : public testing::Test,
     interrupt->precondition =
         ScriptPrecondition::FromProto(path, interrupt_preconditions);
 
-    ordered_interrupts_.push_back(interrupt.get());
-    interrupts_.emplace_back(std::move(interrupt));
+    ordered_interrupts_.emplace_back(std::move(interrupt));
   }
 
   // task_environment_ must be first to guarantee other field
@@ -153,9 +152,7 @@ class ScriptExecutorTest : public testing::Test,
   NiceMock<MockWebController> mock_web_controller_;
   std::map<std::string, ScriptStatusProto> scripts_state_;
 
-  // An owner for the pointers in |ordered_interrupts_|
-  std::vector<std::unique_ptr<Script>> interrupts_;
-  std::vector<Script*> ordered_interrupts_;
+  std::vector<std::unique_ptr<Script>> ordered_interrupts_;
   std::string last_global_payload_;
   std::string last_script_payload_;
   bool should_update_scripts_ = false;

components/autofill_assistant/browser/script_tracker.cc

@@ -17,19 +17,19 @@ namespace autofill_assistant {
 
 namespace {
 
-// Sort scripts by priority.
-void SortScripts(std::vector<Script*>* scripts) {
-  std::sort(scripts->begin(), scripts->end(),
-            [](const Script* a, const Script* b) {
-              // Runnable scripts with lowest priority value are displayed
-              // first, Interrupts with lowest priority values are run first.
-              // Order of scripts with the same priority is arbitrary. Fallback
-              // to ordering by name and path, arbitrarily, for the behavior to
-              // be consistent across runs.
-              return std::tie(a->priority, a->handle.chip.text,
-                              a->handle.path) <
-                     std::tie(b->priority, b->handle.chip.text, a->handle.path);
-            });
+// Sort scripts by priority. T must be a normal or smart pointer to a script
+void SortScripts(std::vector<std::unique_ptr<Script>>* scripts) {
+  std::sort(
+      scripts->begin(), scripts->end(),
+      [](const std::unique_ptr<Script>& a, const std::unique_ptr<Script>& b) {
+        // Runnable scripts with lowest priority value are displayed
+        // first, Interrupts with lowest priority values are run first.
+        // Order of scripts with the same priority is arbitrary. Fallback
+        // to ordering by name and path, arbitrarily, for the behavior to
+        // be consistent across runs.
+        return std::tie(a->priority, a->handle.chip.text, a->handle.path) <
+               std::tie(b->priority, b->handle.chip.text, a->handle.path);
+      });
 }
 
 // Creates a value containing a vector of a simple type, accepted by base::Value
@@ -55,25 +55,18 @@ ScriptTracker::ScriptTracker(ScriptExecutorDelegate* delegate,
 ScriptTracker::~ScriptTracker() = default;
 
 void ScriptTracker::SetScripts(std::vector<std::unique_ptr<Script>> scripts) {
-  // The set of interrupts must not change while a script is running, as they're
-  // passed to ScriptExecutor.
-  if (running())
-    return;
-
   TerminatePendingChecks();
 
   available_scripts_.clear();
-  for (auto& script : scripts) {
-    available_scripts_[script.get()] = std::move(script);
-  }
-
   interrupts_.clear();
-  for (auto& entry : available_scripts_) {
-    Script* script = entry.first;
+  for (auto& script : scripts) {
     if (script->handle.interrupt) {
-      interrupts_.push_back(script);
+      interrupts_.emplace_back(std::move(script));
+    } else {
+      available_scripts_.emplace_back(std::move(script));
     }
   }
+  SortScripts(&available_scripts_);
   SortScripts(&interrupts_);
 }
 
@@ -85,8 +78,7 @@ void ScriptTracker::CheckScripts() {
 
   GURL url = delegate_->GetCurrentURL();
   batch_element_checker_ = std::make_unique<BatchElementChecker>();
-  for (const auto& entry : available_scripts_) {
-    Script* script = entry.first;
+  for (const std::unique_ptr<Script>& script : available_scripts_) {
     if (script->handle.chip.empty() && script->handle.direct_action.empty() &&
         !script->handle.autostart)
       continue;
@@ -95,7 +87,7 @@ void ScriptTracker::CheckScripts() {
         url, batch_element_checker_.get(), *delegate_->GetTriggerContext(),
         scripts_state_,
         base::BindOnce(&ScriptTracker::OnPreconditionCheck,
-                       weak_ptr_factory_.GetWeakPtr(), script));
+                       weak_ptr_factory_.GetWeakPtr(), script->handle.path));
   }
   if (batch_element_checker_->empty() && pending_runnable_scripts_.empty() &&
       !available_scripts_.empty()) {
@@ -164,8 +156,8 @@ base::Value ScriptTracker::GetDebugContext() const {
   dict.SetKey("executed-scripts", base::Value(scripts_state_js));
 
   std::vector<base::Value> available_scripts_js;
-  for (const auto& entry : available_scripts_)
-    available_scripts_js.push_back(base::Value(entry.second->handle.path));
+  for (const std::unique_ptr<Script>& script : available_scripts_)
+    available_scripts_js.push_back(base::Value(script->handle.path));
   dict.SetKey("available-scripts", base::Value(available_scripts_js));
 
   std::vector<base::Value> runnable_scripts_js;
@@ -199,17 +191,9 @@ void ScriptTracker::OnScriptRun(
     ScriptExecutor::RunScriptCallback original_callback,
     const ScriptExecutor::Result& result) {
   executor_.reset();
-  MaybeSwapInScripts();
   std::move(original_callback).Run(result);
 }
 
-void ScriptTracker::MaybeSwapInScripts() {
-  if (scripts_update_) {
-    SetScripts(std::move(*scripts_update_));
-    scripts_update_.reset();
-  }
-}
-
 void ScriptTracker::OnCheckDone() {
   UpdateRunnableScriptsIfNecessary();
   TerminatePendingChecks();
@@ -222,12 +206,16 @@ void ScriptTracker::UpdateRunnableScriptsIfNecessary() {
     return;
   }
 
+  // Go through available scripts, to guarantee that runnable_scripts_ follows
+  // the order of available_scripts_. As a side effect, any invalid path in
+  // runnable_scripts_ is ignored here.
   runnable_scripts_.clear();
-  SortScripts(&pending_runnable_scripts_);
-  for (Script* script : pending_runnable_scripts_) {
-    runnable_scripts_.push_back(script->handle);
+  for (const std::unique_ptr<Script>& script : available_scripts_) {
+    if (pending_runnable_scripts_.find(script->handle.path) !=
+        pending_runnable_scripts_.end()) {
+      runnable_scripts_.push_back(script->handle);
+    }
   }
-
   listener_->OnRunnableScriptsChanged(runnable_scripts_);
 }
 
@@ -240,25 +228,20 @@ bool ScriptTracker::RunnablesHaveChanged() {
   if (runnable_scripts_.size() != pending_runnable_scripts_.size())
     return true;
 
-  std::set<std::string> pending_paths;
-  for (Script* script : pending_runnable_scripts_) {
-    pending_paths.insert(script->handle.path);
-  }
   std::set<std::string> current_paths;
   for (const auto& handle : runnable_scripts_) {
     current_paths.insert(handle.path);
   }
-  return pending_paths != current_paths;
+  return pending_runnable_scripts_ != current_paths;
 }
 
-void ScriptTracker::OnPreconditionCheck(Script* script,
+void ScriptTracker::OnPreconditionCheck(const std::string& script_path,
                                         bool met_preconditions) {
-  if (available_scripts_.find(script) == available_scripts_.end()) {
-    // Result is not relevant anymore.
-    return;
-  }
   if (met_preconditions)
-    pending_runnable_scripts_.push_back(script);
+    pending_runnable_scripts_.insert(script_path);
+
+  // Note that if we receive a path for a script not in available_scripts_ - or
+  // not any more - it'll be ignored in UpdateRunnableScriptsIfNecessary.
 }
 
 void ScriptTracker::OnServerPayloadChanged(const std::string& global_payload,
@@ -269,8 +252,7 @@ void ScriptTracker::OnServerPayloadChanged(const std::string& global_payload,
 
 void ScriptTracker::OnScriptListChanged(
     std::vector<std::unique_ptr<Script>> scripts) {
-  scripts_update_.reset(
-      new std::vector<std::unique_ptr<Script>>(std::move(scripts)));
+  SetScripts(std::move(scripts));
 }
 
 }  // namespace autofill_assistant

components/autofill_assistant/browser/script_tracker.h

@@ -103,17 +103,12 @@ class ScriptTracker : public ScriptExecutor::Listener {
   base::Value GetDebugContext() const;
 
  private:
-  typedef std::map<Script*, std::unique_ptr<Script>> AvailableScriptMap;
-
   friend class ScriptTrackerTest;
 
   void OnScriptRun(const std::string& script_path,
                    ScriptExecutor::RunScriptCallback original_callback,
                    const ScriptExecutor::Result& result);
 
-  // Updates the list of available scripts if there is a pending update from
-  // when a script was still being executed.
-  void MaybeSwapInScripts();
   void OnCheckDone();
   void UpdateRunnableScriptsIfNecessary();
 
@@ -124,7 +119,8 @@ class ScriptTracker : public ScriptExecutor::Listener {
 
   // Returns true if |runnable_| should be updated.
   bool RunnablesHaveChanged();
-  void OnPreconditionCheck(Script* script, bool met_preconditions);
+  void OnPreconditionCheck(const std::string& script_path,
+                           bool met_preconditions);
 
   // Overrides ScriptExecutor::Listener.
   void OnServerPayloadChanged(const std::string& global_payload,
@@ -149,21 +145,22 @@ class ScriptTracker : public ScriptExecutor::Listener {
   // the bottom bar.
   std::vector<ScriptHandle> runnable_scripts_;
 
-  // Sets of available scripts. SetScripts resets this and interrupts
-  // any pending check.
-  AvailableScriptMap available_scripts_;
+  // Sets of available scripts, excluding interrupts, ordered by priority.
+  // SetScripts() resets this.
+  std::vector<std::unique_ptr<Script>> available_scripts_;
 
-  // A subset of available_scripts that are interrupts.
-  std::vector<Script*> interrupts_;
+  // A subset of available_scripts that are interrupts, ordered by priority.
+  // SetScripts() reset this.
+  std::vector<std::unique_ptr<Script>> interrupts_;
 
   // List of scripts that have been executed and their corresponding statuses.
   std::map<std::string, ScriptStatusProto> scripts_state_;
 
   std::unique_ptr<BatchElementChecker> batch_element_checker_;
 
-  // Scripts found to be runnable so far, in the current check, represented by
-  // |batch_element_checker_|.
-  std::vector<Script*> pending_runnable_scripts_;
+  // Path of the scripts found to be runnable so far, in the current check,
+  // represented by |batch_element_checker_|.
+  std::set<std::string> pending_runnable_scripts_;
 
   // If a script is currently running, this is the script's executor. Otherwise,
   // this is nullptr.
@@ -172,10 +169,6 @@ class ScriptTracker : public ScriptExecutor::Listener {
   std::string last_global_payload_;
   std::string last_script_payload_;
 
-  // List of scripts to replace the currently available scripts. The replacement
-  // only occurse when |scripts_update| is not nullptr.
-  std::unique_ptr<std::vector<std::unique_ptr<Script>>> scripts_update_;
-
   base::WeakPtrFactory<ScriptTracker> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(ScriptTracker);