Reland: Make PaymentRequest a DocumentService

The PaymentRequest's lifetime is bounded by
a) Its mojo receiver's connection
b) The current document in the RenderFrameHost that spawned it
c) It self-destroys on an error, which acts as a shortcut for a).

We would like to:
- avoid holding GlobalFrameRoutingIds, and doing null-checks,
- avoid the fact that the mojo connection can slightly outlive
  the RenderFrameHost, and
- avoid tracking the fragile-and-ever-changing WebContentsObserver
  API to manage the object's lifetime.

DocumentService will ensure that the PaymentRequest (as its
subclass) is destroyed appropriately, and never has a null, or
invalid freed pointer back to the RenderFrameHost.

This change allows us to remove most of the abstraction of the
PaymentRequestWebContentsManager. The PaymentRequest's lifetime
is now fully self-managed, and the object can just be created
directly. But the PaymentRequestWebContentsManager has to
stick around just as a place to hold state that the payments
code wants to attach to the WebContents, that is the
SPCTransactionMode.

R=creis@chromium.org, rouslan@chromium.org

Bug: 1251691, 1252046
Change-Id: Ie90c881fbc80abf1f22fcf7c0a2a51c56d143a8e
Cq-Include-Trybots: luci.chromium.try:linux-bfcache-rel,android-bfcache-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3311758
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Auto-Submit: danakj chromium <danakj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#948181}

base/memory/weak_ptr.h

@@ -28,6 +28,8 @@
 //  class Worker {
 //   public:
 //    static void StartNew(WeakPtr<Controller> controller) {
+//      // Move WeakPtr when possible to avoid atomic refcounting churn on its
+//      // internal state.
 //      Worker* worker = new Worker(std::move(controller));
 //      // Kick off asynchronous processing...
 //    }

chrome/browser/devtools/protocol/page_handler.cc

@@ -86,9 +86,10 @@ protocol::Response PageHandler::SetSPCTransactionMode(
     return protocol::Response::ServerError("Unrecognized mode value");
   }
 
-  payments::PaymentRequestWebContentsManager::GetOrCreateForWebContents(
-      web_contents_.get())
-      ->SetSPCTransactionMode(spc_mode);
+  auto* payment_request_manager =
+      payments::PaymentRequestWebContentsManager::GetOrCreateForWebContents(
+          *web_contents_);
+  payment_request_manager->SetSPCTransactionMode(spc_mode);
   return protocol::Response::Success();
 }
 

chrome/browser/payments/payment_request_factory.cc

@@ -9,6 +9,7 @@
 
 #include "base/no_destructor.h"
 #include "chrome/browser/payments/chrome_payment_request_delegate.h"
+#include "components/payments/content/payment_request.h"
 #include "components/payments/content/payment_request_web_contents_manager.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -53,13 +54,22 @@ void CreatePaymentRequest(
                                            render_frame_host);
   }
 
-  PaymentRequestWebContentsManager::GetOrCreateForWebContents(
-      content::WebContents::FromRenderFrameHost(render_frame_host))
-      ->CreatePaymentRequest(
-          render_frame_host,
-          std::make_unique<ChromePaymentRequestDelegate>(render_frame_host),
-          std::move(receiver),
-          /*observer_for_testing=*/nullptr);
+  auto* web_contents =
+      content::WebContents::FromRenderFrameHost(render_frame_host);
+  CHECK(web_contents);
+  auto* web_contents_manager =
+      PaymentRequestWebContentsManager::GetOrCreateForWebContents(
+          *web_contents);
+
+  auto delegate =
+      std::make_unique<ChromePaymentRequestDelegate>(render_frame_host);
+  auto display_manager = delegate->GetDisplayManager()->GetWeakPtr();
+  // PaymentRequest is a DocumentService, whose lifetime is managed by the
+  // RenderFrameHost passed in here.
+  new PaymentRequest(render_frame_host, std::move(delegate),
+                     std::move(display_manager), std::move(receiver),
+                     web_contents_manager->transaction_mode(),
+                     /*observer_for_testing=*/nullptr);
 }
 
 void SetPaymentRequestFactoryForTesting(

chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc

@@ -82,7 +82,7 @@ IN_PROC_BROWSER_TEST_F(MAYBE_PaymentRequestContactInfoEditorTest, HappyPath) {
             profile->GetInfo(autofill::AutofillType(autofill::EMAIL_ADDRESS),
                              GetLocale()));
 
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->contact_profiles().size());
   EXPECT_EQ(request->state()->contact_profiles().back(),
             request->state()->selected_contact_profile());
