[Autofill Assistant] Check element existences and values in sequence.

Before this patch, the script executor checked element existence in
parallel and the script tracker checked script preconditions in
parallel.

This doesn't work: element existence checks only work when run one at a
time. This meant that precondition on elements were never met, because
there were always multiple scripts to check, while WaitForDomProto
worked, as it's typically running a check on a single element.

With this patch, checking for elements from preconditions works, even
when there are more than one script or elements to check.

Bug: 806868
Change-Id: Ib5e97a36a7aff5df5eab183da9461375df4f1ec3
Reviewed-on: https://chromium-review.googlesource.com/c/1256794Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596197}

components/autofill_assistant/browser/script_precondition.cc

@@ -87,35 +87,8 @@ void ScriptPrecondition::Check(
     return;
   }
 
-  pending_preconditions_check_count_ =
-      elements_exist_.size() + form_value_match_.size();
-  if (!pending_preconditions_check_count_) {
-    std::move(callback).Run(true);
-    return;
-  }
-
-  // TODO(crbug.com/806868): Instead of running all checks in parallel and
-  // waiting for all of them to be finished, it might be better to check them
-  // one by one and fail the precondition early when one fails.
   check_preconditions_callback_ = std::move(callback);
-  all_preconditions_check_satisfied_ = true;
-
-  for (const auto& element : elements_exist_) {
-    web_controller->ElementExists(
-        element, base::BindOnce(&ScriptPrecondition::OnCheckElementExists,
-                                weak_ptr_factory_.GetWeakPtr()));
-  }
-
-  for (const auto& value_match : form_value_match_) {
-    DCHECK(!value_match.element().selectors().empty());
-    std::vector<std::string> selectors;
-    for (const auto& selector : value_match.element().selectors()) {
-      selectors.emplace_back(selector);
-    }
-    web_controller->GetFieldValue(
-        selectors, base::BindOnce(&ScriptPrecondition::OnGetFieldValue,
-                                  weak_ptr_factory_.GetWeakPtr()));
-  }
+  RunChecksSequentially(web_controller, 0);
 }
 
 ScriptPrecondition::ScriptPrecondition(
@@ -126,8 +99,6 @@ ScriptPrecondition::ScriptPrecondition(
     const std::vector<FormValueMatchProto>& form_value_match,
     const std::vector<ScriptStatusMatchProto>& status_match)
     : elements_exist_(elements_exist),
-      pending_preconditions_check_count_(0),
-      all_preconditions_check_satisfied_(false),
       domain_match_(domain_match),
       path_pattern_(std::move(path_pattern)),
       parameter_match_(parameter_match),
@@ -135,16 +106,6 @@ ScriptPrecondition::ScriptPrecondition(
       status_match_(status_match),
       weak_ptr_factory_(this) {}
 
-void ScriptPrecondition::OnCheckElementExists(bool result) {
-  DCHECK_LT(0u, pending_preconditions_check_count_);
-  pending_preconditions_check_count_--;
-  if (!result) {
-    all_preconditions_check_satisfied_ = false;
-  }
-
-  MaybeRunCheckPreconditionCallback();
-}
-
 bool ScriptPrecondition::MatchDomain(const GURL& url) const {
   if (domain_match_.empty())
     return true;
@@ -210,21 +171,64 @@ bool ScriptPrecondition::MatchScriptStatus(
   return true;
 }
 
-void ScriptPrecondition::OnGetFieldValue(const std::string& value) {
-  DCHECK_LT(0u, pending_preconditions_check_count_);
-  pending_preconditions_check_count_--;
-  if (value.empty()) {
-    all_preconditions_check_satisfied_ = false;
+void ScriptPrecondition::RunChecksSequentially(
+    WebController* web_controller,
+    size_t check_precondition_count) {
+  if (check_precondition_count < elements_exist_.size()) {
+    web_controller->ElementExists(
+        elements_exist_[check_precondition_count],
+        base::BindOnce(
+            &ScriptPrecondition::OnCheckElementExists,
+            weak_ptr_factory_.GetWeakPtr(),
+            // web_controller must remain valid until the callback is run
+            base::Unretained(web_controller), check_precondition_count));
+    return;
+  }
+
+  // After checking elements existence, check form value match. This starts when
+  // check_precondition_count reaches element_exists_.size().
+  size_t form_value_match_index =
+      check_precondition_count - elements_exist_.size();
+  if (form_value_match_index < form_value_match_.size()) {
+    const auto& value_match = form_value_match_[form_value_match_index];
+    DCHECK(!value_match.element().selectors().empty());
+    std::vector<std::string> selectors;
+    for (const auto& selector : value_match.element().selectors()) {
+      selectors.emplace_back(selector);
+    }
+    web_controller->GetFieldValue(
+        selectors,
+        base::BindOnce(
+            &ScriptPrecondition::OnGetFieldValue,
+            weak_ptr_factory_.GetWeakPtr(),
+            // web_controller must remain valid until the callback is run
+            base::Unretained(web_controller), check_precondition_count));
+    return;
   }
+  DCHECK_EQ(check_precondition_count,
+            elements_exist_.size() + form_value_match_.size());
 
-  MaybeRunCheckPreconditionCallback();
+  std::move(check_preconditions_callback_).Run(true);
 }
 
-void ScriptPrecondition::MaybeRunCheckPreconditionCallback() {
-  if (!pending_preconditions_check_count_ && check_preconditions_callback_) {
-    std::move(check_preconditions_callback_)
-        .Run(all_preconditions_check_satisfied_);
+void ScriptPrecondition::OnCheckElementExists(WebController* web_controller,
+                                              size_t check_precondition_count,
+                                              bool exists) {
+  if (!exists) {
+    std::move(check_preconditions_callback_).Run(false);
+    return;
+  }
+  RunChecksSequentially(web_controller, check_precondition_count + 1);
+}
+
+void ScriptPrecondition::OnGetFieldValue(WebController* web_controller,
+                                         size_t check_precondition_count,
+                                         const std::string& value) {
+  if (value.empty()) {
+    std::move(check_preconditions_callback_).Run(false);
+    return;
   }
+  RunChecksSequentially(web_controller, check_precondition_count + 1);
 }
 
 }  // namespace autofill_assistant

components/autofill_assistant/browser/script_precondition.h

@@ -45,14 +45,13 @@ class ScriptPrecondition {
   ~ScriptPrecondition();
 
   // Check whether the conditions satisfied and return the result through
-  // |callback|.
+  // |callback|. |web_controller| must remain valid until the callback is run.
   void Check(WebController* web_controller,
              const std::map<std::string, std::string>& parameters,
              const std::map<std::string, ScriptStatusProto>& executed_scripts,
              base::OnceCallback<void(bool)> callback);
 
  private:
-  void OnCheckElementExists(bool result);
   bool MatchDomain(const GURL& url) const;
   bool MatchPath(const GURL& url) const;
   bool MatchParameters(
@@ -60,16 +59,17 @@ class ScriptPrecondition {
   bool MatchScriptStatus(
       const std::map<std::string, ScriptStatusProto>& executed_scripts) const;
 
-  void OnGetFieldValue(const std::string& value);
-
-  // Return if all checks have been completed and we have not returned anything
-  // yet.
-  void MaybeRunCheckPreconditionCallback();
+  void RunChecksSequentially(WebController* web_controller,
+                             size_t check_precondition_count);
+  void OnCheckElementExists(WebController* web_controller,
+                            size_t check_precondition_count,
+                            bool result);
+  void OnGetFieldValue(WebController* web_controller,
+                       size_t check_precondition_count,
+                       const std::string& value);
 
   std::vector<std::vector<std::string>> elements_exist_;
   base::OnceCallback<void(bool)> check_preconditions_callback_;
-  size_t pending_preconditions_check_count_;
-  bool all_preconditions_check_satisfied_;
 
   // Domain (exact match) excluding the last '/' character.
   std::set<std::string> domain_match_;

components/autofill_assistant/browser/script_tracker.cc

@@ -17,7 +17,7 @@ ScriptTracker::ScriptTracker(ScriptExecutorDelegate* delegate,
                              ScriptTracker::Listener* listener)
     : delegate_(delegate),
       listener_(listener),
-      pending_precondition_check_count_(0),
+      running_checks_(false),
       weak_ptr_factory_(this) {
   DCHECK(delegate_);
   DCHECK(listener_);
@@ -35,27 +35,11 @@ void ScriptTracker::SetAndCheckScripts(
 }
 
 void ScriptTracker::CheckScripts() {
-  if (pending_precondition_check_count_ > 0)
+  if (running_checks_)
     return;
-  DCHECK_EQ(pending_precondition_check_count_, 0);
-  DCHECK(pending_runnable_scripts_.empty());
-
-  // pending_precondition_check_count_ lets OnPreconditionCheck know when to
-  // stop. It must be set before the callback can possibly be run.
-  pending_precondition_check_count_ = available_scripts_.size();
-  if (pending_precondition_check_count_ == 0) {
-    // Possibly report an empty set of runnable scripts.
-    UpdateRunnableScriptsIfNecessary();
-  } else {
-    for (const auto& entry : available_scripts_) {
-      Script* script = entry.first;
-      script->precondition->Check(
-          delegate_->GetWebController(), delegate_->GetParameters(),
-          executed_scripts_,
-          base::BindOnce(&ScriptTracker::OnPreconditionCheck,
-                         weak_ptr_factory_.GetWeakPtr(), script));
-    }
-  }
+  running_checks_ = true;
+
+  RunPreconditionChecksSequentially(available_scripts_.begin());
 }
 
 void ScriptTracker::ExecuteScript(const std::string& script_path,
@@ -83,7 +67,6 @@ void ScriptTracker::OnScriptRun(
 }
 
 void ScriptTracker::UpdateRunnableScriptsIfNecessary() {
-  DCHECK_EQ(pending_precondition_check_count_, 0);
   bool runnables_changed = RunnablesHaveChanged();
   if (runnables_changed) {
     runnable_scripts_.clear();
@@ -103,6 +86,7 @@ void ScriptTracker::UpdateRunnableScriptsIfNecessary() {
   }
   pending_runnable_scripts_.clear();
 
+  running_checks_ = false;
   if (runnables_changed) {
     listener_->OnRunnableScriptsChanged(runnable_scripts_);
   }
@@ -123,26 +107,39 @@ bool ScriptTracker::RunnablesHaveChanged() {
   return pending_paths != current_paths;
 }
 
+void ScriptTracker::RunPreconditionChecksSequentially(
+    AvailableScriptMap::const_iterator step) {
+  if (step == available_scripts_.end()) {
+    UpdateRunnableScriptsIfNecessary();
+    return;
+  }
+
+  Script* script = step->first;
+  script->precondition->Check(
+      delegate_->GetWebController(), delegate_->GetParameters(),
+      executed_scripts_,
+      base::BindOnce(&ScriptTracker::OnPreconditionCheck,
+                     weak_ptr_factory_.GetWeakPtr(), script, step));
+}
+
 void ScriptTracker::OnPreconditionCheck(Script* script,
+                                        AvailableScriptMap::const_iterator step,
                                         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_precondition_check_count_--;
-  if (pending_precondition_check_count_ > 0)
-    return;
-  UpdateRunnableScriptsIfNecessary();
+
+  RunPreconditionChecksSequentially(++step);
 }
 
 void ScriptTracker::ClearAvailableScripts() {
   available_scripts_.clear();
   // Clearing available_scripts_ has cancelled any pending precondition checks,
   // ending them.
-  pending_precondition_check_count_ = 0;
+  running_checks_ = false;
   pending_runnable_scripts_.clear();
 }
 

components/autofill_assistant/browser/script_tracker.h

@@ -66,6 +66,8 @@ class ScriptTracker {
   bool running() const { return executor_ != nullptr; }
 
  private:
+  typedef std::map<Script*, std::unique_ptr<Script>> AvailableScriptMap;
+
   void OnScriptRun(const std::string& script_path,
                    ScriptExecutor::RunScriptCallback original_callback,
                    ScriptExecutor::Result result);
@@ -73,7 +75,11 @@ class ScriptTracker {
 
   // Returns true if |runnable_| should be updated.
   bool RunnablesHaveChanged();
-  void OnPreconditionCheck(Script* script, bool met_preconditions);
+  void RunPreconditionChecksSequentially(
+      AvailableScriptMap::const_iterator step);
+  void OnPreconditionCheck(Script* script,
+                           AvailableScriptMap::const_iterator step,
+                           bool met_preconditions);
   void ClearAvailableScripts();
 
   ScriptExecutorDelegate* const delegate_;
@@ -89,14 +95,13 @@ class ScriptTracker {
 
   // Sets of available scripts. SetScripts resets this and interrupts
   // any pending check.
-  std::map<Script*, std::unique_ptr<Script>> available_scripts_;
+  AvailableScriptMap available_scripts_;
 
   // List of scripts that have been executed and their corresponding statuses.
   std::map<std::string, ScriptStatusProto> executed_scripts_;
 
-  // Number of precondition checks run for CheckScripts that are still
-  // pending.
-  int pending_precondition_check_count_;
+  // If true, a check is currently in progress.
+  bool running_checks_;
 
   // Scripts found to be runnable so far, in the current run of CheckScripts.
   std::vector<Script*> pending_runnable_scripts_;