Commit 2b5ee7a0 authored by sandromaggi's avatar sandromaggi Committed by Commit Bot

[Autofill Assistant] Attempt fallback if autofill fails

Previously, when autofill actively failed - as opposed to
just being unable to fill the fields -, the action was
completely terminated. New the required fields fallback will
be triggered.

Bug: b/143266929
Change-Id: I40ce9ae9d73f1d592d74a6aae3cdac9c25542434
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879893
Commit-Queue: Sandro Maggi <sandromaggi@google.com>
Reviewed-by: default avatarMathias Carlen <mcarlen@chromium.org>
Reviewed-by: default avatarClemens Arbesser <arbesser@google.com>
Cr-Commit-Position: refs/heads/master@{#710274}
parent 04769322
......@@ -9,6 +9,7 @@
#include "base/bind.h"
#include "base/callback.h"
#include "base/optional.h"
#include "components/autofill_assistant/browser/actions/action_delegate.h"
#include "components/autofill_assistant/browser/batch_element_checker.h"
#include "components/autofill_assistant/browser/client_status.h"
......@@ -20,7 +21,9 @@ RequiredFieldsFallbackHandler::RequiredFieldsFallbackHandler(
base::RepeatingCallback<std::string(const RequiredField&,
const FallbackData&)>
field_value_getter,
base::OnceCallback<void(const ClientStatus&)> status_update_callback,
base::OnceCallback<void(const ClientStatus&,
const base::Optional<ClientStatus>&)>
status_update_callback,
ActionDelegate* action_delegate) {
required_fields_.assign(required_fields.begin(), required_fields.end());
field_value_getter_ = std::move(field_value_getter);
......@@ -33,12 +36,26 @@ RequiredFieldsFallbackHandler::~RequiredFieldsFallbackHandler() {}
RequiredFieldsFallbackHandler::FallbackData::FallbackData() {}
void RequiredFieldsFallbackHandler::CheckAndFallbackRequiredFields(
const ClientStatus& initial_autofill_status,
std::unique_ptr<FallbackData> fallback_data) {
initial_autofill_status_ = initial_autofill_status;
if (required_fields_.empty()) {
std::move(status_update_callback_).Run(ClientStatus(ACTION_APPLIED));
if (!initial_autofill_status.ok()) {
DVLOG(1) << __func__ << " Autofill failed and no fallback provided "
<< initial_autofill_status.proto_status();
}
std::move(status_update_callback_)
.Run(initial_autofill_status, base::nullopt);
return;
}
CheckAllRequiredFields(std::move(fallback_data));
}
void RequiredFieldsFallbackHandler::CheckAllRequiredFields(
std::unique_ptr<FallbackData> fallback_data) {
DCHECK(!batch_element_checker_);
batch_element_checker_ = std::make_unique<BatchElementChecker>();
for (size_t i = 0; i < required_fields_.size(); i++) {
......@@ -81,13 +98,15 @@ void RequiredFieldsFallbackHandler::OnCheckRequiredFieldsDone(
}
if (!should_fallback) {
std::move(status_update_callback_).Run(ClientStatus(ACTION_APPLIED));
std::move(status_update_callback_)
.Run(ClientStatus(ACTION_APPLIED), initial_autofill_status_);
return;
}
if (!fallback_data) {
// Validation failed and we don't want to try the fallback.
std::move(status_update_callback_).Run(ClientStatus(MANUAL_FALLBACK));
std::move(status_update_callback_)
.Run(ClientStatus(MANUAL_FALLBACK), initial_autofill_status_);
return;
}
......@@ -103,7 +122,8 @@ void RequiredFieldsFallbackHandler::OnCheckRequiredFieldsDone(
}
}
if (!has_fallbacks) {
std::move(status_update_callback_).Run(ClientStatus(MANUAL_FALLBACK));
std::move(status_update_callback_)
.Run(ClientStatus(MANUAL_FALLBACK), initial_autofill_status_);
return;
}
......@@ -126,7 +146,7 @@ void RequiredFieldsFallbackHandler::SetFallbackFieldValuesSequentially(
if (required_fields_index >= required_fields_.size()) {
DCHECK_EQ(required_fields_index, required_fields_.size());
return CheckAndFallbackRequiredFields(/* fallback_data= */ nullptr);
return CheckAllRequiredFields(/* fallback_data= */ nullptr);
}
// Set the next field to its fallback value.
......@@ -155,7 +175,8 @@ void RequiredFieldsFallbackHandler::OnSetFallbackFieldValue(
const ClientStatus& setFieldStatus) {
if (!setFieldStatus.ok()) {
// Fallback failed: we stop the script without checking the fields.
std::move(status_update_callback_).Run(ClientStatus(MANUAL_FALLBACK));
std::move(status_update_callback_)
.Run(ClientStatus(MANUAL_FALLBACK), initial_autofill_status_);
return;
}
......
......@@ -13,6 +13,7 @@
#include "base/callback.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "components/autofill/core/browser/data_model/autofill_profile.h"
#include "components/autofill_assistant/browser/actions/action.h"
#include "components/autofill_assistant/browser/batch_element_checker.h"
......@@ -76,18 +77,26 @@ class RequiredFieldsFallbackHandler {
base::RepeatingCallback<std::string(const RequiredField&,
const FallbackData&)>
field_value_getter,
base::OnceCallback<void(const ClientStatus&)> status_update_callback,
base::OnceCallback<void(const ClientStatus&,
const base::Optional<ClientStatus>&)>
status_update_callback,
ActionDelegate* delegate);
~RequiredFieldsFallbackHandler();
// Check if there are required fields. If so, verify them and fallback if
// they are empty. If not, update the status to the result of the autofill
// action.
void CheckAndFallbackRequiredFields(
const ClientStatus& initial_autofill_status,
std::unique_ptr<FallbackData> fallback_data);
private:
// Check whether all required fields have a non-empty value. If it is the
// case, update the status to success. If it's not and |fallback_data|
// is null, update the status to failure. If |fallback_data| is non-null, use
// it to attempt to fill the failed fields without Autofill.
void CheckAndFallbackRequiredFields(
std::unique_ptr<FallbackData> fallback_data);
void CheckAllRequiredFields(std::unique_ptr<FallbackData> fallback_data);
private:
// Triggers the check for a specific field.
void CheckRequiredFieldsSequentially(
bool allow_fallback,
......@@ -113,11 +122,15 @@ class RequiredFieldsFallbackHandler {
std::unique_ptr<FallbackData> fallback_data,
const ClientStatus& status);
ClientStatus initial_autofill_status_;
std::vector<RequiredField> required_fields_;
base::RepeatingCallback<std::string(const RequiredField&,
const FallbackData&)>
field_value_getter_;
base::OnceCallback<void(const ClientStatus&)> status_update_callback_;
base::OnceCallback<void(const ClientStatus&,
const base::Optional<ClientStatus>&)>
status_update_callback_;
ActionDelegate* action_delegate_;
std::unique_ptr<BatchElementChecker> batch_element_checker_;
base::WeakPtrFactory<RequiredFieldsFallbackHandler> weak_ptr_factory_{this};
......
......@@ -9,6 +9,7 @@
#include "base/bind.h"
#include "base/callback.h"
#include "base/optional.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
......@@ -79,8 +80,14 @@ void UseAddressAction::InternalProcessAction(
FillFormWithData();
}
void UseAddressAction::EndAction(const ClientStatus& status) {
UpdateProcessedAction(status);
void UseAddressAction::EndAction(
const ClientStatus& final_status,
const base::Optional<ClientStatus>& optional_details_status) {
UpdateProcessedAction(final_status);
if (optional_details_status.has_value() && !optional_details_status->ok()) {
processed_action_proto_->mutable_status_details()->MergeFrom(
optional_details_status->details());
}
std::move(process_action_callback_).Run(std::move(processed_action_proto_));
}
......@@ -111,14 +118,8 @@ void UseAddressAction::OnWaitForElement(const ClientStatus& element_status) {
void UseAddressAction::OnFormFilled(std::unique_ptr<FallbackData> fallback_data,
const ClientStatus& status) {
// In case Autofill failed, we fail the action.
if (!status.ok()) {
EndAction(status);
return;
}
required_fields_fallback_handler_->CheckAndFallbackRequiredFields(
std::move(fallback_data));
status, std::move(fallback_data));
}
std::string UseAddressAction::GetFallbackValue(
......
......@@ -12,6 +12,7 @@
#include "base/callback.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "components/autofill_assistant/browser/actions/action.h"
#include "components/autofill_assistant/browser/actions/required_fields_fallback_handler.h"
......@@ -29,11 +30,12 @@ class UseAddressAction : public Action {
~UseAddressAction() override;
private:
// Overrides Action:
void InternalProcessAction(ProcessActionCallback callback) override;
void EndAction(const ClientStatus& status);
void EndAction(const ClientStatus& final_status,
const base::Optional<ClientStatus>& optional_details_status =
base::nullopt);
// Fill the form using data in client memory. Return whether filling succeeded
// or not through OnFormFilled.
......
......@@ -16,6 +16,7 @@
#include "components/autofill_assistant/browser/actions/mock_action_delegate.h"
#include "components/autofill_assistant/browser/mock_personal_data_manager.h"
#include "components/autofill_assistant/browser/web/mock_web_controller.h"
#include "components/autofill_assistant/browser/web/web_controller_util.h"
#include "testing/gmock/include/gmock/gmock.h"
namespace autofill_assistant {
......@@ -280,5 +281,65 @@ TEST_F(UseAddressActionTest, FallbackSucceeds) {
ProcessAction(action_proto));
}
TEST_F(UseAddressActionTest, AutofillFailureWithoutRequiredFieldsIsFatal) {
ActionProto action_proto = CreateUseAddressAction();
EXPECT_CALL(mock_action_delegate_,
OnFillAddressForm(
NotNull(), Eq(Selector({kFakeSelector}).MustBeVisible()), _))
.WillOnce(RunOnceCallback<2>(ClientStatus(OTHER_ACTION_STATUS)));
ProcessedActionProto processed_action;
EXPECT_CALL(callback_, Run(_)).WillOnce(SaveArgPointee<0>(&processed_action));
UseAddressAction action(&mock_action_delegate_, action_proto);
action.ProcessAction(callback_.Get());
EXPECT_EQ(processed_action.status(),
ProcessedActionStatusProto::OTHER_ACTION_STATUS);
EXPECT_EQ(processed_action.has_status_details(), false);
}
TEST_F(UseAddressActionTest,
AutofillFailureWithRequiredFieldsLaunchesFallback) {
ActionProto action_proto = CreateUseAddressAction();
AddRequiredField(&action_proto, UseAddressProto::RequiredField::FIRST_NAME,
"#first_name");
EXPECT_CALL(mock_action_delegate_,
OnFillAddressForm(
NotNull(), Eq(Selector({kFakeSelector}).MustBeVisible()), _))
.WillOnce(RunOnceCallback<2>(
FillAutofillErrorStatus(ClientStatus(OTHER_ACTION_STATUS))));
// First validation fails.
EXPECT_CALL(mock_web_controller_,
OnGetFieldValue(Selector({"#first_name"}), _))
.WillOnce(RunOnceCallback<1>(OkClientStatus(), ""));
// Fill first name.
Expectation set_first_name =
EXPECT_CALL(mock_action_delegate_,
OnSetFieldValue(Selector({"#first_name"}), kFirstName, _))
.WillOnce(RunOnceCallback<2>(OkClientStatus()));
// Second validation succeeds.
EXPECT_CALL(mock_web_controller_,
OnGetFieldValue(Selector({"#first_name"}), _))
.After(set_first_name)
.WillOnce(RunOnceCallback<1>(OkClientStatus(), "not empty"));
ProcessedActionProto processed_action;
EXPECT_CALL(callback_, Run(_)).WillOnce(SaveArgPointee<0>(&processed_action));
UseAddressAction action(&mock_action_delegate_, action_proto);
action.ProcessAction(callback_.Get());
EXPECT_EQ(processed_action.status(),
ProcessedActionStatusProto::ACTION_APPLIED);
EXPECT_EQ(processed_action.status_details()
.autofill_error_info()
.autofill_error_status(),
OTHER_ACTION_STATUS);
}
} // namespace
} // namespace autofill_assistant
......@@ -9,6 +9,7 @@
#include "base/bind.h"
#include "base/callback.h"
#include "base/optional.h"
#include "base/strings/strcat.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/stringprintf.h"
......@@ -72,8 +73,14 @@ void UseCreditCardAction::InternalProcessAction(
FillFormWithData();
}
void UseCreditCardAction::EndAction(const ClientStatus& status) {
UpdateProcessedAction(status);
void UseCreditCardAction::EndAction(
const ClientStatus& final_status,
const base::Optional<ClientStatus>& optional_details_status) {
UpdateProcessedAction(final_status);
if (optional_details_status.has_value() && !optional_details_status->ok()) {
processed_action_proto_->mutable_status_details()->MergeFrom(
optional_details_status->details());
}
std::move(process_action_callback_).Run(std::move(processed_action_proto_));
}
......@@ -120,14 +127,8 @@ void UseCreditCardAction::OnGetFullCard(
void UseCreditCardAction::OnFormFilled(
std::unique_ptr<FallbackData> fallback_data,
const ClientStatus& status) {
// In case Autofill failed, we fail the action.
if (!status.ok()) {
EndAction(status);
return;
}
required_fields_fallback_handler_->CheckAndFallbackRequiredFields(
std::move(fallback_data));
status, std::move(fallback_data));
}
std::string UseCreditCardAction::GetFallbackValue(
......
......@@ -12,6 +12,7 @@
#include "base/callback.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "components/autofill_assistant/browser/actions/action.h"
#include "components/autofill_assistant/browser/actions/required_fields_fallback_handler.h"
......@@ -30,11 +31,12 @@ class UseCreditCardAction : public Action {
~UseCreditCardAction() override;
private:
// Overrides Action:
void InternalProcessAction(ProcessActionCallback callback) override;
void EndAction(const ClientStatus& status);
void EndAction(const ClientStatus& final_status,
const base::Optional<ClientStatus>& optional_details_status =
base::nullopt);
// Fill the form using data in client memory. Return whether filling succeeded
// or not through OnFormFilled.
......
......@@ -15,6 +15,7 @@
#include "components/autofill_assistant/browser/actions/mock_action_delegate.h"
#include "components/autofill_assistant/browser/mock_personal_data_manager.h"
#include "components/autofill_assistant/browser/web/mock_web_controller.h"
#include "components/autofill_assistant/browser/web/web_controller_util.h"
#include "testing/gmock/include/gmock/gmock.h"
namespace autofill_assistant {
......@@ -308,5 +309,74 @@ TEST_F(UseCreditCardActionTest, ForcedFallback) {
EXPECT_EQ(ProcessedActionStatusProto::ACTION_APPLIED, ProcessAction(action));
}
TEST_F(UseCreditCardActionTest, AutofillFailureWithoutRequiredFieldsIsFatal) {
ActionProto action_proto = CreateUseCreditCardAction();
autofill::CreditCard credit_card;
client_memory_.set_selected_card(
std::make_unique<autofill::CreditCard>(credit_card));
EXPECT_CALL(mock_action_delegate_, OnGetFullCard(_))
.WillOnce(RunOnceCallback<0>(credit_card, base::UTF8ToUTF16("123")));
EXPECT_CALL(mock_action_delegate_,
OnFillCardForm(_, base::UTF8ToUTF16("123"),
Selector({kFakeSelector}).MustBeVisible(), _))
.WillOnce(RunOnceCallback<3>(ClientStatus(OTHER_ACTION_STATUS)));
ProcessedActionProto processed_action;
EXPECT_CALL(callback_, Run(_)).WillOnce(SaveArgPointee<0>(&processed_action));
UseCreditCardAction action(&mock_action_delegate_, action_proto);
action.ProcessAction(callback_.Get());
EXPECT_EQ(processed_action.status(),
ProcessedActionStatusProto::OTHER_ACTION_STATUS);
EXPECT_EQ(processed_action.has_status_details(), false);
}
TEST_F(UseCreditCardActionTest,
AutofillFailureWithRequiredFieldsLaunchesFallback) {
ActionProto action_proto = CreateUseCreditCardAction();
AddRequiredField(
&action_proto,
UseCreditCardProto::RequiredField::CREDIT_CARD_VERIFICATION_CODE, "#cvc");
autofill::CreditCard credit_card;
client_memory_.set_selected_card(
std::make_unique<autofill::CreditCard>(credit_card));
EXPECT_CALL(mock_action_delegate_, OnGetFullCard(_))
.WillOnce(RunOnceCallback<0>(credit_card, base::UTF8ToUTF16("123")));
EXPECT_CALL(mock_action_delegate_,
OnFillCardForm(_, base::UTF8ToUTF16("123"),
Selector({kFakeSelector}).MustBeVisible(), _))
.WillOnce(RunOnceCallback<3>(
FillAutofillErrorStatus(ClientStatus(OTHER_ACTION_STATUS))));
// First validation fails.
EXPECT_CALL(mock_web_controller_, OnGetFieldValue(Selector({"#cvc"}), _))
.WillOnce(RunOnceCallback<1>(OkClientStatus(), ""));
// Fill CVC.
Expectation set_cvc =
EXPECT_CALL(mock_action_delegate_,
OnSetFieldValue(Selector({"#cvc"}), "123", _))
.WillOnce(RunOnceCallback<2>(OkClientStatus()));
// Second validation succeeds.
EXPECT_CALL(mock_web_controller_, OnGetFieldValue(Selector({"#cvc"}), _))
.After(set_cvc)
.WillOnce(RunOnceCallback<1>(OkClientStatus(), "not empty"));
ProcessedActionProto processed_action;
EXPECT_CALL(callback_, Run(_)).WillOnce(SaveArgPointee<0>(&processed_action));
UseCreditCardAction action(&mock_action_delegate_, action_proto);
action.ProcessAction(callback_.Get());
EXPECT_EQ(processed_action.status(),
ProcessedActionStatusProto::ACTION_APPLIED);
EXPECT_EQ(processed_action.status_details()
.autofill_error_info()
.autofill_error_status(),
OTHER_ACTION_STATUS);
}
} // namespace
} // namespace autofill_assistant
......@@ -516,8 +516,6 @@ message ProcessedActionStatusDetailsProto {
optional ProcessedActionStatusProto original_status = 2;
// More information included for autofill related errors.
//
// For now this is only filled for PRECONDITION_FAILED errors.
optional AutofillErrorInfoProto autofill_error_info = 3;
}
......@@ -589,6 +587,9 @@ message AutofillErrorInfoProto {
// Whether the client memory at |address_key_requested| pointed to null.
optional bool address_pointee_was_null = 3;
// Error status of the Chrome autofill attempt.
optional ProcessedActionStatusProto autofill_error_status = 4;
}
// Next: 22
......
......@@ -738,6 +738,23 @@ void WebController::FillAddressForm(
std::move(callback)));
}
void WebController::FillCardForm(
std::unique_ptr<autofill::CreditCard> card,
const base::string16& cvc,
const Selector& selector,
base::OnceCallback<void(const ClientStatus&)> callback) {
DVLOG(3) << __func__ << " " << selector;
auto data_to_autofill = std::make_unique<FillFormInputData>();
data_to_autofill->card = std::move(card);
data_to_autofill->cvc = cvc;
FindElement(selector,
/* strict_mode= */ true,
base::BindOnce(&WebController::OnFindElementForFillingForm,
weak_ptr_factory_.GetWeakPtr(),
std::move(data_to_autofill), selector,
std::move(callback)));
}
void WebController::OnFindElementForFillingForm(
std::unique_ptr<FillFormInputData> data_to_autofill,
const Selector& selector,
......@@ -746,12 +763,18 @@ void WebController::OnFindElementForFillingForm(
std::unique_ptr<ElementFinder::Result> element_result) {
if (!status.ok()) {
DVLOG(1) << __func__ << " Failed to find the element for filling the form.";
std::move(callback).Run(status);
std::move(callback).Run(FillAutofillErrorStatus(status));
return;
}
ContentAutofillDriver* driver = ContentAutofillDriver::GetForRenderFrameHost(
element_result->container_frame_host);
if (driver == nullptr) {
DVLOG(1) << __func__ << " Failed to get the autofill driver.";
std::move(callback).Run(
FillAutofillErrorStatus(UnexpectedErrorStatus(__FILE__, __LINE__)));
return;
}
DCHECK(!selector.empty());
// TODO(crbug.com/806868): Figure out whether there are cases where we need
// more than one selector, and come up with a solution that can figure out the
......@@ -772,15 +795,17 @@ void WebController::OnGetFormAndFieldDataForFillingForm(
const autofill::FormFieldData& form_field) {
if (form_data.fields.empty()) {
DVLOG(1) << __func__ << " Failed to get form data to fill form.";
std::move(callback).Run(UnexpectedErrorStatus(__FILE__, __LINE__));
std::move(callback).Run(
FillAutofillErrorStatus(UnexpectedErrorStatus(__FILE__, __LINE__)));
return;
}
ContentAutofillDriver* driver =
ContentAutofillDriver::GetForRenderFrameHost(container_frame_host);
if (!driver) {
if (driver == nullptr) {
DVLOG(1) << __func__ << " Failed to get the autofill driver.";
std::move(callback).Run(UnexpectedErrorStatus(__FILE__, __LINE__));
std::move(callback).Run(
FillAutofillErrorStatus(UnexpectedErrorStatus(__FILE__, __LINE__)));
return;
}
......@@ -796,23 +821,6 @@ void WebController::OnGetFormAndFieldDataForFillingForm(
std::move(callback).Run(OkClientStatus());
}
void WebController::FillCardForm(
std::unique_ptr<autofill::CreditCard> card,
const base::string16& cvc,
const Selector& selector,
base::OnceCallback<void(const ClientStatus&)> callback) {
DVLOG(3) << __func__ << " " << selector;
auto data_to_autofill = std::make_unique<FillFormInputData>();
data_to_autofill->card = std::move(card);
data_to_autofill->cvc = cvc;
FindElement(selector,
/* strict_mode= */ true,
base::BindOnce(&WebController::OnFindElementForFillingForm,
weak_ptr_factory_.GetWeakPtr(),
std::move(data_to_autofill), selector,
std::move(callback)));
}
void WebController::SelectOption(
const Selector& selector,
const std::string& selected_option,
......
......@@ -47,6 +47,13 @@ ClientStatus JavaScriptErrorStatus(
return status;
}
ClientStatus FillAutofillErrorStatus(ClientStatus status) {
status.mutable_details()
->mutable_autofill_error_info()
->set_autofill_error_status(status.proto_status());
return status;
}
bool SafeGetObjectId(const runtime::RemoteObject* result, std::string* out) {
if (result && result->HasObjectId()) {
*out = result->GetObjectId();
......
......@@ -53,6 +53,9 @@ ClientStatus CheckJavaScriptResult(
return OkClientStatus();
}
// Fills a ClientStatus with appropriate details for a Chrome Autofill error.
ClientStatus FillAutofillErrorStatus(ClientStatus status);
// Safely gets an object id from a RemoteObject
bool SafeGetObjectId(const runtime::RemoteObject* result, std::string* out);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment