[PRImpl] Move MethodData into PaymentRequestSpec

* Move PaymentRequestImpl's mMethodData into PaymentRequestSpec.
* mSpec's parsed values are still be available after mSpec is closed.
This change is motivated by the fact that AutofillPaymentAppFactory
accesses PRImpl#getMethodData() after mSpec is closed, causing
NullPointerException[1]. In particular, this change includes:
 - mSpec is no longer set null when destroyed.
 - the callers check mSpec.isDestroyed() (added method) before using
its updateWith(), retry(), recomputeSpecForDetails(),
selectedShippingOptionError(). Other methods are accessible even after
mSpec is destroyed.

[1] https://ci.chromium.org/p/chromium/builders/try/android-marshmallow-arm64-rel/648624?

Bug: 1102522

Change-Id: I018d35f50bd2ab42aa4d3b0c80e5245ad86394fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390867
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805322}

chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java

@@ -225,7 +225,6 @@ public class PaymentRequestImpl
     private boolean mHasClosed;
 
     private PaymentRequestSpec mSpec;
-    private Map<String, PaymentMethodData> mMethodData;
     private int mShippingType;
     private boolean mIsFinishedQueryingPaymentApps;
     private List<PaymentApp> mPendingApps = new ArrayList<>();
@@ -324,10 +323,9 @@ public class PaymentRequestImpl
 
     // Implement BrowserPaymentRequest:
     @Override
