[Autofill Assistant] Make script precondition timeout configurable.

Before this change, the client waited for a hardcoded duration for
scripts to become available and then entered a generic error state.

After this change, the timeout and action can configured by the backend.
When that timeout is reached, the client executes a specific script,
controls what the client does.

This change also subtly modifies the time after which the client stops
waiting for autostart scripts: it now stops either once it's found a
script to autostart or when it's found runnable scripts to propose,
instead of waiting for a script to be started. This unifies operations
and avoid the unlikely but confusing case where there client would
autostart a script after having proposed scripts to run.

For background, see go/autobot-a2m74ux-client-errors

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

chrome/browser/android/autofill_assistant/client_android.cc

@@ -16,6 +16,7 @@
 #include "base/metrics/field_trial_params.h"
 #include "base/no_destructor.h"
 #include "base/task/post_task.h"
+#include "base/time/default_tick_clock.h"
 #include "chrome/browser/android/chrome_feature_list.h"
 #include "chrome/browser/autofill/android/personal_data_manager_android.h"
 #include "chrome/browser/autofill/personal_data_manager_factory.h"
@@ -287,7 +288,8 @@ void ClientAndroid::CreateController() {
   if (controller_) {
     return;
   }
-  controller_ = std::make_unique<Controller>(web_contents_, /* client= */ this);
+  controller_ = std::make_unique<Controller>(
+      web_contents_, /* client= */ this, base::DefaultTickClock::GetInstance());
 }
 
 void ClientAndroid::DestroyController() {

components/autofill_assistant/browser/controller.cc

@@ -12,6 +12,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
+#include "base/time/tick_clock.h"
 #include "base/values.h"
 #include "components/autofill_assistant/browser/metrics.h"
 #include "components/autofill_assistant/browser/protocol_utils.h"
@@ -34,12 +35,9 @@ static constexpr base::TimeDelta kPeriodicScriptCheckInterval =
     base::TimeDelta::FromSeconds(2);
 
 // Number of script checks to run after a call to StartPeriodicScriptChecks.
+// This limit does not apply when in autostart mode.
 static constexpr int kPeriodicScriptCheckCount = 10;
 
-// Maximum number of script checks we should do before failing when trying to
-// autostart.
-static constexpr int kAutostartCheckCountLimit = 5;
-
 // The initial progress to set when autostarting and showing the "Loading..."
 // message.
 static constexpr int kAutostartInitialProgress = 10;
@@ -57,11 +55,13 @@ static const char* const kWebsiteVisitedBeforeParameterName =
 static const char* const kTrueValue = "true";
 }  // namespace
 
-Controller::Controller(content::WebContents* web_contents, Client* client)
+Controller::Controller(content::WebContents* web_contents,
+                       Client* client,
+                       const base::TickClock* tick_clock)
     : content::WebContentsObserver(web_contents),
       client_(client),
-      weak_ptr_factory_(this) {
-}
+      tick_clock_(tick_clock),
+      weak_ptr_factory_(this) {}
 
 Controller::~Controller() = default;
 