@@ -244,7 +244,7 @@ IN_PROC_BROWSER_TEST_F(MAYBE_PaymentRequestContactInfoEditorTest,
   InvokePaymentRequestUI();
   OpenContactInfoScreen();
 
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
 
   // No contact profiles are selected because both are incomplete.
   EXPECT_EQ(nullptr, request->state()->selected_contact_profile());
@@ -303,7 +303,7 @@ IN_PROC_BROWSER_TEST_F(MAYBE_PaymentRequestContactInfoEditorTest,
   // In incognito, the profile should be available in contact_profiles but it
   // shouldn't be saved to the PersonalDataManager.
   ASSERT_EQ(0UL, personal_data_manager->GetProfiles().size());
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->contact_profiles().size());
   EXPECT_EQ(request->state()->contact_profiles().back(),
             request->state()->selected_contact_profile());

chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc

@@ -60,7 +60,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // No apps are available.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(0U, request->state()->available_apps().size());
   EXPECT_EQ(nullptr, request->state()->selected_app());
 
@@ -118,7 +118,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // No apps are available.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(0U, request->state()->available_apps().size());
   EXPECT_EQ(nullptr, request->state()->selected_app());
 
@@ -388,7 +388,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
 
   // One app is available, and it's selected because that's allowed for expired
   // credit cards.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->available_apps().size());
   EXPECT_NE(nullptr, request->state()->selected_app());
 
@@ -473,7 +473,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // One app is available, but it's not selected.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->available_apps().size());
   EXPECT_EQ(nullptr, request->state()->selected_app());
 
@@ -534,7 +534,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // One app is available, but it's not selected.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->available_apps().size());
   EXPECT_EQ(nullptr, request->state()->selected_app());
 
@@ -594,7 +594,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // One app is available, it is not selected, but is properly named.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->available_apps().size());
   EXPECT_EQ(nullptr, request->state()->selected_app());
   EXPECT_EQ(
@@ -645,7 +645,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // One app is available, but it's not selected.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->available_apps().size());
   EXPECT_EQ(nullptr, request->state()->selected_app());
 
@@ -731,7 +731,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // One app is available, but it's not selected.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->available_apps().size());
   EXPECT_EQ(nullptr, request->state()->selected_app());
 
@@ -745,7 +745,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // Still have one app, but now it's selected.
-  request = GetPaymentRequests(GetActiveWebContents()).front();
+  request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->available_apps().size());
   EXPECT_EQ(request->state()->available_apps().back().get(),
             request->state()->selected_app());
@@ -818,7 +818,7 @@ IN_PROC_BROWSER_TEST_F(DISABLED_PaymentRequestCreditCardEditorTest,
   InvokePaymentRequestUI();
 
   // No apps are available.
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(0U, request->state()->available_apps().size());
   EXPECT_EQ(nullptr, request->state()->selected_app());
 

chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc

@@ -77,7 +77,7 @@ IN_PROC_BROWSER_TEST_F(PaymentMethodViewControllerTest, OneCardSelected) {
   InvokePaymentRequestUI();
   OpenPaymentMethodScreen();
 
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(1U, request->state()->available_apps().size());
 
   views::View* list_view = dialog_view()->GetViewByID(
@@ -115,7 +115,7 @@ IN_PROC_BROWSER_TEST_F(PaymentMethodViewControllerTest,
   InvokePaymentRequestUI();
   OpenPaymentMethodScreen();
 
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(2U, request->state()->available_apps().size());
   EXPECT_EQ(request->state()->available_apps().front().get(),
             request->state()->selected_app());

chrome/browser/ui/views/payments/payment_request_browsertest.cc

@@ -17,7 +17,6 @@
 #include "components/autofill/core/browser/data_model/autofill_profile.h"
 #include "components/autofill/core/browser/data_model/credit_card.h"
 #include "components/payments/content/payment_request.h"
-#include "components/payments/content/payment_request_web_contents_manager.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
@@ -32,26 +31,16 @@
 #endif
 
 namespace payments {
+namespace {
 
 using ::testing::UnorderedElementsAre;
 
-class PaymentRequestWebContentsManagerTest
-    : public PaymentRequestBrowserTestBase {
- public:
-  PaymentRequestWebContentsManagerTest(
-      const PaymentRequestWebContentsManagerTest&) = delete;
-  PaymentRequestWebContentsManagerTest& operator=(
-      const PaymentRequestWebContentsManagerTest&) = delete;
-
- protected:
-  PaymentRequestWebContentsManagerTest() {}
-};
+class PaymentRequestTest : public PaymentRequestBrowserTestBase {};
 
 // If the page creates multiple PaymentRequest objects, it should not crash.
-IN_PROC_BROWSER_TEST_F(PaymentRequestWebContentsManagerTest, MultipleRequests) {
+IN_PROC_BROWSER_TEST_F(PaymentRequestTest, MultipleRequests) {
   NavigateTo("/payment_request_multiple_requests.html");
-  const std::vector<PaymentRequest*> payment_requests =
-      GetPaymentRequests(GetActiveWebContents());
+  const std::vector<PaymentRequest*> payment_requests = GetPaymentRequests();
   EXPECT_EQ(5U, payment_requests.size());
 }
 
@@ -264,8 +253,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestPaymentMethodIdentifierTest,
   NavigateTo("/payment_request_payment_method_identifier_test.html");
   InvokePaymentRequestWithJs("buy();");
 
-  std::vector<PaymentRequest*> requests =
-      GetPaymentRequests(GetActiveWebContents());
+  std::vector<PaymentRequest*> requests = GetPaymentRequests();
   EXPECT_EQ(1u, requests.size());
   std::vector<std::string> supported_card_networks =
       requests[0]->spec()->supported_card_networks();
@@ -283,8 +271,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestPaymentMethodIdentifierTest,
   NavigateTo("/payment_request_payment_method_identifier_test.html");
   InvokePaymentRequestWithJs("buyHelper([basicCardMethod]);");
 
-  std::vector<PaymentRequest*> requests =
-      GetPaymentRequests(GetActiveWebContents());
+  std::vector<PaymentRequest*> requests = GetPaymentRequests();
   EXPECT_EQ(1u, requests.size());
   std::vector<std::string> supported_card_networks =
       requests[0]->spec()->supported_card_networks();
@@ -311,8 +298,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestPaymentMethodIdentifierTest, Url_Valid) {
       "  supportedMethods: 'basic-card'"
       "}]);");
 
-  std::vector<PaymentRequest*> requests =
-      GetPaymentRequests(GetActiveWebContents());
+  std::vector<PaymentRequest*> requests = GetPaymentRequests();
   EXPECT_EQ(1u, requests.size());
   std::vector<GURL> url_payment_method_identifiers =
       requests[0]->spec()->url_payment_method_identifiers();
@@ -391,4 +377,5 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestSettingsLinkTest, ClickSettingsLink) {
       new_tab_contents->GetVisibleURL().spec());
 }
 
+}  // namespace
 }  // namespace payments

chrome/browser/ui/views/payments/payment_request_browsertest_base.cc

@@ -33,7 +33,6 @@
 #include "components/autofill/core/browser/ui/address_combobox_model.h"
 #include "components/network_session_configurator/common/network_switches.h"
 #include "components/payments/content/payment_request.h"
-#include "components/payments/content/payment_request_web_contents_manager.h"
 #include "components/payments/core/payment_prefs.h"
 #include "components/prefs/pref_service.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
@@ -464,17 +463,14 @@ content::WebContents* PaymentRequestBrowserTestBase::GetActiveWebContents() {
 }
 
 const std::vector<PaymentRequest*>
-PaymentRequestBrowserTestBase::GetPaymentRequests(
-    content::WebContents* web_contents) {
-  PaymentRequestWebContentsManager* manager =
-      PaymentRequestWebContentsManager::GetOrCreateForWebContents(web_contents);
-  if (!manager)
-    return std::vector<PaymentRequest*>();
-
-  std::vector<PaymentRequest*> payment_requests_ptrs;
-  for (const auto& p : manager->payment_requests_)
-    payment_requests_ptrs.push_back(p.first);
-  return payment_requests_ptrs;
+PaymentRequestBrowserTestBase::GetPaymentRequests() {
+  std::vector<PaymentRequest*> ptrs;
+  ptrs.reserve(requests_.size());
+  for (const auto& weak : requests_) {
+    if (weak)
+      ptrs.push_back(&*weak);
+  }
+  return ptrs;
 }
 
 autofill::PersonalDataManager* PaymentRequestBrowserTestBase::GetDataManager() {
@@ -548,10 +544,11 @@ void PaymentRequestBrowserTestBase::CreatePaymentRequestForTest(
           render_frame_host, /*observer=*/GetWeakPtr(), &prefs_, is_incognito_,
           is_valid_ssl_, is_browser_window_active_, skip_ui_for_basic_card_);
   delegate_ = delegate.get();
-  PaymentRequestWebContentsManager::GetOrCreateForWebContents(
-      content::WebContents::FromRenderFrameHost(render_frame_host))
-      ->CreatePaymentRequest(render_frame_host, std::move(delegate),
-                             std::move(receiver), GetWeakPtr());
+  auto display_manager = delegate->GetDisplayManager()->GetWeakPtr();
+  auto* request = new PaymentRequest(
+      render_frame_host, std::move(delegate), std::move(display_manager),
+      std::move(receiver), SPCTransactionMode::NONE, GetWeakPtr());
+  requests_.push_back(request->GetWeakPtr());
 }
 
 void PaymentRequestBrowserTestBase::ClickOnDialogViewAndWait(

chrome/browser/ui/views/payments/payment_request_browsertest_base.h

@@ -176,10 +176,8 @@ class PaymentRequestBrowserTestBase
 
   content::WebContents* GetActiveWebContents();
 
-  // Convenience method to get a list of PaymentRequest associated with
-  // |web_contents|.
-  const std::vector<PaymentRequest*> GetPaymentRequests(
-      content::WebContents* web_contents);
+  // Convenience method to get all the `PaymentRequest`s that are still alive.
+  const std::vector<PaymentRequest*> GetPaymentRequests();
 
   autofill::PersonalDataManager* GetDataManager();
   // Adds the various models to the database, waiting until the personal data
@@ -296,6 +294,7 @@ class PaymentRequestBrowserTestBase
   bool is_valid_ssl_ = true;
   bool is_browser_window_active_ = true;
   bool skip_ui_for_basic_card_ = false;
+  std::vector<base::WeakPtr<PaymentRequest>> requests_;
 
   base::WeakPtrFactory<PaymentRequestBrowserTestBase> weak_ptr_factory_{this};
 };

chrome/browser/ui/views/payments/payment_request_retry_browsertest.cc

@@ -33,7 +33,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestRetryTest,
 
   // Confirm that there are two payment apps available.
   InvokePaymentRequestUI();
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
   EXPECT_EQ(2U, request->state()->available_apps().size());
 
   // Enter a valid CVC format for the Visa card.

chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc

@@ -52,7 +52,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestProfileListTest, PrioritizeCompleteness) {
 
   InvokePaymentRequestUI();
 
-  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+  PaymentRequest* request = GetPaymentRequests().front();
 
   // The complete profile should be selected.
   ASSERT_TRUE(request->state()->selected_shipping_profile());