diff --git a/components/autofill/core/browser/payments/multiple_request_payments_network_interface_base.cc b/components/autofill/core/browser/payments/multiple_request_payments_network_interface_base.cc index 2340f1446aec4..633010d5e0d58 100644 --- a/components/autofill/core/browser/payments/multiple_request_payments_network_interface_base.cc +++ b/components/autofill/core/browser/payments/multiple_request_payments_network_interface_base.cc @@ -357,7 +357,13 @@ void MultipleRequestPaymentsNetworkInterfaceBase::RequestOperation:: void MultipleRequestPaymentsNetworkInterfaceBase::RequestOperation:: ReportOperationResult(PaymentsRpcResult result) { CHECK(request_); + // RespondToDelegate may delete `payments_network_interface_` (and with it + // `this`) if its owner is torn down from within the response callback. + auto weak_this = weak_ptr_factory_.GetWeakPtr(); request_->RespondToDelegate(result); + if (!weak_this) { + return; + } payments_network_interface_->OnRequestFinished(request_operation_id_); } diff --git a/components/autofill/core/browser/payments/multiple_request_payments_network_interface_base_unittest.cc b/components/autofill/core/browser/payments/multiple_request_payments_network_interface_base_unittest.cc index 3e01d9ed2814d..4e2f46e6badde 100644 --- a/components/autofill/core/browser/payments/multiple_request_payments_network_interface_base_unittest.cc +++ b/components/autofill/core/browser/payments/multiple_request_payments_network_interface_base_unittest.cc @@ -265,4 +265,20 @@ TEST_F(MultipleRequestsTest, RequestCanceled) { EXPECT_EQ(0U, operations_size()); } +// A request's RespondToDelegate may synchronously destroy the network +// interface (and thus the in-flight RequestOperation) if its owner outlives +// neither. +TEST_F(MultipleRequestsTest, RespondToDelegateDestroysNetworkInterface) { + EXPECT_CALL(*request1_, RespondToDelegate(PaymentsRpcResult::kSuccess)) + .WillOnce([&](PaymentsRpcResult) { + payments_network_interface_base_.reset(); + }); + + RequestId id1 = + payments_network_interface_base_->IssueRequest(std::move(request1_)); + IssueOAuthToken(); + ReturnResponseForOperation(id1, net::HTTP_OK, "{}"); + EXPECT_FALSE(payments_network_interface_base_); +} + } // namespace autofill::payments diff --git a/components/autofill/core/browser/payments/virtual_card_enrollment_manager.cc b/components/autofill/core/browser/payments/virtual_card_enrollment_manager.cc index 55a6bf0ca87af..2a7eb27147e42 100644 --- a/components/autofill/core/browser/payments/virtual_card_enrollment_manager.cc +++ b/components/autofill/core/browser/payments/virtual_card_enrollment_manager.cc @@ -347,17 +347,20 @@ void VirtualCardEnrollmentManager::OnDidGetUpdateVirtualCardEnrollmentResponse( state_.virtual_card_enrollment_fields.credit_card.instrument_id())); } - // Relay the response to the server card editor page. This also destroys the - // payments delegate if the editor was already closed. - if (virtual_card_enrollment_update_response_callback_.has_value()) { - std::move(virtual_card_enrollment_update_response_callback_.value()) - .Run(result); - } - LogUpdateVirtualCardEnrollmentRequestResult( state_.virtual_card_enrollment_fields.virtual_card_enrollment_source, type, result == PaymentsRpcResult::kSuccess); + + std::optional response_callback = + std::move(virtual_card_enrollment_update_response_callback_); Reset(); + + // Relay the response to the server card editor page. This may destroy the + // payments delegate (and with it `this`) if the editor was already closed, + // so it must run last. + if (response_callback.has_value()) { + std::move(response_callback.value()).Run(result); + } } void VirtualCardEnrollmentManager::OnVirtualCardEnrollCompleted( diff --git a/components/autofill/core/browser/payments/virtual_card_enrollment_manager_unittest.cc b/components/autofill/core/browser/payments/virtual_card_enrollment_manager_unittest.cc index af5c7e242a309..9b4bea2e174b8 100644 --- a/components/autofill/core/browser/payments/virtual_card_enrollment_manager_unittest.cc +++ b/components/autofill/core/browser/payments/virtual_card_enrollment_manager_unittest.cc @@ -8,6 +8,7 @@ #include "base/functional/callback.h" #include "base/strings/strcat.h" #include "base/strings/string_number_conversions.h" +#include "base/test/bind.h" #include "base/test/metrics/histogram_tester.h" #include "base/test/mock_callback.h" #include "base/test/scoped_feature_list.h" @@ -404,6 +405,50 @@ TEST_F(VirtualCardEnrollmentManagerTest, Unenroll) { /*sample=*/false, 1); } +// On the Clank settings page the update-response callback can synchronously +// destroy the owning delegate (and with it, this manager) when the editor was +// closed while a request was in flight. The manager must finish all of its own +// work before invoking that callback. +TEST_F(VirtualCardEnrollmentManagerTest, + Unenroll_ResponseCallbackDeletesManager) { + base::HistogramTester histogram_tester; + payments_data_manager().SetPaymentsCustomerData( + std::make_unique(/*customer_id=*/"123456")); + + base::OnceCallback + network_response_callback; + EXPECT_CALL(multiple_request_payments_network_interface(), + UpdateVirtualCardEnrollment) + .WillOnce( + [&](const payments::UpdateVirtualCardEnrollmentRequestDetails&, + base::OnceCallback + callback) { + network_response_callback = std::move(callback); + return payments::RequestId("11223344"); + }); + + bool response_received = false; + virtual_card_enrollment_manager_->Unenroll( + /*instrument_id=*/9223372036854775807, + /*virtual_card_enrollment_update_response_callback=*/ + base::BindLambdaForTesting( + [&](payments::PaymentsAutofillClient::PaymentsRpcResult) { + response_received = true; + virtual_card_enrollment_manager_.reset(); + })); + + ASSERT_FALSE(network_response_callback.is_null()); + std::move(network_response_callback) + .Run(payments::PaymentsAutofillClient::PaymentsRpcResult::kSuccess); + + EXPECT_TRUE(response_received); + EXPECT_FALSE(virtual_card_enrollment_manager_); + histogram_tester.ExpectBucketCount( + "Autofill.VirtualCard.Unenroll.Result.SettingsPage", + /*sample=*/true, 1); +} + #if !BUILDFLAG(IS_IOS) TEST_F(VirtualCardEnrollmentManagerTest, StrikeDatabase_BubbleAccepted) { base::HistogramTester histogram_tester;