[Autofill Assistant] Handle MANUAL_FALLBACK in Autofill Action like any other errors.

With this patch, the Autofill Action reports a MANUAL_FALLBACK or
FAILED_PRECONDITION error and let the server decide what to do about it.

Before this patch, the Autofill Action would handle these case
specially, displaying an error and then interrupting the script.

The goal of this change is to allow configuring the way the errors from
Autofill Action are handled from the server, allowing retries for
MANUAL_FALLBACK.

Nowadays, FAILED_PRECONDITION is the sign of a script error instead of
an expected manual fallback case. This is why it's now reported
separately.

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

components/autofill_assistant/browser/BUILD.gn

@@ -144,8 +144,6 @@ source_set("unit_tests") {
     "element_area_unittest.cc",
     "fake_script_executor_delegate.cc",
     "fake_script_executor_delegate.h",
-    "mock_client_memory.cc",
-    "mock_client_memory.h",
     "mock_run_once_callback.h",
     "mock_service.cc",
     "mock_service.h",

components/autofill_assistant/browser/actions/action_delegate.h

@@ -174,10 +174,6 @@ class ActionDelegate {
   // state.
   virtual void Restart() = 0;
 
-  // Stop the current script, shutdown autofill assistant and show |message| if
-  // it is not empty or a default message otherwise.
-  virtual void StopCurrentScriptAndShutdown(const std::string& message) = 0;
-
   // Return the current ClientMemory.
   virtual ClientMemory* GetClientMemory() = 0;
 

components/autofill_assistant/browser/actions/autofill_action.cc

@@ -26,8 +26,6 @@ AutofillAction::AutofillAction(const ActionProto& proto)
     prompt_ = proto.use_address().prompt();
     name_ = proto.use_address().name();
     selector_ = Selector(proto.use_address().form_field_element());
-    fill_form_message_ = proto.use_address().strings().fill_form();
-    check_form_message_ = proto.use_address().strings().check_form();
     required_fields_value_status_.resize(
         proto_.use_address().required_fields_size(), UNKNOWN);
   } else {
@@ -36,8 +34,6 @@ AutofillAction::AutofillAction(const ActionProto& proto)
     prompt_ = proto.use_card().prompt();
     name_ = "";
     selector_ = Selector(proto.use_card().form_field_element());
-    fill_form_message_ = proto.use_card().strings().fill_form();
-    check_form_message_ = proto.use_card().strings().check_form();
   }
   DCHECK(!selector_.empty());
 }