@@ -231,6 +231,7 @@ void Controller::EnterState(AutofillAssistantState state) {
 
   DCHECK_NE(state_, AutofillAssistantState::STOPPED)
       << "Unexpected transition from " << state_ << " to " << state;
+  DVLOG(2) << __func__ << ": " << state_ << " -> " << state;
 
   state_ = state;
   GetUiController()->OnStateChanged(state);
@@ -281,21 +282,26 @@ void Controller::StopPeriodicScriptChecks() {
 }
 
 void Controller::OnPeriodicScriptCheck() {
-  if (periodic_script_check_count_ <= 0) {
+  if (periodic_script_check_count_ > 0) {
+    periodic_script_check_count_--;
+  }
+
+  if (periodic_script_check_count_ <= 0 && !allow_autostart_) {
     DCHECK_EQ(0, periodic_script_check_count_);
     periodic_script_check_scheduled_ = false;
     return;
   }
 
-  if (should_fail_after_checking_scripts_ &&
-      ++total_script_check_count_ >= kAutostartCheckCountLimit) {
-    should_fail_after_checking_scripts_ = false;
-    OnFatalError(l10n_util::GetStringUTF8(IDS_AUTOFILL_ASSISTANT_DEFAULT_ERROR),
-                 Metrics::AUTOSTART_TIMEOUT);
+  if (allow_autostart_ && !autostart_timeout_script_path_.empty() &&
+      tick_clock_->NowTicks() >= absolute_autostart_timeout_) {
+    DVLOG(1) << __func__ << " giving up waiting on autostart.";
+    std::string script_path = autostart_timeout_script_path_;
+    autostart_timeout_script_path_.clear();
+    periodic_script_check_scheduled_ = false;
+    ExecuteScript(script_path, state_);
     return;
   }
 
-  periodic_script_check_count_--;
   script_tracker()->CheckScripts(kPeriodicScriptCheckInterval);
   base::PostDelayedTaskWithTraits(
       FROM_HERE, {content::BrowserThread::UI},
@@ -322,9 +328,8 @@ void Controller::OnGetScripts(const GURL& url,
     return;
   }
 
-  std::vector<std::unique_ptr<Script>> scripts;
-  bool parse_result = ProtocolUtils::ParseScripts(response, &scripts);
-  if (!parse_result) {
+  SupportsScriptResponseProto response_proto;
+  if (!response_proto.ParseFromString(response)) {
     DVLOG(2) << __func__ << " from " << script_domain_ << " returned "
              << "unparseable response";
     OnFatalError(l10n_util::GetStringUTF8(IDS_AUTOFILL_ASSISTANT_DEFAULT_ERROR),
@@ -332,11 +337,23 @@ void Controller::OnGetScripts(const GURL& url,
     return;
   }
 
+  std::vector<std::unique_ptr<Script>> scripts;
+  for (const auto& script_proto : response_proto.scripts()) {
+    ProtocolUtils::AddScript(script_proto, &scripts);
+  }
   if (scripts.empty()) {
     OnNoRunnableScripts();
     return;
   }
 
+  if (allow_autostart_) {
+    autostart_timeout_script_path_ =
+        response_proto.script_timeout_error().script_path();
+    autostart_timeout_ = base::TimeDelta::FromMilliseconds(
+        response_proto.script_timeout_error().timeout_ms());
+    absolute_autostart_timeout_ = tick_clock_->NowTicks() + autostart_timeout_;
+  }
+
   DVLOG(2) << __func__ << " from " << script_domain_ << " returned "
            << scripts.size() << " scripts";
   script_tracker()->SetScripts(std::move(scripts));
@@ -344,7 +361,8 @@ void Controller::OnGetScripts(const GURL& url,
   StartPeriodicScriptChecks();
 }
 
-void Controller::ExecuteScript(const std::string& script_path) {
+void Controller::ExecuteScript(const std::string& script_path,
+                               AutofillAssistantState end_state) {
   DCHECK(!script_tracker()->running());
   EnterState(AutofillAssistantState::RUNNING);
 
@@ -358,12 +376,14 @@ void Controller::ExecuteScript(const std::string& script_path) {
   // TODO(crbug.com/806868): Consider making ClearRunnableScripts part of
   // ExecuteScripts to simplify the controller.
   script_tracker()->ExecuteScript(
-      script_path, base::BindOnce(&Controller::OnScriptExecuted,
-                                  // script_tracker_ is owned by Controller.
-                                  base::Unretained(this), script_path));
+      script_path,
+      base::BindOnce(&Controller::OnScriptExecuted,
+                     // script_tracker_ is owned by Controller.
+                     base::Unretained(this), script_path, end_state));
 }
 
 void Controller::OnScriptExecuted(const std::string& script_path,
+                                  AutofillAssistantState end_state,
                                   const ScriptExecutor::Result& result) {
   if (!result.success) {
     DVLOG(1) << "Failed to execute script " << script_path;
@@ -414,19 +434,19 @@ void Controller::OnScriptExecuted(const std::string& script_path,
       DVLOG(1) << "Unexpected value for at_end: " << result.at_end;
       break;
   }
-  EnterState(AutofillAssistantState::PROMPT);
+  EnterState(end_state);
   GetOrCheckScripts();
 }
 
 bool Controller::MaybeAutostartScript(
     const std::vector<ScriptHandle>& runnable_scripts) {
-  // We want to g through all runnable autostart interrupts first, one at a
+  // We want to go through all runnable autostart interrupts first, one at a
   // time. To do that, always run highest priority autostartable interrupt from
   // runnable_script, which is ordered by priority.
   for (const auto& script : runnable_scripts) {
     if (script.autostart && script.interrupt) {
       std::string script_path = script.path;
-      ExecuteScript(script_path);
+      ExecuteScript(script_path, state_);
       // making a copy of script.path is necessary, as ExecuteScript clears
       // runnable_scripts, so script.path will not survive until the end of
       // ExecuteScript.
@@ -448,14 +468,19 @@ bool Controller::MaybeAutostartScript(
       }
     }
     if (autostart_count == 1) {
-      allow_autostart_ = false;
-      ExecuteScript(autostart_path);
+      DisableAutostart();
+      ExecuteScript(autostart_path, AutofillAssistantState::PROMPT);
       return true;
     }
   }
   return false;
 }
 
+void Controller::DisableAutostart() {
+  allow_autostart_ = false;
+  autostart_timeout_script_path_.clear();
+}
+
 void Controller::OnGetCookie(bool has_cookie) {
   if (has_cookie) {
     // This code is only active with the experiment parameter.
@@ -479,7 +504,6 @@ void Controller::OnSetCookie(bool result) {
 void Controller::FinishStart() {
   started_ = true;
   if (allow_autostart_) {
-    should_fail_after_checking_scripts_ = true;
     MaybeSetInitialDetails();
     SetStatusMessage(
         l10n_util::GetStringFUTF8(IDS_AUTOFILL_ASSISTANT_LOADING,
@@ -539,11 +563,7 @@ bool Controller::Terminate(Metrics::DropOutReason reason) {
 
 void Controller::OnScriptSelected(const std::string& script_path) {
   DCHECK(!script_path.empty());
-
-  // This is a script selected from the UI, so it should disable autostart.
-  allow_autostart_ = false;
-
-  ExecuteScript(script_path);
+  ExecuteScript(script_path, AutofillAssistantState::PROMPT);
 }
 
 void Controller::UpdateTouchableArea() {
@@ -634,10 +654,6 @@ void Controller::OnRunnableScriptsChanged(
   if (script_tracker()->running() || state_ == AutofillAssistantState::STOPPED)
     return;
 
-  if (!runnable_scripts.empty()) {
-    should_fail_after_checking_scripts_ = false;
-  }
-
   if (MaybeAutostartScript(runnable_scripts)) {
     return;
   }
@@ -665,11 +681,15 @@ void Controller::OnRunnableScriptsChanged(
   }
   SetDefaultChipType(chips.get());
 
-  if (allow_autostart_) {
+  if (chips->empty() && state_ == AutofillAssistantState::STARTING) {
+    // Continue waiting
+    return;
+  }
+
+  if (allow_autostart_ ||
+      state_ == AutofillAssistantState::AUTOSTART_FALLBACK_PROMPT) {
     // Autostart was expected, but only non-autostartable scripts were found.
-    //
-    // TODO(crbug.com/806868): Consider the case where no non-autostartable
-    // scripts were found.
+    DisableAutostart();
     EnterState(AutofillAssistantState::AUTOSTART_FALLBACK_PROMPT);
   } else {
     EnterState(AutofillAssistantState::PROMPT);

components/autofill_assistant/browser/controller.h

@@ -31,6 +31,10 @@ class RenderFrameHost;
 class WebContents;
 }  // namespace content
 
+namespace base {
+class TickClock;
+}  // namespace base
+
 namespace autofill_assistant {
 class ControllerTest;
 
@@ -42,9 +46,11 @@ class Controller : public ScriptExecutorDelegate,
                    public ScriptTracker::Listener,
                    private content::WebContentsObserver {
  public:
-  // |web_contents| and |client| must remain valid for the lifetime of the
-  // instance.
-  Controller(content::WebContents* web_contents, Client* client);
+  // |web_contents|, |client| and |tick_clock| must remain valid for the
+  // lifetime of the instance.
+  Controller(content::WebContents* web_contents,
+             Client* client,
+             const base::TickClock* tick_clock);
   ~Controller() override;
 
   // Returns true if the controller is in a state where UI is necessary.
@@ -120,8 +126,13 @@ class Controller : public ScriptExecutorDelegate,
 
   void GetOrCheckScripts();
   void OnGetScripts(const GURL& url, bool result, const std::string& response);
-  void ExecuteScript(const std::string& script_path);
+
+  // Execute |script_path| and, if execution succeeds, enter |end_state| and
+  // call |on_success|.
+  void ExecuteScript(const std::string& script_path,
+                     AutofillAssistantState end_state);
   void OnScriptExecuted(const std::string& script_path,
+                        AutofillAssistantState end_state,
                         const ScriptExecutor::Result& result);
 
   // Check script preconditions every few seconds for a certain number of times.
@@ -137,6 +148,8 @@ class Controller : public ScriptExecutorDelegate,
   // right. Returns true if a script was auto-started.
   bool MaybeAutostartScript(const std::vector<ScriptHandle>& runnable_scripts);
 
+  void DisableAutostart();
+
   // Autofill Assistant cookie logic.
   //
   // On startup of the controller we set a cookie. If a cookie already existed
@@ -175,6 +188,7 @@ class Controller : public ScriptExecutorDelegate,
   ScriptTracker* script_tracker();
 
   Client* const client_;
+  const base::TickClock* const tick_clock_;
 
   // Lazily instantiate in GetWebController().
   std::unique_ptr<WebController> web_controller_;
@@ -200,11 +214,18 @@ class Controller : public ScriptExecutorDelegate,
 
   // Number of remaining periodic checks.
   int periodic_script_check_count_ = 0;
-  int total_script_check_count_ = 0;
 
-  // Whether we should hide the overlay and show an error message after a first
-  // unsuccessful round of preconditions checking.
-  bool should_fail_after_checking_scripts_ = false;
+  // Run this script if no scripts become autostartable after
+  // absolute_autostart_timeout.
+  //
+  // Ignored unless |allow_autostart_| is true.
+  std::string autostart_timeout_script_path_;
+
+  // How long to wait for an autostartable script before failing.
+  base::TimeDelta autostart_timeout_;
+
+  // Ticks at which we'll have reached |autostart_timeout_|.
+  base::TimeTicks absolute_autostart_timeout_;
 
   // Area of the screen that corresponds to the current set of touchable
   // elements.

components/autofill_assistant/browser/controller_unittest.cc

@@ -13,7 +13,8 @@
 #include "components/autofill_assistant/browser/mock_ui_controller.h"
 #include "components/autofill_assistant/browser/mock_web_controller.h"
 #include "components/autofill_assistant/browser/service.h"
-#include "content/public/test/test_renderer_host.h"
+#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/web_contents_tester.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
@@ -68,20 +69,25 @@ class FakeClient : public Client {
 
 }  // namespace
 
-class ControllerTest : public content::RenderViewHostTestHarness {
+class ControllerTest : public testing::Test {
  public:
-  ControllerTest() : fake_client_(&mock_ui_controller_) {}
+  ControllerTest()
+      : thread_bundle_(
+            base::test::ScopedTaskEnvironment::MainThreadType::UI_MOCK_TIME),
+        web_contents_(
+            content::WebContentsTester::CreateTestWebContents(&browser_context_,
+                                                              nullptr)),
+        fake_client_(&mock_ui_controller_) {}
   ~ControllerTest() override {}
 
   void SetUp() override {
-    content::RenderViewHostTestHarness::SetUp();
-
     auto web_controller = std::make_unique<NiceMock<MockWebController>>();
     mock_web_controller_ = web_controller.get();
     auto service = std::make_unique<NiceMock<MockService>>();
     mock_service_ = service.get();
 
-    controller_ = std::make_unique<Controller>(web_contents(), &fake_client_);
+    controller_ = std::make_unique<Controller>(
+        web_contents_.get(), &fake_client_, thread_bundle_.GetMockTickClock());
     controller_->SetWebControllerAndServiceForTest(std::move(web_controller),
                                                    std::move(service));
 
@@ -101,15 +107,8 @@ class ControllerTest : public content::RenderViewHostTestHarness {
           states_.emplace_back(state);
         }));
 
-    tester_ = content::WebContentsTester::For(web_contents());
-  }
-
-  void TearDown() override {
-    // Controller must be deleted before the WebContents, owned by
-    // RenderViewHostTestHarness. In production, this is guaranteed by
-    // autofill_assistant::ClientAndroid, which owns Controller.
-    controller_.reset();
-    content::RenderViewHostTestHarness::TearDown();
+    ON_CALL(*mock_web_controller_, OnElementCheck(_, _, _))
+        .WillByDefault(RunOnceCallback<2>(false));
   }
 
  protected:
@@ -145,7 +144,8 @@ class ControllerTest : public content::RenderViewHostTestHarness {
   }
 
   void SetLastCommittedUrl(const GURL& url) {
-    tester_->SetLastCommittedURL(url);
+    content::WebContentsTester::For(web_contents_.get())
+        ->SetLastCommittedURL(url);
   }
 
   // Updates the current url of the controller and forces a refresh, without
@@ -179,12 +179,17 @@ class ControllerTest : public content::RenderViewHostTestHarness {
 
   UiDelegate* GetUiDelegate() { return controller_.get(); }
 
+  // |thread_bundle_| must be the first field, to make sure that everything runs
+  // in the same task environment.
+  content::TestBrowserThreadBundle thread_bundle_;
+  content::TestBrowserContext browser_context_;
+  std::unique_ptr<content::WebContents> web_contents_;
+  base::TimeTicks now_;
   std::vector<AutofillAssistantState> states_;
   MockService* mock_service_;
   MockWebController* mock_web_controller_;
   NiceMock<FakeClient> fake_client_;
   NiceMock<MockUiController> mock_ui_controller_;
-  content::WebContentsTester* tester_;
 
   std::unique_ptr<Controller> controller_;
 };
@@ -218,6 +223,38 @@ TEST_F(ControllerTest, FetchAndRunScripts) {
               ElementsAre(Field(&Chip::text, StrEq("script1"))));
 }
 
+TEST_F(ControllerTest, NoScripts) {
+  SupportsScriptResponseProto empty;
+  SetNextScriptResponse(empty);
+
+  Start("http://a.example.com/path");
+  EXPECT_EQ(AutofillAssistantState::STOPPED, controller_->GetState());
+}
+
+TEST_F(ControllerTest, NoRelevantScripts) {
+  SupportsScriptResponseProto script_response;
+  AddRunnableScript(&script_response, "no_match")
+      ->mutable_presentation()
+      ->mutable_precondition()
+      ->add_domain("http://otherdomain.com");
+  SetNextScriptResponse(script_response);
+
+  Start("http://a.example.com/path");
+  EXPECT_EQ(AutofillAssistantState::STOPPED, controller_->GetState());
+}
+
+TEST_F(ControllerTest, NoRelevantScriptYet) {
+  SupportsScriptResponseProto script_response;
+  AddRunnableScript(&script_response, "no_match_yet")
+      ->mutable_presentation()
+      ->mutable_precondition()
+      ->add_elements_exist()
+      ->add_selectors("#element");
+  SetNextScriptResponse(script_response);
+
+  Start("http://a.example.com/path");
+  EXPECT_EQ(AutofillAssistantState::STARTING, controller_->GetState());
+}
 TEST_F(ControllerTest, ReportPromptAndSuggestionsChanged) {
   SupportsScriptResponseProto script_response;
   AddRunnableScript(&script_response, "script1");
@@ -565,4 +602,100 @@ TEST_F(ControllerTest, ShowUIWhenContentsFocused) {
   SimulateWebContentsFocused();  // must not call ShowUI
 }
 
+TEST_F(ControllerTest, KeepCheckingForElement) {
+  SupportsScriptResponseProto script_response;
+  AddRunnableScript(&script_response, "no_match_yet")
+      ->mutable_presentation()
+      ->mutable_precondition()
+      ->add_elements_exist()
+      ->add_selectors("#element");
+  SetNextScriptResponse(script_response);
+
+  Start("http://a.example.com/path");
+  // No scripts yet; the element doesn't exit.
+  EXPECT_EQ(AutofillAssistantState::STARTING, controller_->GetState());
+
+  for (int i = 0; i < 3; i++) {
+    thread_bundle_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+    EXPECT_EQ(AutofillAssistantState::STARTING, controller_->GetState());
+  }
+
+  EXPECT_CALL(*mock_web_controller_, OnElementCheck(_, _, _))
+      .WillRepeatedly(RunOnceCallback<2>(true));
+  thread_bundle_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  EXPECT_EQ(AutofillAssistantState::AUTOSTART_FALLBACK_PROMPT,
+            controller_->GetState());
+}
+
+TEST_F(ControllerTest, ScriptTimeoutError) {
+  // Wait for #element to show up for will_never_match. After 25s, execute the
+  // script on_timeout_error.
+  SupportsScriptResponseProto script_response;
+  AddRunnableScript(&script_response, "will_never_match")
+      ->mutable_presentation()
+      ->mutable_precondition()
+      ->add_elements_exist()
+      ->add_selectors("#element");
+  script_response.mutable_script_timeout_error()->set_timeout_ms(30000);
+  script_response.mutable_script_timeout_error()->set_script_path(
+      "on_timeout_error");
+  SetNextScriptResponse(script_response);
+
+  // on_timeout_error stops everything with a custom error message.
+  ActionsResponseProto on_timeout_error;
+  on_timeout_error.add_actions()->mutable_tell()->set_message("I give up");
+  on_timeout_error.add_actions()->mutable_stop();
+  std::string on_timeout_error_str;
+  on_timeout_error.SerializeToString(&on_timeout_error_str);
+  EXPECT_CALL(*mock_service_,
+              OnGetActions(StrEq("on_timeout_error"), _, _, _, _, _))
+      .WillOnce(RunOnceCallback<5>(true, on_timeout_error_str));
+
+  Start("http://a.example.com/path");
+  for (int i = 0; i < 30; i++) {
+    EXPECT_EQ(AutofillAssistantState::STARTING, controller_->GetState());
+    thread_bundle_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+  }
+  EXPECT_EQ(AutofillAssistantState::STOPPED, controller_->GetState());
+  EXPECT_EQ("I give up", controller_->GetStatusMessage());
+}
+
+TEST_F(ControllerTest, ScriptTimeoutWarning) {
+  // Wait for #element to show up for will_never_match. After 10s, execute the
+  // script on_timeout_error.
+  SupportsScriptResponseProto script_response;
+  AddRunnableScript(&script_response, "will_never_match")
+      ->mutable_presentation()
+      ->mutable_precondition()
+      ->add_elements_exist()
+      ->add_selectors("#element");
+  script_response.mutable_script_timeout_error()->set_timeout_ms(4000);
+  script_response.mutable_script_timeout_error()->set_script_path(
+      "on_timeout_error");
+  SetNextScriptResponse(script_response);
+
+  // on_timeout_error displays an error message and terminates
+  ActionsResponseProto on_timeout_error;
+  on_timeout_error.add_actions()->mutable_tell()->set_message("This is slow");
+  std::string on_timeout_error_str;
+  on_timeout_error.SerializeToString(&on_timeout_error_str);
+  EXPECT_CALL(*mock_service_,
+              OnGetActions(StrEq("on_timeout_error"), _, _, _, _, _))
+      .WillOnce(RunOnceCallback<5>(true, on_timeout_error_str));
+
+  Start("http://a.example.com/path");
+
+  // Warning after 4s, script succeeds and the client continues to wait.
+  for (int i = 0; i < 4; i++) {
+    EXPECT_EQ(AutofillAssistantState::STARTING, controller_->GetState());
+    thread_bundle_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+  }
+  EXPECT_EQ(AutofillAssistantState::STARTING, controller_->GetState());
+  EXPECT_EQ("This is slow", controller_->GetStatusMessage());
+  for (int i = 0; i < 10; i++) {
+    EXPECT_EQ(AutofillAssistantState::STARTING, controller_->GetState());
+    thread_bundle_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+  }
+}
 }  // namespace autofill_assistant

components/autofill_assistant/browser/protocol_utils.cc

@@ -65,26 +65,6 @@ std::string ProtocolUtils::CreateGetScriptsRequest(
   return serialized_script_proto;
 }
 
-// static
-bool ProtocolUtils::ParseScripts(
-    const std::string& response,
-    std::vector<std::unique_ptr<Script>>* scripts) {
-  DCHECK(scripts);
-
-  SupportsScriptResponseProto response_proto;
-  if (!response_proto.ParseFromString(response)) {
-    LOG(ERROR) << "Failed to parse getting assistant scripts response.";
-    return false;
-  }
-
-  scripts->clear();
-  for (const auto& script_proto : response_proto.scripts()) {
-    ProtocolUtils::AddScript(script_proto, scripts);
-  }
-
-  return true;
-}
-
 // static
 void ProtocolUtils::AddScript(const SupportedScriptProto& script_proto,
                               std::vector<std::unique_ptr<Script>>* scripts) {

components/autofill_assistant/browser/protocol_utils.h

@@ -28,18 +28,6 @@ class ProtocolUtils {
       const std::map<std::string, std::string>& parameters,
       const ClientContextProto& client_context);
 
-  using Scripts = std::map<Script*, std::unique_ptr<Script>>;
-  // Parse assistant scripts from the given |response|, which should not be an
-  // empty string.
-  //
-  // Parsed assistant scripts are returned through |scripts|, which should not
-  // be nullptr. Returned scripts are guaranteed to be fully initialized, and
-  // have a name, path and precondition.
-  //
-  // Return false if parse failed, otherwise return true.
-  static bool ParseScripts(const std::string& response,
-                           std::vector<std::unique_ptr<Script>>* scripts);
-
   // Convert |script_proto| to a script struct and if the script is valid, add
   // it to |scripts|.
   static void AddScript(const SupportedScriptProto& script_proto,

components/autofill_assistant/browser/protocol_utils_unittest.cc

@@ -28,48 +28,49 @@ void AssertClientContext(const ClientContextProto& context) {
   EXPECT_EQ("v", context.chrome().chrome_version());
 }
 
-TEST(ProtocolUtilsTest, NoScripts) {
+TEST(ProtocolUtilsTest, ScriptMissingPath) {
+  SupportedScriptProto script;
+  script.mutable_presentation()->set_name("missing path");
   std::vector<std::unique_ptr<Script>> scripts;
-  EXPECT_TRUE(ProtocolUtils::ParseScripts("", &scripts));
+  ProtocolUtils::AddScript(script, &scripts);
+
   EXPECT_THAT(scripts, IsEmpty());
 }
 
-TEST(ProtocolUtilsTest, SomeInvalidScripts) {
-  SupportsScriptResponseProto proto;
+TEST(ProtocolUtilsTest, ScriptMissingName) {
+  SupportedScriptProto script;
+  script.set_path("missing name");
+  std::vector<std::unique_ptr<Script>> scripts;
+  ProtocolUtils::AddScript(script, &scripts);
 
-  // 2 Invalid scripts, 1 valid one, with no preconditions.
-  proto.add_scripts()->mutable_presentation()->set_name("missing path");
-  proto.add_scripts()->set_path("missing name");
-  SupportedScriptProto* script = proto.add_scripts();
-  script->set_path("ok");
-  script->mutable_presentation()->set_name("ok name");
+  EXPECT_THAT(scripts, IsEmpty());
+}
 
-  // Only the valid script is returned.
+TEST(ProtocolUtilsTest, MinimalValidScript) {
+  SupportedScriptProto script;
+  script.set_path("path");
+  script.mutable_presentation()->set_name("name");
   std::vector<std::unique_ptr<Script>> scripts;
-  std::string proto_str;
-  proto.SerializeToString(&proto_str);
-  EXPECT_TRUE(ProtocolUtils::ParseScripts(proto_str, &scripts));
+  ProtocolUtils::AddScript(script, &scripts);
+
   ASSERT_THAT(scripts, SizeIs(1));
-  EXPECT_EQ("ok", scripts[0]->handle.path);
-  EXPECT_EQ("ok name", scripts[0]->handle.name);
+  EXPECT_EQ("path", scripts[0]->handle.path);
+  EXPECT_EQ("name", scripts[0]->handle.name);
   EXPECT_NE(nullptr, scripts[0]->precondition);
 }
 
 TEST(ProtocolUtilsTest, OneFullyFeaturedScript) {
-  SupportsScriptResponseProto proto;
-
-  SupportedScriptProto* script = proto.add_scripts();
-  script->set_path("path");
-  auto* presentation = script->mutable_presentation();
+  SupportedScriptProto script_proto;
+  script_proto.set_path("path");
+  auto* presentation = script_proto.mutable_presentation();
   presentation->set_name("name");
   presentation->set_autostart(true);
   presentation->set_initial_prompt("prompt");
   presentation->mutable_precondition()->add_domain("www.example.com");
 
   std::vector<std::unique_ptr<Script>> scripts;
-  std::string proto_str;
-  proto.SerializeToString(&proto_str);
-  EXPECT_TRUE(ProtocolUtils::ParseScripts(proto_str, &scripts));
+  ProtocolUtils::AddScript(script_proto, &scripts);
+
   ASSERT_THAT(scripts, SizeIs(1));
   EXPECT_EQ("path", scripts[0]->handle.path);
   EXPECT_EQ("name", scripts[0]->handle.name);
@@ -79,20 +80,16 @@ TEST(ProtocolUtilsTest, OneFullyFeaturedScript) {
 }
 
 TEST(ProtocolUtilsTest, AllowInterruptsWithNoName) {
-  SupportsScriptResponseProto proto;
-
-  SupportedScriptProto* script = proto.add_scripts();
-  script->set_path("path");
-  auto* presentation = script->mutable_presentation();
+  SupportedScriptProto script_proto;
+  script_proto.set_path("path");
+  auto* presentation = script_proto.mutable_presentation();
   presentation->set_autostart(true);
   presentation->set_initial_prompt("prompt");
   presentation->set_interrupt(true);
   presentation->mutable_precondition()->add_domain("www.example.com");
 
   std::vector<std::unique_ptr<Script>> scripts;
-  std::string proto_str;
-  proto.SerializeToString(&proto_str);
-  EXPECT_TRUE(ProtocolUtils::ParseScripts(proto_str, &scripts));
+  ProtocolUtils::AddScript(script_proto, &scripts);
   ASSERT_THAT(scripts, SizeIs(1));
   EXPECT_EQ("path", scripts[0]->handle.path);
   EXPECT_EQ("", scripts[0]->handle.name);

components/autofill_assistant/browser/service.proto

@@ -55,6 +55,22 @@ message ScriptParameterProto {
 // Response of the list of supported scripts.
 message SupportsScriptResponseProto {
   repeated SupportedScriptProto scripts = 1;
+
+  // Defines what should happen if no scripts in [scripts] becomes runnable,
+  // because of preconditions.
+  optional ScriptTimeoutError script_timeout_error = 2;
+}
+
+message ScriptTimeoutError {
+  // Wait for that long before considering that scripts preconditions have timed
+  // out and executing the script specified in script_path.
+  //
+  // The script might be called more than once if the script terminates
+  // successfully and again still nothing is found after timeout_ms.
+  optional int32 timeout_ms = 1;
+
+  // The script to execute when the error happens.
+  optional string script_path = 2;
 }
 
 // Supported script.