-    public boolean initAndValidate(PaymentMethodData[] methodData, PaymentDetails details,
+    public boolean initAndValidate(PaymentMethodData[] rawMethodData, PaymentDetails details,
             @Nullable PaymentOptions options, boolean googlePayBridgeEligible) {
         assert mComponentPaymentRequestImpl != null;
-        mMethodData = new HashMap<>();
         mJourneyLogger.recordCheckoutStep(CheckoutFunnelStep.INITIATED);
 
         mPaymentOptions = options;
@@ -360,22 +358,23 @@ public class PaymentRequestImpl
         }
 
         boolean googlePayBridgeActivated = googlePayBridgeEligible
-                && SkipToGPayHelper.canActivateExperiment(mWebContents, methodData);
+                && SkipToGPayHelper.canActivateExperiment(mWebContents, rawMethodData);
 
-        mMethodData = getValidatedMethodData(
-                methodData, googlePayBridgeActivated, mPaymentUIsManager.getCardEditor());
-        if (mMethodData == null) {
+        Map<String, PaymentMethodData> methodData = getValidatedMethodData(
+                rawMethodData, googlePayBridgeActivated, mPaymentUIsManager.getCardEditor());
+
+        if (methodData == null) {
             mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
             disconnectFromClientWithDebugMessage(ErrorStrings.INVALID_PAYMENT_METHODS_OR_DATA);
             return false;
         }
 
         if (googlePayBridgeActivated) {
-            PaymentMethodData data = mMethodData.get(MethodStrings.GOOGLE_PAY);
+            PaymentMethodData data = methodData.get(MethodStrings.GOOGLE_PAY);
             mSkipToGPayHelper = new SkipToGPayHelper(options, data.gpayBridgeData);
         }
 
-        mQueryForQuota = new HashMap<>(mMethodData);
+        mQueryForQuota = new HashMap<>(methodData);
         if (mQueryForQuota.containsKey(MethodStrings.BASIC_CARD)
                 && PaymentFeatureList.isEnabledOrExperimentalFeaturesEnabled(
                         PaymentFeatureList.STRICT_HAS_ENROLLED_AUTOFILL_INSTRUMENT)) {
@@ -385,7 +384,7 @@ public class PaymentRequestImpl
             mQueryForQuota.put("basic-card-payment-options", paymentMethodData);
         }
 
-        mSpec = new PaymentRequestSpec(mPaymentOptions, mMethodData.values());
+        mSpec = new PaymentRequestSpec(mPaymentOptions, methodData);
         if (parseAndValidateDetails(details)
                 && parseAndValidateDetailsForSkipToGPayHelper(details)) {
             mPaymentUIsManager.updateDetailsOnPaymentRequestUI(
@@ -420,7 +419,7 @@ public class PaymentRequestImpl
         // better captures this group of interest than requestedMethodBasicCard.
         boolean requestedMethodOther = false;
         mURLPaymentMethodIdentifiersSupported = false;
-        for (String methodName : mMethodData.keySet()) {
+        for (String methodName : mSpec.getMethodData().keySet()) {
             switch (methodName) {
                 case MethodStrings.ANDROID_PAY:
                 case MethodStrings.GOOGLE_PAY:
@@ -906,6 +905,8 @@ public class PaymentRequestImpl
     @Override
     public void updateWith(PaymentDetails details) {
         if (mComponentPaymentRequestImpl == null) return;
+        // mSpec.updateWith() can be used only when mSpec has not been destroyed.
+        assert !mSpec.isDestroyed();
 
         if (mWaitForUpdatedDetails) {
             initializeWithUpdatedDetails(details);
@@ -965,6 +966,8 @@ public class PaymentRequestImpl
 
     private void initializeWithUpdatedDetails(PaymentDetails details) {
         assert mWaitForUpdatedDetails;
+        // mSpec.updateWith() can be used only when mSpec has not been destroyed.
+        assert !mSpec.isDestroyed();
 
         ChromeActivity chromeActivity = ChromeActivity.fromWebContents(mWebContents);
         if (chromeActivity == null) {
@@ -1027,6 +1030,9 @@ public class PaymentRequestImpl
     @Override
     public void onPaymentDetailsNotUpdated() {
         if (mComponentPaymentRequestImpl == null) return;
+        // mSpec.recomputeSpecForDetails(), mSpec.selectedShippingOptionError() can be used only
+        // when mSpec has not been destroyed.
+        assert !mSpec.isDestroyed();
 
         if (mPaymentUIsManager.getPaymentRequestUI() == null) {
             mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
@@ -1292,8 +1298,8 @@ public class PaymentRequestImpl
         Map<String, PaymentDetailsModifier> modifiers = new HashMap<>();
         boolean isGooglePaymentApp = false;
         for (String paymentMethodName : mInvokedPaymentApp.getInstrumentMethodNames()) {
-            if (mMethodData.containsKey(paymentMethodName)) {
-                methodData.put(paymentMethodName, mMethodData.get(paymentMethodName));
+            if (mSpec.getMethodData().containsKey(paymentMethodName)) {
+                methodData.put(paymentMethodName, mSpec.getMethodData().get(paymentMethodName));
             }
             if (mSpec.getModifiers().containsKey(paymentMethodName)) {
                 modifiers.put(paymentMethodName, mSpec.getModifiers().get(paymentMethodName));
@@ -1464,6 +1470,8 @@ public class PaymentRequestImpl
     @Override
     public void retry(PaymentValidationErrors errors) {
         if (mComponentPaymentRequestImpl == null) return;
+        // mSpec.retry() can be used only when mSpec has not been destroyed.
+        assert !mSpec.isDestroyed();
 
         if (!PaymentValidator.validatePaymentValidationErrors(errors)) {
             mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
@@ -1618,7 +1626,7 @@ public class PaymentRequestImpl
     // PaymentAppFactoryParams implementation.
     @Override
     public Map<String, PaymentMethodData> getMethodData() {
-        return mMethodData;
+        return mSpec.getMethodData();
     }
 
     // PaymentAppFactoryParams implementation.
@@ -1899,11 +1907,11 @@ public class PaymentRequestImpl
                 }
 
                 if (TextUtils.isEmpty(mRejectShowErrorMessage) && !isInTwa()
-                        && mMethodData.get(MethodStrings.GOOGLE_PLAY_BILLING) != null) {
+                        && mSpec.getMethodData().get(MethodStrings.GOOGLE_PLAY_BILLING) != null) {
                     mRejectShowErrorMessage = ErrorStrings.APP_STORE_METHOD_ONLY_SUPPORTED_IN_TWA;
                 }
                 disconnectFromClientWithDebugMessage(
-                        ErrorMessageUtil.getNotSupportedErrorMessage(mMethodData.keySet())
+                        ErrorMessageUtil.getNotSupportedErrorMessage(mSpec.getMethodData().keySet())
                                 + (TextUtils.isEmpty(mRejectShowErrorMessage)
                                                 ? ""
                                                 : " " + mRejectShowErrorMessage),
@@ -1928,7 +1936,7 @@ public class PaymentRequestImpl
      * @return Whether client has been disconnected.
      */
     private boolean disconnectForStrictShow() {
-        if (!mIsUserGestureShow || !mMethodData.containsKey(MethodStrings.BASIC_CARD)
+        if (!mIsUserGestureShow || !mSpec.getMethodData().containsKey(MethodStrings.BASIC_CARD)
                 || mHasEnrolledInstrument || mHasNonAutofillApp
                 || !PaymentFeatureList.isEnabledOrExperimentalFeaturesEnabled(
                         PaymentFeatureList.STRICT_HAS_ENROLLED_AUTOFILL_INSTRUMENT)) {
@@ -1940,7 +1948,7 @@ public class PaymentRequestImpl
         }
         mRejectShowErrorMessage = ErrorStrings.STRICT_BASIC_CARD_SHOW_REJECT;
         disconnectFromClientWithDebugMessage(
-                ErrorMessageUtil.getNotSupportedErrorMessage(mMethodData.keySet()) + " "
+                ErrorMessageUtil.getNotSupportedErrorMessage(mSpec.getMethodData().keySet()) + " "
                         + mRejectShowErrorMessage,
                 PaymentErrorReason.NOT_SUPPORTED);
 
@@ -2133,7 +2141,6 @@ public class PaymentRequestImpl
 
         if (mSpec != null) {
             mSpec.destroy();
-            mSpec = null;
         }
         PaymentDetailsUpdateServiceHelper.getInstance().reset();
     }

components/payments/content/android/java/src/org/chromium/components/payments/PaymentRequestSpec.java

@@ -19,7 +19,6 @@ import org.chromium.payments.mojom.PaymentShippingOption;
 import org.chromium.payments.mojom.PaymentValidationErrors;
 
 import java.nio.ByteBuffer;
-import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 
@@ -38,17 +37,17 @@ public class PaymentRequestSpec {
     private List<PaymentItem> mRawLineItems;
     private PaymentItem mRawTotal;
     private final PaymentOptions mOptions;
-    private final Collection<PaymentMethodData> mMethodData;
+    private final Map<String, PaymentMethodData> mMethodData;
     private long mNativePointer;
 
     /**
      * Creates an instance to store the information received from the renderer that invoked the
      * Payment Request API.
      * @param options The payment options, e.g., whether shipping is requested.
-     * @param methodData The list of supported payment method identifiers and corresponding payment
+     * @param methodData The map of supported payment method identifiers and corresponding payment
      * method specific data.
      */
-    public PaymentRequestSpec(PaymentOptions options, Collection<PaymentMethodData> methodData) {
+    public PaymentRequestSpec(PaymentOptions options, Map<String, PaymentMethodData> methodData) {
         mOptions = options;
         mMethodData = methodData;
     }
@@ -60,20 +59,38 @@ public class PaymentRequestSpec {
      * @param appLocale The current application locale.
      */
     public void createNative(PaymentDetails details, String appLocale) {
-        mNativePointer = PaymentRequestSpecJni.get().create(mOptions.serialize(),
-                details.serialize(), MojoStructCollection.serialize(mMethodData), appLocale);
+        mNativePointer =
+                PaymentRequestSpecJni.get().create(mOptions.serialize(), details.serialize(),
+                        MojoStructCollection.serialize(mMethodData.values()), appLocale);
+    }
+
+    /** @return Whether destroy() has been called. */
+    public boolean isDestroyed() {
+        return mNativePointer == 0;
+    }
+
+    /**
+     * @return The map of supported payment method identifiers and corresponding payment
+     * method specific data. This value is still available after the instance is destroyed.
+     */
+    public Map<String, PaymentMethodData> getMethodData() {
+        return mMethodData;
     }
 
     /**
      * A mapping from method names to modifiers, which include modified totals and additional line
      * items. Used to display modified totals for each payment apps, modified total in order
-     * summary, and additional line items in order summary.
+     * summary, and additional line items in order summary. This value is still available after the
+     * instance is destroyed.
      */
     public Map<String, PaymentDetailsModifier> getModifiers() {
         return mModifiers;
     }
 
-    /** @return The id of the request, found in PaymentDetails. */
+    /**
+     * @return The id of the request, found in PaymentDetails. This value is still available after
+     *         the instance is destroyed.
+     */
     public String getId() {
         return mId;
     }
@@ -85,7 +102,8 @@ public class PaymentRequestSpec {
 
     /**
      * The raw shipping options, as it was received from the website. This data is passed to the
-     * payment app when the app is responsible for handling shipping address.
+     * payment app when the app is responsible for handling shipping address. This value is still
+     * available after the instance is destroyed.
      */
     public List<PaymentShippingOption> getRawShippingOptions() {
         return mRawShippingOptions;
@@ -98,7 +116,7 @@ public class PaymentRequestSpec {
 
     /**
      * The raw items in the shopping cart, as they were received from the website. This data is
-     * passed to the payment app.
+     * passed to the payment app. This value is still available after the instance is destroyed.
      */
     public List<PaymentItem> getRawLineItems() {
         return mRawLineItems;
@@ -111,7 +129,7 @@ public class PaymentRequestSpec {
 
     /**
      * The raw total amount being charged, as it was received from the website. This data is passed
-     * to the payment app.
+     * to the payment app. This value is still available after the instance is destroyed.
      */
     public PaymentItem getRawTotal() {
         return mRawTotal;
@@ -124,7 +142,7 @@ public class PaymentRequestSpec {
 
     /**
      * Called when the renderer updates the payment details in response to, e.g., new shipping
-     * address.
+     * address. This cannot be used after the instance is destroyed.
      * @param details The updated payment details, e.g., the updated total amount.
      */
     public void updateWith(PaymentDetails details) {
@@ -132,7 +150,8 @@ public class PaymentRequestSpec {
     }
 
     /**
-     * Called when merchant retries a failed payment.
+     * Called when merchant retries a failed payment. This cannot be used after the instance is
+     * destroyed.
      * @param validationErrors The information about the fields that failed the validation.
      */
     public void retry(PaymentValidationErrors validationErrors) {
@@ -140,14 +159,15 @@ public class PaymentRequestSpec {
     }
 
     /**
-     * Recomputes spec based on details.
+     * Recomputes spec based on details. This cannot be used after the instance is destroyed.
      */
     public void recomputeSpecForDetails() {
         PaymentRequestSpecJni.get().recomputeSpecForDetails(mNativePointer);
     }
 
     /**
-     * Returns the selected shipping option error.
+     * Returns the selected shipping option error. This cannot be used after the instance is
+     * destroyed.
      */
     @Nullable
     public String selectedShippingOptionError() {