[Autofill Assistant] Remove BatchElementChecker::StopTrying

BatchElementChecker:StopTrying was introduced as a work around when
finding elements failed when run in parallel. On ScriptTracker, this
required delaying re-checks and even executing scripts until a clean
script end, which is complex to get right.

Now that finding elements can safely be run in parallel, a
BatchElementChecker can safely be deleted to interrupt any pending
checks. This simplifies ScriptTracker.

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

components/autofill_assistant/browser/batch_element_checker.cc

@@ -23,7 +23,6 @@ BatchElementChecker::BatchElementChecker(WebController* web_controller)
     : web_controller_(web_controller),
       pending_checks_count_(0),
       all_found_(false),
-      stopped_(false),
       weak_ptr_factory_(this) {
   DCHECK(web_controller);
 }
@@ -65,10 +64,6 @@ void BatchElementChecker::Run(const base::TimeDelta& duration,
 void BatchElementChecker::Try(base::OnceCallback<void()> try_done_callback) {
   DCHECK(!try_done_callback_);
 
-  if (stopped_) {
-    std::move(try_done_callback).Run();
-    return;
-  }
   try_done_callback_ = std::move(try_done_callback);
 
   DCHECK_EQ(pending_checks_count_, 0);
@@ -130,7 +125,7 @@ void BatchElementChecker::OnTryDone(int64_t remaining_attempts,
   }
 
   --remaining_attempts;
-  if (remaining_attempts <= 0 || stopped_) {
+  if (remaining_attempts <= 0) {
     // GiveUp is run before calling try_done, so its effects are visible right
     // away.
     GiveUp();

components/autofill_assistant/browser/batch_element_checker.h

@@ -88,9 +88,6 @@ class BatchElementChecker {
            base::RepeatingCallback<void()> try_done,
            base::OnceCallback<void()> all_done);
 
-  // Makes any pending Run stop after the end of the current try.
-  void StopTrying() { stopped_ = true; }
-
   // Returns true if all element that were asked for have been found. Can be
   // called while Run is progress or afterwards.
   bool all_found() { return all_found_; }
@@ -136,7 +133,6 @@ class BatchElementChecker {
       get_field_value_callbacks_;
   int pending_checks_count_;
   bool all_found_;
-  bool stopped_;
 
   // The callback built for Try(). It is kept around while going through the
   // maps and called once all selectors in that map have been looked up once,

components/autofill_assistant/browser/batch_element_checker_unittest.cc

@@ -351,26 +351,5 @@ TEST_F(BatchElementCheckerTest, TryOnceGivenSmallDuration) {
   EXPECT_FALSE(checks_.all_found());
 }
 
-TEST_F(BatchElementCheckerTest, StopTrying) {
-  EXPECT_CALL(mock_web_controller_,
-              OnElementCheck(kExistenceCheck, ElementsAre("element"), _))
-      .WillRepeatedly(RunOnceCallback<2>(false));
-
-  checks_.AddElementCheck(kExistenceCheck, {"element"}, base::DoNothing());
-  checks_.Run(base::TimeDelta::FromSeconds(1), TryCallback("try"),
-              DoneCallback("all_done"));
-
-  // The first try does not fully succeed.
-  EXPECT_THAT(try_done_, Contains(Pair("try", 1)));
-  EXPECT_THAT(all_done_, Not(Contains("all_done")));
-
-  checks_.StopTrying();
-
-  // Give up on the second try, because of StopTrying.
-  AdvanceTime();
-  EXPECT_THAT(try_done_, Contains(Pair("try", 2)));
-  EXPECT_THAT(all_done_, Contains("all_done"));
-}
-
 }  // namespace
 }  // namespace autofill_assistant

components/autofill_assistant/browser/script_tracker.cc

@@ -32,25 +32,9 @@ void ScriptTracker::SetScripts(std::vector<std::unique_ptr<Script>> scripts) {
 }
 
 void ScriptTracker::CheckScripts(const base::TimeDelta& max_duration) {
-  if (batch_element_checker_) {
-    // It should be possible to just call batch_element_checker_.reset() to give
-    // up on all checks. This doesn't work, however, because it ends up running
-    // multiple checks in parallel, which fails.
-    //
-    // StopTrying() tells BatchElementChecker to give up early and call
-    // OnCheckDone as soon as possible, which is a safe point for deleting the
-    // checker and starting a new check.
-    //
-    // TODO(crbug.com/806868): Figure out why checks run in parallel don't work
-    // and simplify this logic.
-    batch_element_checker_->StopTrying();
-
-    // TODO(crbug.com/806868): May stop recheck if there is a script pending to
-    // run.
-    must_recheck_ = base::BindOnce(&ScriptTracker::CheckScripts,
-                                   base::Unretained(this), max_duration);
-    return;
-  }
+  // In case checks are still running, terminate them.
+  TerminatePendingChecks();
+
   DCHECK(pending_runnable_scripts_.empty());
 
   batch_element_checker_ =
@@ -91,18 +75,8 @@ void ScriptTracker::ExecuteScript(const std::string& script_path,
   ScriptExecutor::RunScriptCallback run_script_callback = base::BindOnce(
       &ScriptTracker::OnScriptRun, weak_ptr_factory_.GetWeakPtr(), script_path,
       std::move(callback));
-  // Postpone running script until finishing the current round of preconditions
-  // check.
-  if (!batch_element_checker_ && !must_recheck_) {
-    executor_->Run(std::move(run_script_callback));
-  } else {
-    pending_run_script_callback_ = std::move(run_script_callback);
-    // Do not recheck and retry when there is a script pending to run. Note
-    // that |batch_element_checker_| may take a long time to wait on retrying
-    // unsatisfied preconditions check without stop trying.
-    must_recheck_.Reset();
-    batch_element_checker_->StopTrying();
-  }
+  TerminatePendingChecks();
+  executor_->Run(std::move(run_script_callback));
 }
 
 void ScriptTracker::ClearRunnableScripts() {
@@ -114,7 +88,6 @@ void ScriptTracker::OnScriptRun(
     const std::string& script_path,
     ScriptExecutor::RunScriptCallback original_callback,
     ScriptExecutor::Result result) {
-  DCHECK(!pending_run_script_callback_);
   executor_.reset();
   executed_scripts_[script_path] =
       result.success ? SCRIPT_STATUS_SUCCESS : SCRIPT_STATUS_FAILURE;
@@ -144,14 +117,6 @@ void ScriptTracker::UpdateRunnableScriptsIfNecessary() {
 
 void ScriptTracker::OnCheckDone() {
   TerminatePendingChecks();
-  if (must_recheck_) {
-    std::move(must_recheck_).Run();
-    return;
-  }
-
-  // TODO(crbug.com/806868): Check whether the script is still runnable.
-  if (pending_run_script_callback_)
-    executor_->Run(std::move(pending_run_script_callback_));
 }
 
 void ScriptTracker::TerminatePendingChecks() {

components/autofill_assistant/browser/script_tracker.h

@@ -53,8 +53,8 @@ class ScriptTracker : public ScriptExecutor::Listener {
   // Run the preconditions on the current set of scripts, and possibly update
   // the set of runnable scripts.
   //
-  // Calling CheckScripts() while a check is in progress cleanly cancels the
-  // previously running check and starts a new one right afterwards.
+  // Calling CheckScripts() while a check is in progress cancels the previously
+  // running check and starts a new one right away.
   void CheckScripts(const base::TimeDelta& max_duration);
 
   // Runs a script and reports, when the script has ended, whether the run was
@@ -87,7 +87,9 @@ class ScriptTracker : public ScriptExecutor::Listener {
   // Overrides ScriptExecutor::Listener.
   void OnServerPayloadChanged(const std::string& server_payload) override;
 
-  // Cleans up any state use by pending checks. Stops running pending checks.
+  // Stops running pending checks and cleans up any state used by pending
+  // checks. This can safely be called at any time, including when no checks are
+  // running.
   void TerminatePendingChecks();
 
   // Returns true if |runnable_| should be updated.
@@ -120,18 +122,10 @@ class ScriptTracker : public ScriptExecutor::Listener {
   // |batch_element_checker_|.
   std::vector<Script*> pending_runnable_scripts_;
 
-  // If a Check() was called while a check was in progress, run another one just
-  // afterwards, in case things have changed.
-  base::OnceCallback<void()> must_recheck_;
-
   // If a script is currently running, this is the script's executor. Otherwise,
   // this is nullptr.
   std::unique_ptr<ScriptExecutor> executor_;
 
-  // The callback of the pending run script. |executor_| must not be nullptr if
-  // |pending_run_script_callback_| is not nullptr.
-  ScriptExecutor::RunScriptCallback pending_run_script_callback_;
-
   std::string last_server_payload_;
 
   base::WeakPtrFactory<ScriptTracker> weak_ptr_factory_;

components/autofill_assistant/browser/script_tracker_unittest.cc

@@ -271,33 +271,4 @@ TEST_F(ScriptTrackerTest, CheckScriptsAfterDOMChange) {
   ASSERT_THAT(runnable_script_paths(), ElementsAre("script path"));
 }
 
-TEST_F(ScriptTrackerTest, DuplicateCheckCalls) {
-  SupportsScriptResponseProto scripts;
-  AddScript(&scripts, "runnable name", "runnable path", "exists");
-
-  base::OnceCallback<void(bool)> captured_callback;
-  EXPECT_CALL(mock_web_controller_,
-              OnElementCheck(kExistenceCheck, ElementsAre("exists"), _))
-      .WillOnce(CaptureOnceCallback<2>(&captured_callback))
-      .WillOnce(RunOnceCallback<2>(false));
-  SetAndCheckScripts(scripts);
-
-  // At this point, since the callback hasn't been run, there's still a check in
-  // progress. The three calls to CheckScripts will trigger one call to
-  // CheckScript right after first_call has run.
-  for (int i = 0; i < 3; i++) {
-    tracker_.CheckScripts(base::TimeDelta::FromSeconds(0));
-  }
-
-  EXPECT_THAT(runnable_scripts(), IsEmpty());
-  ASSERT_TRUE(captured_callback);
-  std::move(captured_callback).Run(true);
-
-  // The second check is run right away, after the first check, say that the
-  // element doesn't exist anymore, and we end up again with an empty
-  // runnable_scripts.
-  EXPECT_THAT(runnable_scripts(), IsEmpty());
-  EXPECT_EQ(2, runnable_scripts_changed_);
-}
-
 }  // namespace autofill_assistant