[Autofill Assistant] Add configurable timeout to trigger scripts.

Bug: b/171776026
Change-Id: I84fddd0353aedea9d9fd1878dfb04b24e550fa5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532251
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Reviewed-by: Marian Fechete <marianfe@google.com>
Cr-Commit-Position: refs/heads/master@{#826744}

components/autofill_assistant/browser/protocol_utils.cc

@@ -309,9 +309,13 @@ std::string ProtocolUtils::CreateGetTriggerScriptsRequest(
 bool ProtocolUtils::ParseTriggerScripts(
     const std::string& response,
     std::vector<std::unique_ptr<TriggerScript>>* trigger_scripts,
-    std::vector<std::string>* additional_allowed_domains) {
+    std::vector<std::string>* additional_allowed_domains,
+    int* trigger_condition_check_interval_ms,
+    base::Optional<int>* timeout_ms) {
   DCHECK(trigger_scripts);
   DCHECK(additional_allowed_domains);
+  DCHECK(trigger_condition_check_interval_ms);
+  DCHECK(timeout_ms);
 
   GetTriggerScriptsResponseProto response_proto;
   if (!response_proto.ParseFromString(response)) {
@@ -328,6 +332,12 @@ bool ProtocolUtils::ParseTriggerScripts(
        response_proto.additional_allowed_domains()) {
     additional_allowed_domains->emplace_back(allowed_domain);
   }
+
+  *trigger_condition_check_interval_ms =
+      response_proto.trigger_condition_check_interval_ms();
+  if (response_proto.has_timeout_ms()) {
+    *timeout_ms = response_proto.timeout_ms();
+  }
   return true;
 }
 

components/autofill_assistant/browser/protocol_utils.h

@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 
+#include "base/optional.h"
 #include "components/autofill_assistant/browser/actions/action.h"
 #include "components/autofill_assistant/browser/script.h"
 #include "components/autofill_assistant/browser/service.pb.h"
@@ -86,7 +87,9 @@ class ProtocolUtils {
   static bool ParseTriggerScripts(
       const std::string& response,
       std::vector<std::unique_ptr<TriggerScript>>* trigger_scripts,
-      std::vector<std::string>* additional_allowed_domains);
+      std::vector<std::string>* additional_allowed_domains,
+      int* trigger_condition_check_interval_ms,
+      base::Optional<int>* timeout_ms);
 
  private:
   // To avoid instantiate this class by accident.

components/autofill_assistant/browser/protocol_utils_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/autofill_assistant/browser/protocol_utils.h"
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "components/autofill_assistant/browser/selector.h"
 #include "components/autofill_assistant/browser/service.pb.h"
 #include "components/autofill_assistant/browser/test_util.h"
@@ -328,8 +329,11 @@ TEST_F(ProtocolUtilsTest, ParseActionsUpdateScriptListFullFeatured) {
 TEST_F(ProtocolUtilsTest, ParseTriggerScriptsParseError) {
   std::vector<std::unique_ptr<TriggerScript>> trigger_scripts;
   std::vector<std::string> additional_allowed_domains;
+  int interval_ms;
+  base::Optional<int> timeout_ms;
   EXPECT_FALSE(ProtocolUtils::ParseTriggerScripts("invalid", &trigger_scripts,
-                                                  &additional_allowed_domains));
+                                                  &additional_allowed_domains,
+                                                  &interval_ms, &timeout_ms));
   EXPECT_TRUE(trigger_scripts.empty());
 }
 
@@ -352,6 +356,9 @@ TEST_F(ProtocolUtilsTest, ParseTriggerScriptsValid) {
   proto.add_additional_allowed_domains("example.com");
   proto.add_additional_allowed_domains("other-example.com");
 
+  proto.set_trigger_condition_check_interval_ms(2000);
+  proto.set_timeout_ms(500000);
+
   TriggerScriptProto trigger_script_1;
   *trigger_script_1.mutable_trigger_condition()->mutable_selector() =
       ToSelectorProto("fake_element_1");
@@ -365,8 +372,11 @@ TEST_F(ProtocolUtilsTest, ParseTriggerScriptsValid) {
 
   std::vector<std::unique_ptr<TriggerScript>> trigger_scripts;
   std::vector<std::string> additional_allowed_domains;
+  int interval_ms;
+  base::Optional<int> timeout_ms;
   EXPECT_TRUE(ProtocolUtils::ParseTriggerScripts(proto_str, &trigger_scripts,
-                                                 &additional_allowed_domains));
+                                                 &additional_allowed_domains,
+                                                 &interval_ms, &timeout_ms));
   EXPECT_THAT(
       trigger_scripts,
       ElementsAre(
@@ -374,6 +384,8 @@ TEST_F(ProtocolUtilsTest, ParseTriggerScriptsValid) {
           Pointee(Property(&TriggerScript::AsProto, Eq(trigger_script_2)))));
   EXPECT_THAT(additional_allowed_domains,
               ElementsAre("example.com", "other-example.com"));
+  EXPECT_EQ(interval_ms, 2000);
+  EXPECT_EQ(timeout_ms, 500000);
 }
 
 }  // namespace

components/autofill_assistant/browser/service.proto

@@ -457,6 +457,21 @@ message GetTriggerScriptsResponseProto {
   // automatically cancel the ongoing session if the user navigates away from
   // the original domain or any of the additional domains.
   repeated string additional_allowed_domains = 2;
+
+  // The amount of time a trigger script may evaluate trigger conditions while
+  // invisible. If a trigger script is invisible for this amount of time, it
+  // will automatically finish with LITE_SCRIPT_TRIGGER_CONDITION_TIMEOUT.
+  // If not specified, there is no automatic timeout.
+  //
+  // This is only counted while no trigger script is shown, and the time is
+  // reset on tab switch and whenever a trigger script is hidden. Note that this
+  // only counts the time in-between checks, so the actual timeout will be
+  // slightly longer.
+  optional int32 timeout_ms = 3;
+
+  // The amount of time between consecutive checks of trigger conditions. Should
+  // not be too small to limit performance impact.
+  optional int32 trigger_condition_check_interval_ms = 4 [default = 1000];
 }
 
 // A trigger script contains the full specification for a trigger script that is

components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator.cc

@@ -6,6 +6,7 @@
 
 #include <map>
 
+#include "base/numerics/clamped_math.h"
 #include "components/autofill_assistant/browser/client_context.h"
 #include "components/autofill_assistant/browser/protocol_utils.h"
 #include "components/autofill_assistant/browser/url_utils.h"
@@ -106,11 +107,27 @@ void TriggerScriptCoordinator::OnGetTriggerScripts(
 
   trigger_scripts_.clear();
   additional_allowed_domains_.clear();
+  base::Optional<int> timeout_ms;
+  int check_interval_ms;
   if (!ProtocolUtils::ParseTriggerScripts(response, &trigger_scripts_,
-                                          &additional_allowed_domains_)) {
+                                          &additional_allowed_domains_,
+                                          &check_interval_ms, &timeout_ms)) {
     Stop(Metrics::LiteScriptFinishedState::LITE_SCRIPT_GET_ACTIONS_PARSE_ERROR);
     return;
   }
+  trigger_condition_check_interval_ =
+      base::TimeDelta::FromMilliseconds(check_interval_ms);
+  if (timeout_ms.has_value()) {
+    // Note: add 1 for the initial, not-delayed check.
+    initial_trigger_condition_evaluations_ =
+        1 + base::ClampCeil<int64_t>(
+                base::TimeDelta::FromMilliseconds(*timeout_ms) /
+                trigger_condition_check_interval_);
+  } else {
+    initial_trigger_condition_evaluations_ = -1;
+  }
+  remaining_trigger_condition_evaluations_ =
+      initial_trigger_condition_evaluations_;
 
   Metrics::RecordLiteScriptShownToUser(
       ukm_recorder_, client_->GetWebContents(),
@@ -310,6 +327,10 @@ void TriggerScriptCoordinator::HideTriggerScript() {
     return;
   }
 
+  // Since the trigger script is now hidden, the timer to track the amount of
+  // time a script was invisible is reset.
+  remaining_trigger_condition_evaluations_ =
+      initial_trigger_condition_evaluations_;
   static_trigger_conditions_->set_is_first_time_user(false);
   visible_trigger_script_ = -1;
   for (Observer& observer : observers_) {
@@ -374,11 +395,20 @@ void TriggerScriptCoordinator::OnDynamicTriggerConditionsEvaluated() {
     }
   }
 
+  if (visible_trigger_script_ == -1 &&
+      remaining_trigger_condition_evaluations_ > 0) {
+    remaining_trigger_condition_evaluations_--;
+  }
+  if (remaining_trigger_condition_evaluations_ == 0) {
+    Stop(Metrics::LiteScriptFinishedState::
+             LITE_SCRIPT_TRIGGER_CONDITION_TIMEOUT);
+    return;
+  }
   content::GetUIThreadTaskRunner({})->PostDelayedTask(
       FROM_HERE,
       base::BindOnce(&TriggerScriptCoordinator::CheckDynamicTriggerConditions,
                      weak_ptr_factory_.GetWeakPtr()),
-      periodic_element_check_interval_);
+      trigger_condition_check_interval_);
 }
 
 void TriggerScriptCoordinator::NotifyOnTriggerScriptFinished(

components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator.h

@@ -149,9 +149,21 @@ class TriggerScriptCoordinator : public content::WebContentsObserver {
   std::unique_ptr<DynamicTriggerConditions> dynamic_trigger_conditions_;
 
   // The time between consecutive evaluations of dynamic trigger conditions.
-  // TODO(arbesser): Maybe make this configurable in proto?
-  base::TimeDelta periodic_element_check_interval_ =
-      base::TimeDelta::FromSeconds(1);
+  base::TimeDelta trigger_condition_check_interval_ =
+      base::TimeDelta::FromMilliseconds(1000);
+
+  // The number of times the trigger condition may be evaluated. If this reaches
+  // 0, the trigger script stops with |LITE_SCRIPT_TRIGGER_CONDITION_TIMEOUT|.
+  // -1 means no limit.
+  //
+  // This number is defined by the timeout (specified in proto) divided by
+  // |trigger_condition_check_interval_|. It resets on tab resume and on UI
+  // hidden. While the UI is visible, the number does not decrease.
+  int64_t remaining_trigger_condition_evaluations_ = -1;
+
+  // The initial number of evaluations. Used to store the reset value. See
+  // |remaining_trigger_condition_evaluations_|.
+  int64_t initial_trigger_condition_evaluations_ = -1;
 
   // The UKM recorder used for metrics.
   ukm::UkmRecorder* const ukm_recorder_;

components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator_unittest.cc

@@ -573,4 +573,111 @@ TEST_F(TriggerScriptCoordinatorTest, BottomSheetClosedWithSwipe) {
       Metrics::LiteScriptShownToUser::LITE_SCRIPT_SWIPE_DISMISSED, 1);
 }
 
+TEST_F(TriggerScriptCoordinatorTest, TimeoutAfterInvisibleForTooLong) {
+  GetTriggerScriptsResponseProto response;
+  *response.add_trigger_scripts()
+       ->mutable_trigger_condition()
+       ->mutable_selector() = ToSelectorProto("#selector");
+  response.set_timeout_ms(3000);
+  response.set_trigger_condition_check_interval_ms(1000);
+  std::string serialized_response;
+  response.SerializeToString(&serialized_response);
+
+  EXPECT_CALL(*mock_request_sender_, OnSendRequest(GURL(kFakeServerUrl), _, _))
+      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, serialized_response));
+  EXPECT_CALL(*mock_static_trigger_conditions_, Init)
+      .WillOnce(RunOnceCallback<3>());
+
+  // Note: expect 4 calls: 1 initial plus 3 until timeout.
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_,
+              OnUpdate(mock_web_controller_, _))
+      .Times(4)
+      .WillRepeatedly(RunOnceCallback<1>());
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_, GetSelectorMatches)
+      .Times(4)
+      .WillRepeatedly(Return(false));
+  coordinator_->Start(GURL(kFakeDeepLink),
+                      std::make_unique<TriggerContextImpl>());
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  EXPECT_CALL(mock_observer_, OnTriggerScriptFinished(
+                                  Metrics::LiteScriptFinishedState::
+                                      LITE_SCRIPT_TRIGGER_CONDITION_TIMEOUT));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  AssertRecordedFinishedState(
+      Metrics::LiteScriptFinishedState::LITE_SCRIPT_TRIGGER_CONDITION_TIMEOUT);
+}
+
+TEST_F(TriggerScriptCoordinatorTest, TimeoutResetsAfterTriggerScriptShown) {
+  GetTriggerScriptsResponseProto response;
+  *response.add_trigger_scripts()
+       ->mutable_trigger_condition()
+       ->mutable_selector() = ToSelectorProto("#selector");
+  response.set_timeout_ms(3000);
+  response.set_trigger_condition_check_interval_ms(1000);
+  std::string serialized_response;
+  response.SerializeToString(&serialized_response);
+
+  EXPECT_CALL(*mock_request_sender_, OnSendRequest(GURL(kFakeServerUrl), _, _))
+      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, serialized_response));
+  EXPECT_CALL(*mock_static_trigger_conditions_, Init)
+      .WillOnce(RunOnceCallback<3>());
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_,
+              OnUpdate(mock_web_controller_, _))
+      .WillRepeatedly(RunOnceCallback<1>());
+
+  // While the trigger script is shown, the timeout is ignored.
+  EXPECT_CALL(mock_observer_, OnTriggerScriptShown).Times(1);
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_, GetSelectorMatches)
+      .WillRepeatedly(Return(true));
+  coordinator_->Start(GURL(kFakeDeepLink),
+                      std::make_unique<TriggerContextImpl>());
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  // When the trigger script is hidden, the timeout resets.
+  EXPECT_CALL(mock_observer_, OnTriggerScriptHidden).Times(1);
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_, GetSelectorMatches)
+      .WillRepeatedly(Return(false));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  EXPECT_CALL(mock_observer_, OnTriggerScriptFinished(
+                                  Metrics::LiteScriptFinishedState::
+                                      LITE_SCRIPT_TRIGGER_CONDITION_TIMEOUT));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  AssertRecordedFinishedState(
+      Metrics::LiteScriptFinishedState::LITE_SCRIPT_TRIGGER_CONDITION_TIMEOUT);
+}
+
+TEST_F(TriggerScriptCoordinatorTest, NoTimeoutByDefault) {
+  GetTriggerScriptsResponseProto response;
+  *response.add_trigger_scripts()
+       ->mutable_trigger_condition()
+       ->mutable_selector() = ToSelectorProto("#selector");
+  std::string serialized_response;
+  response.SerializeToString(&serialized_response);
+
+  EXPECT_CALL(*mock_request_sender_, OnSendRequest(GURL(kFakeServerUrl), _, _))
+      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, serialized_response));
+  EXPECT_CALL(*mock_static_trigger_conditions_, Init)
+      .WillOnce(RunOnceCallback<3>());
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_,
+              OnUpdate(mock_web_controller_, _))
+      .WillRepeatedly(RunOnceCallback<1>());
+
+  EXPECT_CALL(mock_observer_, OnTriggerScriptFinished).Times(0);
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_, GetSelectorMatches)
+      .WillRepeatedly(Return(false));
+  coordinator_->Start(GURL(kFakeDeepLink),
+                      std::make_unique<TriggerContextImpl>());
+  for (int i = 0; i < 10; ++i) {
+    task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  }
+}
+
 }  // namespace autofill_assistant