[Autofill Assistant] Immediately report an error for "no script".

Before this change, when there were no matching scripts for a page, or
when fetching scripts failed, the client would wait always 20s before
displaying an error message.

With this change, the client complains as soon as it's clear that there
are no scripts for the current page and that there cannot be any on the
current URL.

This change updates the way the case where we don't have a committed URL
yet is handled, to avoid ever checking scripts with an empty URL. Before
this change, the first run of CheckScripts would almost always run
against an empty URL, so all preconditions would fail. With this change,
the first run of CheckScripts runs with the initial URL, the one passed
to CCT.

This change updates most test in ControllerTest, which were written in
such a way as to always start with a no-script answer.

Bug: 806868
Change-Id: I819fce0aadf4263ea0d17e8d58bd4371d5beec00
Reviewed-on: https://chromium-review.googlesource.com/c/1486274
Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635563}

chrome/browser/android/autofill_assistant/client_android.cc

@@ -253,7 +253,12 @@ void ClientAndroid::Shutdown(Metrics::DropOutReason reason) {
   }
 
   Metrics::RecordDropOut(reason);
-  controller_.reset();
+
+  // Delete the controller in a separate task. This avoids tricky ordering
+  // issues when Shutdown is called from the controller.
+  base::PostTaskWithTraits(FROM_HERE, {content::BrowserThread::UI},
+                           base::BindOnce(&ClientAndroid::DestroyController,
+                                          weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ClientAndroid::FetchAccessToken(
@@ -279,6 +284,10 @@ void ClientAndroid::CreateController() {
   controller_ = std::make_unique<Controller>(web_contents_, /* client= */ this);
 }
 
+void ClientAndroid::DestroyController() {
+  controller_.reset();
+}
+
 bool ClientAndroid::NeedsUI() {
   return !ui_controller_android_ && controller_ && controller_->NeedsUI();
 }

chrome/browser/android/autofill_assistant/client_android.h

@@ -86,6 +86,7 @@ class ClientAndroid : public Client,
 
   explicit ClientAndroid(content::WebContents* web_contents);
   void CreateController();
+  void DestroyController();
   bool NeedsUI();
   void SetUI(std::unique_ptr<UiControllerAndroid> ui_controller_android);
 

components/autofill_assistant/browser/controller.cc

@@ -65,6 +65,14 @@ Controller::Controller(content::WebContents* web_contents, Client* client)
 
 Controller::~Controller() = default;
 
+const GURL& Controller::GetCurrentURL() {
+  const GURL& last_committed = web_contents()->GetLastCommittedURL();
+  if (!last_committed.is_empty())
+    return last_committed;
+
+  return initial_url_;
+}
+
 Service* Controller::GetService() {
   if (!service_) {
     service_ = Service::Create(web_contents()->GetBrowserContext(), client_);
@@ -235,11 +243,12 @@ void Controller::SetWebControllerAndServiceForTest(
   service_ = std::move(service);
 }
 
-void Controller::GetOrCheckScripts(const GURL& url) {
+void Controller::GetOrCheckScripts() {
   if (!started_ || script_tracker()->running()) {
     return;
   }
 
+  const GURL& url = GetCurrentURL();
   if (script_domain_ != url.host()) {
     StopPeriodicScriptChecks();
     script_domain_ = url.host();
@@ -307,16 +316,29 @@ void Controller::OnGetScripts(const GURL& url,
     return;
 
   if (!result) {
-    DVLOG(1) << "Failed to get assistant scripts for URL " << url.spec();
-    // TODO(crbug.com/806868): Terminate Autofill Assistant.
+    DVLOG(1) << "Failed to get assistant scripts for " << script_domain_;
+    OnFatalError(l10n_util::GetStringUTF8(IDS_AUTOFILL_ASSISTANT_DEFAULT_ERROR),
+                 Metrics::GET_SCRIPTS_FAILED);
     return;
   }
 
   std::vector<std::unique_ptr<Script>> scripts;
   bool parse_result = ProtocolUtils::ParseScripts(response, &scripts);
-  DVLOG(2) << __func__ << " from " << url.host() << " returned "
+  if (!parse_result) {
+    DVLOG(2) << __func__ << " from " << script_domain_ << " returned "
+             << "unparseable response";
+    OnFatalError(l10n_util::GetStringUTF8(IDS_AUTOFILL_ASSISTANT_DEFAULT_ERROR),
+                 Metrics::GET_SCRIPTS_UNPARSABLE);
+    return;
+  }
+
+  if (scripts.empty()) {
+    OnNoRunnableScripts();
+    return;
+  }
+
+  DVLOG(2) << __func__ << " from " << script_domain_ << " returned "
            << scripts.size() << " scripts";
-  DCHECK(parse_result);
   script_tracker()->SetScripts(std::move(scripts));
   script_tracker()->CheckScripts(kPeriodicScriptCheckInterval);
   StartPeriodicScriptChecks();
@@ -393,7 +415,7 @@ void Controller::OnScriptExecuted(const std::string& script_path,
       break;
   }
   EnterState(AutofillAssistantState::PROMPT);
-  GetOrCheckScripts(web_contents()->GetLastCommittedURL());
+  GetOrCheckScripts();
 }
 
 bool Controller::MaybeAutostartScript(
@@ -434,36 +456,37 @@ bool Controller::MaybeAutostartScript(
   return false;
 }
 
-void Controller::OnGetCookie(const GURL& initial_url, bool has_cookie) {
+void Controller::OnGetCookie(bool has_cookie) {
   if (has_cookie) {
     // This code is only active with the experiment parameter.
     parameters_.insert(
         std::make_pair(kWebsiteVisitedBeforeParameterName, kTrueValue));
-    OnSetCookie(initial_url, has_cookie);
+    OnSetCookie(has_cookie);
     return;
   }
   GetWebController()->SetCookie(
-      initial_url.host(),
+      initial_url_.host(),
       base::BindOnce(&Controller::OnSetCookie,
                      // WebController is owned by Controller.
-                     base::Unretained(this), initial_url));
+                     base::Unretained(this)));
 }
 
-void Controller::OnSetCookie(const GURL& initial_url, bool result) {
+void Controller::OnSetCookie(bool result) {
   DCHECK(result) << "Setting cookie failed";
-  FinishStart(initial_url);
+  FinishStart();
 }
 
-void Controller::FinishStart(const GURL& initial_url) {
+void Controller::FinishStart() {
   started_ = true;
-  GetOrCheckScripts(initial_url);
   if (allow_autostart_) {
     should_fail_after_checking_scripts_ = true;
     MaybeSetInitialDetails();
-    SetStatusMessage(l10n_util::GetStringFUTF8(
-        IDS_AUTOFILL_ASSISTANT_LOADING, base::UTF8ToUTF16(initial_url.host())));
+    SetStatusMessage(
+        l10n_util::GetStringFUTF8(IDS_AUTOFILL_ASSISTANT_LOADING,
+                                  base::UTF8ToUTF16(initial_url_.host())));
     SetProgress(kAutostartInitialProgress);
   }
+  GetOrCheckScripts();
 }
 
 void Controller::MaybeSetInitialDetails() {
@@ -477,22 +500,23 @@ bool Controller::NeedsUI() const {
          state_ != AutofillAssistantState::STOPPED;
 }
 
-void Controller::Start(const GURL& initialUrl,
+void Controller::Start(const GURL& initial_url,
                        const std::map<std::string, std::string>& parameters) {
   if (state_ != AutofillAssistantState::INACTIVE) {
     NOTREACHED();
     return;
   }
   parameters_ = parameters;
+  initial_url_ = initial_url;
   EnterState(AutofillAssistantState::STARTING);
   client_->ShowUI();
   if (IsCookieExperimentEnabled()) {
     GetWebController()->HasCookie(
         base::BindOnce(&Controller::OnGetCookie,
                        // WebController is owned by Controller.
-                       base::Unretained(this), initialUrl));
+                       base::Unretained(this)));
   } else {
-    FinishStart(initialUrl);
+    FinishStart();
   }
 }
 
@@ -584,15 +608,23 @@ void Controller::OnFatalError(const std::string& error_message,
   StopAndShutdown(reason);
 }
 
-void Controller::OnNoRunnableScriptsAnymore() {
+void Controller::OnNoRunnableScripts() {
   if (script_tracker()->running())
     return;
 
+  if (state_ == AutofillAssistantState::STARTING) {
+    // We're still waiting for the set of initial scripts, but either didn't get
+    // any scripts or didn't get scripts that could possibly become runnable
+    // with a DOM change.
+    OnFatalError(l10n_util::GetStringUTF8(IDS_AUTOFILL_ASSISTANT_DEFAULT_ERROR),
+                 Metrics::NO_INITIAL_SCRIPTS);
+    return;
+  }
+
   // We're navigated to a page that has no scripts or the scripts have reached a
   // state from which they cannot recover through a DOM change.
   OnFatalError(l10n_util::GetStringUTF8(IDS_AUTOFILL_ASSISTANT_GIVE_UP),
                Metrics::NO_SCRIPTS);
-  return;
 }
 
 void Controller::OnRunnableScriptsChanged(
@@ -663,7 +695,7 @@ void Controller::DidFinishLoad(content::RenderFrameHost* render_frame_host,
                                const GURL& validated_url) {
   // validated_url might not be the page URL. Ignore it and always check the
   // last committed url.
-  GetOrCheckScripts(web_contents()->GetLastCommittedURL());
+  GetOrCheckScripts();
 }
 
 void Controller::DidStartNavigation(
@@ -695,7 +727,7 @@ void Controller::DidStartNavigation(
 }
 
 void Controller::DocumentAvailableInMainFrame() {
-  GetOrCheckScripts(web_contents()->GetLastCommittedURL());
+  GetOrCheckScripts();
 }
 
 void Controller::RenderProcessGone(base::TerminationStatus status) {

components/autofill_assistant/browser/controller.h

@@ -51,7 +51,7 @@ class Controller : public ScriptExecutorDelegate,
   bool NeedsUI() const;
 
   // Called when autofill assistant can start executing scripts.
-  void Start(const GURL& initialUrl,
+  void Start(const GURL& initial_url,
              const std::map<std::string, std::string>& parameters);
 
   // Initiates a clean shutdown.
@@ -69,6 +69,7 @@ class Controller : public ScriptExecutorDelegate,
   bool Terminate(Metrics::DropOutReason reason);
 
   // Overrides ScriptExecutorDelegate:
+  const GURL& GetCurrentURL() override;
   Service* GetService() override;
   UiController* GetUiController() override;
   WebController* GetWebController() override;
@@ -117,7 +118,7 @@ class Controller : public ScriptExecutorDelegate,
       std::unique_ptr<WebController> web_controller,
       std::unique_ptr<Service> service);
 
-  void GetOrCheckScripts(const GURL& url);
+  void GetOrCheckScripts();
   void OnGetScripts(const GURL& url, bool result, const std::string& response);
   void ExecuteScript(const std::string& script_path);
   void OnScriptExecuted(const std::string& script_path,
@@ -142,16 +143,16 @@ class Controller : public ScriptExecutorDelegate,
   // for the intial URL, we show a warning that the website has already been
   // visited and could contain old data. The cookie is cleared (or expires) when
   // a script terminated with a Stop action.
-  void OnGetCookie(const GURL& initial_url, bool has_cookie);
-  void OnSetCookie(const GURL& initial_url, bool result);
-  void FinishStart(const GURL& initial_url);
+  void OnGetCookie(bool has_cookie);
+  void OnSetCookie(bool result);
+  void FinishStart();
   void MaybeSetInitialDetails();
 
   // Called when a script is selected.
   void OnScriptSelected(const std::string& script_path);
 
   // Overrides ScriptTracker::Listener:
-  void OnNoRunnableScriptsAnymore() override;
+  void OnNoRunnableScripts() override;
   void OnRunnableScriptsChanged(
       const std::vector<ScriptHandle>& runnable_scripts) override;
 
@@ -187,6 +188,9 @@ class Controller : public ScriptExecutorDelegate,
 
   AutofillAssistantState state_ = AutofillAssistantState::INACTIVE;
 
+  // The URL passed to Start(). Used only as long as there's no committed URL.
+  GURL initial_url_;
+
   // Domain of the last URL the controller requested scripts from.
   std::string script_domain_;
   bool allow_autostart_ = true;

components/autofill_assistant/browser/controller_unittest.cc

@@ -96,9 +96,6 @@ class ControllerTest : public content::RenderViewHostTestHarness {
     ON_CALL(*mock_service_, OnGetNextActions(_, _, _, _))
         .WillByDefault(RunOnceCallback<3>(true, ""));
 
-    // Make WebController::GetUrl accessible.
-    ON_CALL(*mock_web_controller_, GetUrl()).WillByDefault(ReturnRef(url_));
-
     ON_CALL(mock_ui_controller_, OnStateChanged(_))
         .WillByDefault(Invoke([this](AutofillAssistantState state) {
           states_.emplace_back(state);
@@ -141,13 +138,13 @@ class ControllerTest : public content::RenderViewHostTestHarness {
         .WillOnce(RunOnceCallback<2>(true, scripts_str));
   }
 
-  void Start() {
-    GURL initialUrl("http://initialurl.com");
-    controller_->Start(initialUrl, /* parameters= */ {});
+  void Start() { Start("http://initialurl.com"); }
+
+  void Start(const std::string& url) {
+    controller_->Start(GURL(url), /* parameters= */ {});
   }
 
   void SetLastCommittedUrl(const GURL& url) {
-    url_ = url;
     tester_->SetLastCommittedURL(url);
   }
 
@@ -186,7 +183,6 @@ class ControllerTest : public content::RenderViewHostTestHarness {
 
   UiDelegate* GetUiDelegate() { return controller_.get(); }
 
-  GURL url_;
   std::vector<AutofillAssistantState> states_;
   MockService* mock_service_;
   MockWebController* mock_web_controller_;
@@ -198,10 +194,6 @@ class ControllerTest : public content::RenderViewHostTestHarness {
 };
 
 TEST_F(ControllerTest, FetchAndRunScripts) {
-  Start();
-
-  // Going to the URL triggers a whole flow:
-  // loading scripts
   SupportsScriptResponseProto script_response;
   AddRunnableScript(&script_response, "script1");
   auto* script2 = AddRunnableScript(&script_response, "script2");
@@ -210,8 +202,7 @@ TEST_F(ControllerTest, FetchAndRunScripts) {
 
   testing::InSequence seq;
 
-  // Start the flow.
-  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+  Start("http://a.example.com/path");
 
   // Offering the choices: script1 and script2
   EXPECT_EQ(AutofillAssistantState::AUTOSTART_FALLBACK_PROMPT,
@@ -232,23 +223,19 @@ TEST_F(ControllerTest, FetchAndRunScripts) {
 }
 
 TEST_F(ControllerTest, ReportPromptAndSuggestionsChanged) {
-  Start();
-
   SupportsScriptResponseProto script_response;
   AddRunnableScript(&script_response, "script1");
   AddRunnableScript(&script_response, "script2");
   SetNextScriptResponse(script_response);
 
   EXPECT_CALL(mock_ui_controller_, OnSuggestionsChanged(SizeIs(2)));
-  EXPECT_CALL(
-      mock_ui_controller_,
-      OnStateChanged(AutofillAssistantState::AUTOSTART_FALLBACK_PROMPT));
-  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+  Start("http://a.example.com/path");
+
+  EXPECT_EQ(AutofillAssistantState::AUTOSTART_FALLBACK_PROMPT,
+            controller_->GetState());
 }
 
 TEST_F(ControllerTest, ClearChipsWhenRunning) {
-  Start();
-
   SupportsScriptResponseProto script_response;
   AddRunnableScript(&script_response, "script1");
   AddRunnableScript(&script_response, "script2");
@@ -266,13 +253,11 @@ TEST_F(ControllerTest, ClearChipsWhenRunning) {
     EXPECT_CALL(mock_ui_controller_, OnSuggestionsChanged(_))
         .Times(AnyNumber());
   }
-  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+  Start("http://a.example.com/path");
   controller_->SelectSuggestion(0);
 }
 
 TEST_F(ControllerTest, ShowFirstInitialStatusMessage) {
-  Start();
-
   SupportsScriptResponseProto script_response;
   AddRunnableScript(&script_response, "script1");
 
@@ -293,17 +278,14 @@ TEST_F(ControllerTest, ShowFirstInitialStatusMessage) {
 
   SetNextScriptResponse(script_response);
 
-  // Start the flow.
-  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+  Start("http://a.example.com/path");
 
+  EXPECT_THAT(controller_->GetSuggestions(), SizeIs(4));
   // Script3, with higher priority (lower number), wins.
   EXPECT_EQ("script3 prompt", controller_->GetStatusMessage());
-  EXPECT_THAT(controller_->GetSuggestions(), SizeIs(4));
 }
 
 TEST_F(ControllerTest, Stop) {
-  Start();
-
   ActionsResponseProto actions_response;
   actions_response.add_actions()->mutable_stop();
   std::string actions_response_str;
@@ -322,8 +304,6 @@ TEST_F(ControllerTest, Stop) {
 }
 
 TEST_F(ControllerTest, Reset) {
-  Start();
-
     // 1. Fetch scripts for URL, which in contains a single "reset" script.
     SupportsScriptResponseProto script_response;
     auto* reset_script = AddRunnableScript(&script_response, "reset");
@@ -333,7 +313,7 @@ TEST_F(ControllerTest, Reset) {
     EXPECT_CALL(*mock_service_, OnGetScriptsForUrl(_, _, _))
         .WillRepeatedly(RunOnceCallback<2>(true, script_response_str));
 
-    SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+    Start("http://a.example.com/path");
     EXPECT_THAT(controller_->GetSuggestions(),
                 ElementsAre(Field(&Chip::text, StrEq("reset"))));
 
@@ -361,7 +341,6 @@ TEST_F(ControllerTest, Reset) {
 }
 
 TEST_F(ControllerTest, RefreshScriptWhenDomainChanges) {
-  Start();
 
   EXPECT_CALL(*mock_service_,
               OnGetScriptsForUrl(Eq(GURL("http://a.example.com/path1")), _, _))
@@ -370,7 +349,7 @@ TEST_F(ControllerTest, RefreshScriptWhenDomainChanges) {
               OnGetScriptsForUrl(Eq(GURL("http://b.example.com/path1")), _, _))
       .WillOnce(RunOnceCallback<2>(true, ""));
 
-  SimulateNavigateToUrl(GURL("http://a.example.com/path1"));
+  Start("http://a.example.com/path1");
   SimulateNavigateToUrl(GURL("http://a.example.com/path2"));
   SimulateNavigateToUrl(GURL("http://b.example.com/path1"));
   SimulateNavigateToUrl(GURL("http://b.example.com/path2"));
@@ -388,8 +367,6 @@ TEST_F(ControllerTest, ForwardParameters) {
 }
 
 TEST_F(ControllerTest, Autostart) {
-  Start();
-
   SupportsScriptResponseProto script_response;
   AddRunnableScript(&script_response, "runnable");
   AddRunnableScript(&script_response, "autostart")
@@ -401,12 +378,10 @@ TEST_F(ControllerTest, Autostart) {
   EXPECT_CALL(*mock_service_, OnGetActions(StrEq("autostart"), _, _, _, _, _))
       .WillOnce(RunOnceCallback<5>(true, ""));
 
-  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+  Start("http://a.example.com/path");
 }
 
 TEST_F(ControllerTest, AutostartFirstInterrupt) {
-  Start();
-
   SupportsScriptResponseProto script_response;
   AddRunnableScript(&script_response, "runnable");
 
@@ -430,12 +405,10 @@ TEST_F(ControllerTest, AutostartFirstInterrupt) {
   // The script fails, ending the flow. What matters is that the correct
   // expectation is met.
 
-  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+  Start("http://a.example.com/path");
 }
 
 TEST_F(ControllerTest, InterruptThenAutostart) {
-  Start();
-
   SupportsScriptResponseProto script_response;
   AddRunnableScript(&script_response, "runnable");
 
@@ -458,12 +431,10 @@ TEST_F(ControllerTest, InterruptThenAutostart) {
                 OnGetActions(StrEq("autostart"), _, _, _, _, _));
   }
 
-  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+  Start("http://a.example.com/path");
 }
 
 TEST_F(ControllerTest, AutostartIsNotPassedToTheUi) {
-  Start();
-
   SupportsScriptResponseProto script_response;
   auto* autostart = AddRunnableScript(&script_response, "runnable");
   autostart->mutable_presentation()->set_autostart(true);
@@ -531,8 +502,6 @@ TEST_F(ControllerTest, IgnoreProgressDecreases) {
 
 TEST_F(ControllerTest, StateChanges) {
   EXPECT_EQ(AutofillAssistantState::INACTIVE, GetUiDelegate()->GetState());
-  Start();
-  EXPECT_EQ(AutofillAssistantState::STARTING, GetUiDelegate()->GetState());
 
   SupportsScriptResponseProto script_response;
   auto* script1 = AddRunnableScript(&script_response, "script1");
@@ -541,10 +510,10 @@ TEST_F(ControllerTest, StateChanges) {
   RunOnce(script2);
   SetNextScriptResponse(script_response);
 
-  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
-
-  EXPECT_EQ(AutofillAssistantState::AUTOSTART_FALLBACK_PROMPT,
-            GetUiDelegate()->GetState());
+  Start("http://a.example.com/path");
+  EXPECT_THAT(states_,
+              ElementsAre(AutofillAssistantState::STARTING,
+                          AutofillAssistantState::AUTOSTART_FALLBACK_PROMPT));
 
   // Run script1: State should become RUNNING, as there's another script, then
   // go back to prompt to propose that script.
@@ -579,7 +548,12 @@ TEST_F(ControllerTest, ShowUIWhenContentsFocused) {
 
   testing::InSequence seq;
   EXPECT_CALL(fake_client_, ShowUI());
+
+  SupportsScriptResponseProto script_response;
+  AddRunnableScript(&script_response, "script1");
+  SetNextScriptResponse(script_response);
   Start();  // must call ShowUI
+
   EXPECT_CALL(fake_client_, ShowUI());
   SimulateWebContentsFocused();  // must call ShowUI
 

components/autofill_assistant/browser/fake_script_executor_delegate.cc

@@ -11,6 +11,10 @@ namespace autofill_assistant {
 FakeScriptExecutorDelegate::FakeScriptExecutorDelegate() = default;
 FakeScriptExecutorDelegate::~FakeScriptExecutorDelegate() = default;
 
+const GURL& FakeScriptExecutorDelegate::GetCurrentURL() {
+  return current_url_;
+}
+
 Service* FakeScriptExecutorDelegate::GetService() {
   return service_;
 }

components/autofill_assistant/browser/fake_script_executor_delegate.h

@@ -22,6 +22,7 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
   FakeScriptExecutorDelegate();
   ~FakeScriptExecutorDelegate() override;
 
+  const GURL& GetCurrentURL() override;
   Service* GetService() override;
   UiController* GetUiController() override;
   WebController* GetWebController() override;
@@ -40,6 +41,8 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
   void SetPaymentRequestOptions(
       std::unique_ptr<PaymentRequestOptions> options) override;
 
+  void SetCurrentURL(const GURL& url) { current_url_ = url; }
+
   void SetService(Service* service) { service_ = service; }
 
   void SetUiController(UiController* ui_controller) {
@@ -63,6 +66,7 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
   PaymentRequestOptions* GetOptions() { return payment_request_options_.get(); }
 
  private:
+  GURL current_url_;
   Service* service_ = nullptr;
   UiController* ui_controller_ = nullptr;
   WebController* web_controller_ = nullptr;

components/autofill_assistant/browser/metrics.h

@@ -35,7 +35,11 @@ class Metrics {
     SAFETY_NET_TERMINATE = 14,  // This is a "should never happen" entry.
     TAB_DETACHED = 15,
     TAB_CHANGED = 16,
-    NUM_ENTRIES = 17,
+    GET_SCRIPTS_FAILED = 17,
+    GET_SCRIPTS_UNPARSABLE = 18,
+    NO_INITIAL_SCRIPTS = 19,
+
+    NUM_ENTRIES = 20,
   };
 
   static void RecordDropOut(DropOutReason reason);
@@ -101,8 +105,21 @@ class Metrics {
       case TAB_CHANGED:
         out << "TAB_CHANGED";
         break;
+
       case NUM_ENTRIES:
         out << "NUM_ENTRIES";
+        break;
+
+      case GET_SCRIPTS_FAILED:
+        out << "GET_SCRIPTS_FAILED";
+        break;
+
+      case GET_SCRIPTS_UNPARSABLE:
+        out << "GET_SCRIPTS_UNPARSEABLE";
+        break;
+
+      case NO_INITIAL_SCRIPTS:
+        out << "NO_INITIAL_SCRIPTS";
         // Intentionally no default case to make compilation fail if a new value
         // was added to the enum but not to this list.
     }

components/autofill_assistant/browser/mock_web_controller.h

@@ -20,8 +20,6 @@ class MockWebController : public WebController {
   MockWebController();
   ~MockWebController() override;
 
-  MOCK_METHOD0(GetUrl, const GURL&());
-
   MOCK_METHOD1(LoadURL, void(const GURL&));
 
   void ClickOrTapElement(const Selector& selector,

components/autofill_assistant/browser/script_executor.cc

@@ -147,11 +147,10 @@ void ScriptExecutor::Run(RunScriptCallback callback) {
   callback_ = std::move(callback);
   DCHECK(delegate_->GetService());
 
-  DVLOG(2) << "GetActions for "
-           << delegate_->GetWebController()->GetUrl().host();
+  DVLOG(2) << "GetActions for " << delegate_->GetCurrentURL().host();
   delegate_->GetService()->GetActions(
-      script_path_, delegate_->GetWebController()->GetUrl(),
-      delegate_->GetParameters(), last_global_payload_, last_script_payload_,
+      script_path_, delegate_->GetCurrentURL(), delegate_->GetParameters(),
+      last_global_payload_, last_script_payload_,
       base::BindOnce(&ScriptExecutor::OnGetActions,
                      weak_ptr_factory_.GetWeakPtr()));
 }
@@ -640,9 +639,8 @@ void ScriptExecutor::WaitWithInterrupts::Run() {
     }
 
     interrupt->precondition->Check(
-        main_script_->delegate_->GetWebController()->GetUrl(),
-        batch_element_checker_.get(), main_script_->delegate_->GetParameters(),
-        *main_script_->scripts_state_,
+        main_script_->delegate_->GetCurrentURL(), batch_element_checker_.get(),
+        main_script_->delegate_->GetParameters(), *main_script_->scripts_state_,
         base::BindOnce(&WaitWithInterrupts::OnPreconditionCheckDone,
                        weak_ptr_factory_.GetWeakPtr(),
                        base::Unretained(interrupt)));

components/autofill_assistant/browser/script_executor_delegate.h

@@ -14,6 +14,7 @@
 #include "components/autofill_assistant/browser/details.h"
 #include "components/autofill_assistant/browser/payment_request.h"
 #include "components/autofill_assistant/browser/state.h"
+#include "url/gurl.h"
 
 namespace autofill {
 class PersonalDataManager;
@@ -32,20 +33,14 @@ class ClientMemory;
 
 class ScriptExecutorDelegate {
  public:
+  virtual const GURL& GetCurrentURL() = 0;
   virtual Service* GetService() = 0;
-
   virtual UiController* GetUiController() = 0;
-
   virtual WebController* GetWebController() = 0;
-
   virtual ClientMemory* GetClientMemory() = 0;
-
   virtual const std::map<std::string, std::string>& GetParameters() = 0;
-
   virtual autofill::PersonalDataManager* GetPersonalDataManager() = 0;
-
   virtual content::WebContents* GetWebContents() = 0;
-
   virtual void EnterState(AutofillAssistantState state) = 0;
 
   // Make the area of the screen that correspond to the given elements

components/autofill_assistant/browser/script_executor_unittest.cc

@@ -51,6 +51,7 @@ class ScriptExecutorTest : public testing::Test,
     delegate_.SetService(&mock_service_);
     delegate_.SetUiController(&mock_ui_controller_);
     delegate_.SetWebController(&mock_web_controller_);
+    delegate_.SetCurrentURL(GURL("http://example.com/"));
 
     executor_ = std::make_unique<ScriptExecutor>(
         kScriptPath,
@@ -58,7 +59,6 @@ class ScriptExecutorTest : public testing::Test,
         /* script_payload= */ "initial payload",
         /* listener= */ this, &scripts_state_, &ordered_interrupts_,
         /* delegate= */ &delegate_);
-    url_ = GURL("http://example.com/");
 
     // In this test, "tell" actions always succeed and "click" actions always
     // fail. The following makes a click action fail immediately
@@ -69,7 +69,6 @@ class ScriptExecutorTest : public testing::Test,
         .WillByDefault(RunOnceCallback<2>(true));
     ON_CALL(mock_web_controller_, OnFocusElement(_, _))
         .WillByDefault(RunOnceCallback<1>(true));
-    ON_CALL(mock_web_controller_, GetUrl()).WillByDefault(ReturnRef(url_));
   }
 
  protected:
@@ -164,7 +163,6 @@ class ScriptExecutorTest : public testing::Test,
   std::unique_ptr<ScriptExecutor> executor_;
   StrictMock<base::MockCallback<ScriptExecutor::RunScriptCallback>>
       executor_callback_;
-  GURL url_;
 };
 
 TEST_F(ScriptExecutorTest, GetActionsFails) {

components/autofill_assistant/browser/script_tracker.cc

@@ -36,7 +36,6 @@ ScriptTracker::ScriptTracker(ScriptExecutorDelegate* delegate,
                              ScriptTracker::Listener* listener)
     : delegate_(delegate),
       listener_(listener),
-      reported_runnable_scripts_(false),
       weak_ptr_factory_(this) {
   DCHECK(delegate_);
   DCHECK(listener_);
@@ -73,6 +72,7 @@ void ScriptTracker::CheckScripts(const base::TimeDelta& max_duration) {
 
   DCHECK(pending_runnable_scripts_.empty());
 
+  GURL url = delegate_->GetCurrentURL();
   batch_element_checker_ =
       delegate_->GetWebController()->CreateBatchElementChecker();
   for (const auto& entry : available_scripts_) {
@@ -81,17 +81,19 @@ void ScriptTracker::CheckScripts(const base::TimeDelta& max_duration) {
       continue;
 
     script->precondition->Check(
-        delegate_->GetWebController()->GetUrl(), batch_element_checker_.get(),
-        delegate_->GetParameters(), scripts_state_,
+        url, batch_element_checker_.get(), delegate_->GetParameters(),
+        scripts_state_,
         base::BindOnce(&ScriptTracker::OnPreconditionCheck,
                        weak_ptr_factory_.GetWeakPtr(), script));
   }
   if (batch_element_checker_->all_found() &&
-      pending_runnable_scripts_.empty() && reported_runnable_scripts_) {
+      pending_runnable_scripts_.empty() && !available_scripts_.empty()) {
+    DVLOG(1) << __func__ << ": No runnable scripts for " << url << " out of "
+             << available_scripts_.size() << " available.";
     // There are no runnable scripts, even though we haven't checked the DOM
     // yet. Report it all immediately.
     UpdateRunnableScriptsIfNecessary();
-    listener_->OnNoRunnableScriptsAnymore();
+    listener_->OnNoRunnableScripts();
     OnCheckDone();
     return;
   }
@@ -204,7 +206,6 @@ void ScriptTracker::UpdateRunnableScriptsIfNecessary() {
     runnable_scripts_.push_back(script->handle);
   }
 
-  reported_runnable_scripts_ = true;
   listener_->OnRunnableScriptsChanged(runnable_scripts_);
 }
 

components/autofill_assistant/browser/script_tracker.h

@@ -45,9 +45,9 @@ class ScriptTracker : public ScriptExecutor::Listener {
     //
     // This is not called if a DOM change could make some scripts runnable.
     //
-    // This is not called until some scripts have been reported runnable to
-    // OnRunnableScriptsChanged at least once.
-    virtual void OnNoRunnableScriptsAnymore() = 0;
+    // This is only called when there are scripts. That is, SetScripts was last
+    // passed a non-empty vector.
+    virtual void OnNoRunnableScripts() = 0;
   };
 
   // |delegate| and |listener| should outlive this object and should not be
@@ -145,11 +145,6 @@ class ScriptTracker : public ScriptExecutor::Listener {
   // the bottom bar.
   std::vector<ScriptHandle> runnable_scripts_;
 
-  // True if OnRunnableScriptsChanged was called at least once - necessarily
-  // with a non-empty set of scripts the first time, since the tracker starts
-  // with an empty set of scripts.
-  bool reported_runnable_scripts_;
-
   // Sets of available scripts. SetScripts resets this and interrupts
   // any pending check.
   AvailableScriptMap available_scripts_;

components/autofill_assistant/browser/script_tracker_unittest.cc

@@ -33,6 +33,8 @@ using ::testing::UnorderedElementsAre;
 class ScriptTrackerTest : public testing::Test, public ScriptTracker::Listener {
  public:
   void SetUp() override {
+    delegate_.SetCurrentURL(GURL("http://www.example.com/"));
+
     ON_CALL(mock_web_controller_,
             OnElementCheck(kExistenceCheck, Eq(Selector({"exists"})), _))
         .WillByDefault(RunOnceCallback<2>(true));
@@ -40,7 +42,6 @@ class ScriptTrackerTest : public testing::Test, public ScriptTracker::Listener {
         mock_web_controller_,
         OnElementCheck(kExistenceCheck, Eq(Selector({"does_not_exist"})), _))
         .WillByDefault(RunOnceCallback<2>(false));
-    ON_CALL(mock_web_controller_, GetUrl()).WillByDefault(ReturnRef(url_));
 
     // Scripts run, but have no actions.
     ON_CALL(mock_service_, OnGetActions(_, _, _, _, _, _))
@@ -64,7 +65,7 @@ class ScriptTrackerTest : public testing::Test, public ScriptTracker::Listener {
     runnable_scripts_ = runnable_scripts;
   }
 
-  void OnNoRunnableScriptsAnymore() override { no_runnable_scripts_anymore_++; }
+  void OnNoRunnableScripts() override { no_runnable_scripts_anymore_++; }
 
   void SetAndCheckScripts(const SupportsScriptResponseProto& response) {
     std::string response_str;
@@ -149,6 +150,7 @@ TEST_F(ScriptTrackerTest, SomeRunnableScripts) {
   ASSERT_THAT(runnable_scripts(), SizeIs(1));
   EXPECT_EQ("runnable name", runnable_scripts()[0].name);
   EXPECT_EQ("runnable path", runnable_scripts()[0].path);
+  EXPECT_EQ(0, no_runnable_scripts_anymore_);
 }
 
 TEST_F(ScriptTrackerTest, DoNotCheckInterruptWithNoName) {
@@ -388,4 +390,24 @@ TEST_F(ScriptTrackerTest, UpdateScriptListFromInterrupt) {
   EXPECT_EQ("update path 2", runnable_scripts()[1].path);
 }
 
+TEST_F(ScriptTrackerTest, NoRunnableScriptsEvenWithDOMChanges) {
+  SupportsScriptResponseProto scripts;
+  auto* script = AddScript(&scripts, "name", "path", "");
+  script->mutable_presentation()->mutable_precondition()->add_path_pattern(
+      "doesnotmatch");
+  SetAndCheckScripts(scripts);
+
+  EXPECT_THAT(runnable_scripts(), SizeIs(0));
+  EXPECT_EQ(1, no_runnable_scripts_anymore_);
+}
+
+TEST_F(ScriptTrackerTest, NoRunnableScriptsWaitingForDOMChanges) {
+  SupportsScriptResponseProto scripts;
+  AddScript(&scripts, "runnable name", "runnable path", "does_not_exist");
+  SetAndCheckScripts(scripts);
+
+  EXPECT_THAT(runnable_scripts(), SizeIs(0));
+  EXPECT_EQ(0, no_runnable_scripts_anymore_);
+}
+
 }  // namespace autofill_assistant

components/autofill_assistant/browser/web_controller.cc

@@ -377,10 +377,6 @@ WebController::FillFormInputData::FillFormInputData() {}
 
 WebController::FillFormInputData::~FillFormInputData() {}
 
-const GURL& WebController::GetUrl() {
-  return web_contents_->GetLastCommittedURL();
-}
-
 void WebController::LoadURL(const GURL& url) {
   DVLOG(3) << __func__ << " " << url;
   web_contents_->GetController().LoadURLWithParams(

components/autofill_assistant/browser/web_controller.h

@@ -20,6 +20,7 @@
 #include "components/autofill_assistant/browser/devtools/devtools_client.h"
 #include "components/autofill_assistant/browser/rectf.h"
 #include "components/autofill_assistant/browser/selector.h"
+#include "url/gurl.h"
 
 namespace autofill {
 class AutofillProfile;
@@ -58,9 +59,6 @@ class WebController {
                 std::unique_ptr<DevtoolsClient> devtools_client);
   virtual ~WebController();
 
-  // Returns the last committed URL of the associated |web_contents_|.
-  virtual const GURL& GetUrl();
-
   // Load |url| in the current tab. Returns immediately, before the new page has
   // been loaded.
   virtual void LoadURL(const GURL& url);

components/autofill_assistant/browser/web_controller_browsertest.cc

@@ -751,10 +751,12 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, ConcurrentGetFieldsValue) {
 }
 
 IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, NavigateToUrl) {
-  EXPECT_EQ(kTargetWebsitePath, web_controller_->GetUrl().path());
+  EXPECT_EQ(kTargetWebsitePath,
+            shell()->web_contents()->GetLastCommittedURL().path());
   web_controller_->LoadURL(GURL(url::kAboutBlankURL));
   WaitForLoadStop(shell()->web_contents());
-  EXPECT_EQ(url::kAboutBlankURL, web_controller_->GetUrl().spec());
+  EXPECT_EQ(url::kAboutBlankURL,
+            shell()->web_contents()->GetLastCommittedURL().spec());
 }
 
 IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, HighlightElement) {