@@ -60,11 +56,7 @@ void AutofillAction::InternalProcessAction(
       (!is_autofill_card_ &&
        delegate->GetClientMemory()->selected_address(name_));
   if (!has_valid_data) {
-    // User selected 'Fill manually'.
-    // TODO(crbug.com/806868): Check whether it is still possible to reach this
-    // part of the code.
-    delegate->StopCurrentScriptAndShutdown(fill_form_message_);
-    EndAction(MANUAL_FALLBACK);
+    EndAction(PRECONDITION_FAILED);
     return;
   }
 
@@ -113,8 +105,7 @@ void AutofillAction::OnGetFullCard(ActionDelegate* delegate,
   if (!card) {
     // Gracefully shutdown the script. The empty message forces the use of the
     // default message.
-    delegate->StopCurrentScriptAndShutdown("");
-    EndAction(MANUAL_FALLBACK);
+    EndAction(GET_FULL_CARD_FAILED);
     return;
   }
 
@@ -194,9 +185,7 @@ void AutofillAction::OnCheckRequiredFieldsDone(ActionDelegate* delegate,
   }
 
   if (!allow_fallback) {
-    // Validation failed and we don't want to try the fallback, so we stop
-    // the script.
-    delegate->StopCurrentScriptAndShutdown(check_form_message_);
+    // Validation failed and we don't want to try the fallback.
     EndAction(MANUAL_FALLBACK);
     return;
   }
@@ -216,7 +205,6 @@ void AutofillAction::OnCheckRequiredFieldsDone(ActionDelegate* delegate,
     }
   }
   if (!has_fallbacks) {
-    delegate->StopCurrentScriptAndShutdown(check_form_message_);
     EndAction(MANUAL_FALLBACK);
     return;
   }
@@ -272,7 +260,6 @@ void AutofillAction::OnSetFallbackFieldValue(ActionDelegate* delegate,
                                              bool successful) {
   if (!successful) {
     // Fallback failed: we stop the script without checking the fields.
-    delegate->StopCurrentScriptAndShutdown(check_form_message_);
     EndAction(MANUAL_FALLBACK);
     return;
   }

components/autofill_assistant/browser/actions/autofill_action.h

@@ -92,8 +92,6 @@ class AutofillAction : public Action {
   std::string name_;
   std::string prompt_;
   Selector selector_;
-  std::string fill_form_message_;
-  std::string check_form_message_;
 
   // True if autofilling a card, otherwise we are autofilling an address.
   bool is_autofill_card_;

components/autofill_assistant/browser/actions/autofill_action_unittest.cc

@@ -13,7 +13,7 @@
 #include "components/autofill/core/browser/autofill_test_utils.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
 #include "components/autofill_assistant/browser/actions/mock_action_delegate.h"
-#include "components/autofill_assistant/browser/mock_client_memory.h"
+#include "components/autofill_assistant/browser/client_memory.h"
 #include "components/autofill_assistant/browser/mock_run_once_callback.h"
 #include "components/autofill_assistant/browser/mock_web_controller.h"
 #include "components/autofill_assistant/browser/service.pb.h"
@@ -99,16 +99,19 @@ class DirectCallback {
 class AutofillActionTest : public testing::Test {
  public:
   void SetUp() override {
-    autofill_profile_ = std::make_unique<autofill::AutofillProfile>(
+    auto autofill_profile = std::make_unique<autofill::AutofillProfile>(
         base::GenerateGUID(), autofill::test::kEmptyOrigin);
-    autofill::test::SetProfileInfo(autofill_profile_.get(), kFirstName, "",
+    autofill::test::SetProfileInfo(autofill_profile.get(), kFirstName, "",
                                    kLastName, kEmail, "", "", "", "", "", "",
                                    "", "");
     personal_data_manager_ = std::make_unique<MockPersonalDataManager>();
-    personal_data_manager_->SaveImportedProfile(*autofill_profile_);
+    personal_data_manager_->SaveImportedProfile(*autofill_profile);
+
+    client_memory_.set_selected_address(kAddressName,
+                                        std::move(autofill_profile));
 
     ON_CALL(mock_action_delegate_, GetClientMemory)
-        .WillByDefault(Return(&mock_client_memory_));
+        .WillByDefault(Return(&client_memory_));
     ON_CALL(mock_action_delegate_, GetPersonalDataManager)
         .WillByDefault(Return(personal_data_manager_.get()));
     ON_CALL(mock_action_delegate_, CreateBatchElementChecker)
@@ -126,16 +129,12 @@ class AutofillActionTest : public testing::Test {
   const char* const kFirstName = "FirstName";
   const char* const kLastName = "LastName";
   const char* const kEmail = "foobar@gmail.com";
-  const char* const kFillForm = "fill_form";
-  const char* const kCheckForm = "check_form";
 
   ActionProto CreateUseAddressAction() {
     ActionProto action;
     UseAddressProto* use_address = action.mutable_use_address();
     use_address->set_name(kAddressName);
     use_address->mutable_form_field_element()->add_selectors(kFakeSelector);
-    use_address->mutable_strings()->set_fill_form(kFillForm);
-    use_address->mutable_strings()->set_check_form(kCheckForm);
     return action;
   }
 
@@ -151,8 +150,6 @@ class AutofillActionTest : public testing::Test {
     ActionProto action;
     UseCreditCardProto* use_card = action.mutable_use_card();
     use_card->mutable_form_field_element()->add_selectors(kFakeSelector);
-    use_card->mutable_strings()->set_fill_form(kFillForm);
-    use_card->mutable_strings()->set_check_form(kCheckForm);
     return action;
   }
 
@@ -165,19 +162,9 @@ class AutofillActionTest : public testing::Test {
     return callback.GetResultOrDie()->status();
   }
 
-  void ExpectActionToStopScript(const ActionProto& action_proto,
-                                const std::string& expected_message) {
-    EXPECT_CALL(mock_action_delegate_,
-                StopCurrentScriptAndShutdown(expected_message));
-
-    EXPECT_EQ(ProcessedActionStatusProto::MANUAL_FALLBACK,
-              ProcessAction(action_proto));
-  }
-
   MockActionDelegate mock_action_delegate_;
   MockWebController mock_web_controller_;
-  MockClientMemory mock_client_memory_;
-  std::unique_ptr<autofill::AutofillProfile> autofill_profile_;
+  ClientMemory client_memory_;
   std::unique_ptr<autofill::PersonalDataManager> personal_data_manager_;
 };
 
@@ -192,14 +179,20 @@ TEST_F(AutofillActionTest, MAYBE_FillManually) {
   ActionProto action_proto = CreateUseAddressAction();
   action_proto.mutable_use_address()->set_prompt(kSelectionPrompt);
 
-  // No selection was made previously.
-  // Note: We use ON_CALL instead of EXPECT_CALL as the `has_selected_address`
-  // call is made inside a DCHECK, which means this won't be called when testing
-  // a release build.
-  ON_CALL(mock_client_memory_, has_selected_address(kAddressName))
-      .WillByDefault(Return(true));
+  EXPECT_EQ(ProcessedActionStatusProto::MANUAL_FALLBACK,
+            ProcessAction(action_proto));
+}
+
+TEST_F(AutofillActionTest, NoSelectedAddress) {
+  InSequence seq;
+
+  ActionProto action_proto = CreateUseAddressAction();
+  action_proto.mutable_use_address()->set_prompt(kSelectionPrompt);
+
+  client_memory_.set_selected_address(kAddressName, nullptr);
 
-  ExpectActionToStopScript(action_proto, kFillForm);
+  EXPECT_EQ(ProcessedActionStatusProto::PRECONDITION_FAILED,
+            ProcessAction(action_proto));
 }
 
 TEST_F(AutofillActionTest, ValidationSucceeds) {
@@ -213,12 +206,6 @@ TEST_F(AutofillActionTest, ValidationSucceeds) {
   AddRequiredField(&action_proto, UseAddressProto::RequiredField::EMAIL,
                    "#email");
 
-  // Return a fake selected address.
-  ON_CALL(mock_client_memory_, has_selected_address(kAddressName))
-      .WillByDefault(Return(true));
-  ON_CALL(mock_client_memory_, selected_address(kAddressName))
-      .WillByDefault(Return(autofill_profile_.get()));
-
   // Autofill succeeds.
   EXPECT_CALL(mock_action_delegate_,
               OnFillAddressForm(NotNull(), Eq(Selector({kFakeSelector})), _))
@@ -243,12 +230,6 @@ TEST_F(AutofillActionTest, FallbackFails) {
   AddRequiredField(&action_proto, UseAddressProto::RequiredField::EMAIL,
                    "#email");
 
-  // Return a fake selected address.
-  ON_CALL(mock_client_memory_, has_selected_address(kAddressName))
-      .WillByDefault(Return(true));
-  ON_CALL(mock_client_memory_, selected_address(kAddressName))
-      .WillByDefault(Return(autofill_profile_.get()));
-
   // Autofill succeeds.
   EXPECT_CALL(mock_action_delegate_,
               OnFillAddressForm(NotNull(), Eq(Selector({kFakeSelector})), _))
@@ -270,7 +251,8 @@ TEST_F(AutofillActionTest, FallbackFails) {
               OnSetFieldValue(Eq(Selector({"#first_name"})), kFirstName, _))
       .WillOnce(RunOnceCallback<2>(false));
 
-  ExpectActionToStopScript(action_proto, kCheckForm);
+  EXPECT_EQ(ProcessedActionStatusProto::MANUAL_FALLBACK,
+            ProcessAction(action_proto));
 }
 
 TEST_F(AutofillActionTest, FallbackSucceeds) {
@@ -284,12 +266,6 @@ TEST_F(AutofillActionTest, FallbackSucceeds) {
   AddRequiredField(&action_proto, UseAddressProto::RequiredField::EMAIL,
                    "#email");
 
-  // Return a fake selected address.
-  ON_CALL(mock_client_memory_, has_selected_address(kAddressName))
-      .WillByDefault(Return(true));
-  ON_CALL(mock_client_memory_, selected_address(kAddressName))
-      .WillByDefault(Return(autofill_profile_.get()));
-
   // Autofill succeeds.
   EXPECT_CALL(mock_action_delegate_,
               OnFillAddressForm(NotNull(), Eq(Selector({kFakeSelector})), _))

components/autofill_assistant/browser/actions/mock_action_delegate.h

@@ -131,7 +131,6 @@ class MockActionDelegate : public ActionDelegate {
   MOCK_METHOD0(GetClientMemory, ClientMemory*());
   MOCK_METHOD0(GetPersonalDataManager, autofill::PersonalDataManager*());
   MOCK_METHOD0(GetWebContents, content::WebContents*());
-  MOCK_METHOD1(StopCurrentScriptAndShutdown, void(const std::string& message));
   MOCK_METHOD1(SetDetails, void(const Details& details));
   MOCK_METHOD0(ClearDetails, void());
   MOCK_METHOD1(SetProgress, void(int progress));

components/autofill_assistant/browser/mock_client_memory.cc

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "components/autofill_assistant/browser/mock_client_memory.h"
-
-namespace autofill_assistant {
-
-MockClientMemory::MockClientMemory() = default;
-MockClientMemory::~MockClientMemory() = default;
-
-}  // namespace autofill_assistant
\ No newline at end of file

components/autofill_assistant/browser/mock_client_memory.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_MOCK_CLIENT_MEMORY_H_
-#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_MOCK_CLIENT_MEMORY_H_
-
-#include "components/autofill_assistant/browser/client_memory.h"
-#include "testing/gmock/include/gmock/gmock.h"
-
-namespace autofill_assistant {
-
-class MockClientMemory : public ClientMemory {
- public:
-  MockClientMemory();
-  ~MockClientMemory();
-
-  MOCK_METHOD0(selected_card, const autofill::CreditCard*());
-  MOCK_METHOD0(has_selected_card, bool());
-  MOCK_METHOD1(selected_address,
-               const autofill::AutofillProfile*(const std::string& name));
-  MOCK_METHOD1(has_selected_address, bool(const std::string& name));
-  MOCK_METHOD1(set_selected_card,
-               void(std::unique_ptr<autofill::CreditCard> card));
-  MOCK_METHOD2(set_selected_address,
-               void(const std::string& name,
-                    std::unique_ptr<autofill::AutofillProfile> address));
-};
-
-}  // namespace autofill_assistant
-
-#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_MOCK_CLIENT_MEMORY_H_

components/autofill_assistant/browser/script_executor.cc

@@ -70,6 +70,15 @@ std::ostream& operator<<(std::ostream& out,
     case ProcessedActionStatusProto::USER_ABORTED_ACTION:
       out << "USER_ABORTED_ACTION";
       break;
+
+    case ProcessedActionStatusProto::GET_FULL_CARD_FAILED:
+      out << "GET_FULL_CARD_FAILED";
+      break;
+
+    case ProcessedActionStatusProto::PRECONDITION_FAILED:
+      out << "PRECONDITION_FAILED";
+      break;
+
       // Intentionally no default case to make compilation fail if a new value
       // was added to the enum but not to this list.
   }
@@ -388,15 +397,6 @@ void ScriptExecutor::Restart() {
   at_end_ = RESTART;
 }
 
-void ScriptExecutor::StopCurrentScriptAndShutdown(const std::string& message) {
-  // Use a default message when |message| is empty.
-  delegate_->SetStatusMessage(
-      message.empty() ? l10n_util::GetStringUTF8(IDS_AUTOFILL_ASSISTANT_GIVE_UP)
-                      : message);
-  at_end_ = SHUTDOWN_GRACEFULLY;
-  should_stop_script_ = true;
-}
-
 ClientMemory* ScriptExecutor::GetClientMemory() {
   return delegate_->GetClientMemory();
 }

components/autofill_assistant/browser/script_executor.h

@@ -155,7 +155,6 @@ class ScriptExecutor : public ActionDelegate {
   ClientMemory* GetClientMemory() override;
   autofill::PersonalDataManager* GetPersonalDataManager() override;
   content::WebContents* GetWebContents() override;
-  void StopCurrentScriptAndShutdown(const std::string& message) override;
   void ClearDetails() override;
   void SetDetails(const Details& details) override;
   void SetProgress(int progress) override;

components/autofill_assistant/browser/service.proto

@@ -333,6 +333,23 @@ enum ProcessedActionStatusProto {
   // affect any action. It is a signal that the action or script ran in an
   // abnormal situation and its outcome cannot be trusted.
   USER_ABORTED_ACTION = 8;
+
+  // The Autofill Action failed to get the full card information.
+  //
+  // Possible causes:
+  // - the user filled in the wrong CVC number.
+  // - the card has expired
+  GET_FULL_CARD_FAILED = 9;
+
+  // The action did not have what it needs. This is generally a bug in the
+  // script.
+  //
+  // This is currently returned by the autofill action, when it could not find
+  // the credit card or the address it needs in the client memory. This is
+  // usually the sign that the Get Payment Information action was not run or
+  // failed.
+  //
+  PRECONDITION_FAILED = 10;
 }
 
 // The pseudo type values come from
@@ -423,14 +440,6 @@ message ElementAreaProto {
   repeated Rectangle rectangles = 1;
 }
 
-message AutofillStrings {
-  optional string fill_manually = 1;
-
-  optional string fill_form = 2;
-
-  optional string check_form = 3;
-}
-
 // Fill a form with an address if there is, otherwise fail this action.
 message UseAddressProto {
   // Message used to indicate what form fields should be filled with what
@@ -474,8 +483,6 @@ message UseAddressProto {
 
   // An optional list of fields that should be filled by this action.
   repeated RequiredField required_fields = 6;
-
-  optional AutofillStrings strings = 8;
 }
 
 // Fill a form with a credit card if there is, otherwise fail this action.
@@ -486,8 +493,6 @@ message UseCreditCardProto {
 
   // A reference to the card number field in the form that should be filled.
   optional ElementReferenceProto form_field_element = 3;
-
-  optional AutofillStrings strings = 8;
 }
 
 // Ask Chrome to wait for an element in the DOM. This can be used to only