[Payments]PaymentRequestSpec.java retrieves payment details from c++.

Before:
PaymentRequestSpec.java stores duplicates of payment details to be used
in its getters. This is error prone since the variables on c++ side and
their duplicates on java side need to kept synced.

After:
PaymentRequestSpec.java always retrieves most updated payment details
from c++ implementation.

Bug: 1129708
Change-Id: Ia106446bc08e91a6fcdcfc3875f078d2fd6ed6c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2417543Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Liquan (Max) Gu <maxlg@chromium.org>
Commit-Queue: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809326}

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

@@ -279,15 +279,15 @@ public class PaymentRequestImpl
             mQueryForQuota.put("basic-card-payment-options", paymentMethodData);
         }
 
-        mSpec = PaymentRequestSpec.createAndValidate(
-                details, mPaymentOptions, methodData, LocaleUtils.getDefaultLocaleString());
-        if (mSpec == null
-                || !parseAndValidateDetailsForSkipToGPayHelper(mSpec.getPaymentDetails())) {
+        if (!PaymentValidator.validatePaymentDetails(details)
+                || !parseAndValidateDetailsForSkipToGPayHelper(details)) {
             mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
             disconnectFromClientWithDebugMessage(ErrorStrings.INVALID_PAYMENT_DETAILS);
             return false;
         }
 
+        mSpec = new PaymentRequestSpec(mPaymentOptions, details, methodData.values(),
+                LocaleUtils.getDefaultLocaleString());
         if (mSpec.getRawTotal() == null) {
             mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
             disconnectFromClientWithDebugMessage(ErrorStrings.TOTAL_REQUIRED);
@@ -754,7 +754,7 @@ public class PaymentRequestImpl
             return;
         }
 
-        if (!mSpec.parseAndValidateDetails(details)
+        if (!PaymentValidator.validatePaymentDetails(details)
                 || !parseAndValidateDetailsForSkipToGPayHelper(details)) {
             mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
             disconnectFromClientWithDebugMessage(ErrorStrings.INVALID_PAYMENT_DETAILS);
@@ -802,7 +802,7 @@ public class PaymentRequestImpl
             return;
         }
 
-        if (!mSpec.parseAndValidateDetails(details)
+        if (!PaymentValidator.validatePaymentDetails(details)
                 || !parseAndValidateDetailsForSkipToGPayHelper(details)) {
             mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
             disconnectFromClientWithDebugMessage(ErrorStrings.INVALID_PAYMENT_DETAILS);
@@ -1307,7 +1307,11 @@ public class PaymentRequestImpl
     // PaymentAppFactoryParams implementation.
     @Override
     public Map<String, PaymentMethodData> getMethodData() {
-        return mSpec.getMethodData();
+        // TODO(crbug.com/1130343): This should only get called while mSpec is not destroyed.
+        // uncomment the following assertion after fixing the issue.
+        // assert !mSpec.isDestroyed();
+        return mSpec.isDestroyed() ? new ArrayMap<String, PaymentMethodData>()
+                                   : mSpec.getMethodData();
     }
 
     // PaymentAppFactoryParams implementation.
@@ -1586,7 +1590,6 @@ public class PaymentRequestImpl
                 if (ComponentPaymentRequestImpl.getNativeObserverForTest() != null) {
                     ComponentPaymentRequestImpl.getNativeObserverForTest().onNotSupportedError();
                 }
-
                 if (TextUtils.isEmpty(mRejectShowErrorMessage) && !isInTwa()
                         && mSpec.getMethodData().get(MethodStrings.GOOGLE_PLAY_BILLING) != null) {
                     mRejectShowErrorMessage = ErrorStrings.APP_STORE_METHOD_ONLY_SUPPORTED_IN_TWA;

components/payments/content/android/byte_buffer_helper.h

@@ -9,6 +9,7 @@
 #include <stdint.h>
 #include <vector>
 
+#include "base/android/jni_array.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/check.h"
 #include "mojo/public/cpp/bindings/struct_ptr.h"
@@ -58,6 +59,18 @@ bool DeserializeFromJavaByteBufferArray(
   return true;
 }
 
+// Serializes a vector of native Mojo objects into a Java byte[][].
+template <typename T>
+base::android::ScopedJavaLocalRef<jobjectArray>
+SerializeToJavaArrayOfByteArrays(JNIEnv* env,
+                                 const std::vector<mojo::StructPtr<T>>& input) {
+  std::vector<std::vector<uint8_t>> serialized_elements(input.size());
+  for (size_t i = 0; i < input.size(); i++) {
+    serialized_elements[i] = T::Serialize(&input[i]);
+  }
+  return base::android::ToJavaArrayOfByteArray(env, serialized_elements);
+}
+
 }  // namespace android
 }  // namespace payments
 

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

@@ -21,6 +21,7 @@ import org.chromium.payments.mojom.PaymentValidationErrors;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -32,46 +33,21 @@ import java.util.Map;
  */
 @JNINamespace("payments::android")
 public class PaymentRequestSpec {
-    // TODO(crbug.com/1124917): these parameters duplicate with those in payment_request_spec from
-    // the blink side. We need to de-dup them.
-    private final Map<String, PaymentMethodData> mMethodData;
-    private final String mId;
-    private Map<String, PaymentDetailsModifier> mModifiers = new ArrayMap<>();
-    private List<PaymentShippingOption> mRawShippingOptions;
-    private List<PaymentItem> mRawLineItems;
-    private PaymentItem mRawTotal;
     private long mNativePointer;
-    private PaymentDetails mPaymentDetails;
 
     /**
-     * Creates a valid instance of PaymentRequestSpec with payment parameters, and saves the
-     * parameters in the instance.
-     * @param details The payment details, e.g., the total amount.
+     * Stores the information received from the renderer that invoked the Payment Request API.
+     * Creates an instance of native payment_request_spec.cc with the given parameters.
      * @param options The payment options, e.g., whether shipping is requested.
-     * @param methodData The map of supported payment method identifiers and corresponding payment
+     * @param details The payment details, e.g., the total amount.
+     * @param methodData The list of supported payment method identifiers and corresponding payment
+     * method specific data.
      * @param appLocale The current application locale.
-     * @return The created instance if valid; null otherwise.
      */
-    @Nullable
-    public static PaymentRequestSpec createAndValidate(PaymentDetails details,
-            PaymentOptions options, Map<String, PaymentMethodData> methodData, String appLocale) {
-        PaymentRequestSpec spec = new PaymentRequestSpec(details, methodData);
-        if (!spec.parseAndValidateDetails(details)) return null;
-        spec.createNative(options, appLocale);
-        return spec;
-    }
-
-    private PaymentRequestSpec(PaymentDetails details, Map<String, PaymentMethodData> methodData) {
-        mPaymentDetails = details;
-        mId = mPaymentDetails.id;
-        mMethodData = methodData;
-    }
-
-    private void createNative(PaymentOptions options, String appLocale) {
-        assert mNativePointer == 0;
-        mNativePointer =
-                PaymentRequestSpecJni.get().create(options.serialize(), mPaymentDetails.serialize(),
-                        MojoStructCollection.serialize(mMethodData.values()), appLocale);
+    public PaymentRequestSpec(PaymentOptions options, PaymentDetails details,
+            Collection<PaymentMethodData> methodData, String appLocale) {
+        mNativePointer = PaymentRequestSpecJni.get().create(options.serialize(),
+                details.serialize(), MojoStructCollection.serialize(methodData), appLocale);
     }
 
     /** @return Whether destroy() has been called. */
@@ -84,7 +60,15 @@ public class PaymentRequestSpec {
      * method specific data. This value is still available after the instance is destroyed.
      */
     public Map<String, PaymentMethodData> getMethodData() {
-        return mMethodData;
+        Map<String, PaymentMethodData> methodDataMap = new ArrayMap<>();
+        byte[][] methodDataByteArrays = PaymentRequestSpecJni.get().getMethodData(mNativePointer);
+        for (int i = 0; i < methodDataByteArrays.length; i++) {
+            PaymentMethodData methodData =
+                    PaymentMethodData.deserialize(ByteBuffer.wrap(methodDataByteArrays[i]));
+            String method = methodData.supportedMethod;
+            methodDataMap.put(method, methodData);
+        }
+        return methodDataMap;
     }
 
     /**
@@ -94,7 +78,16 @@ public class PaymentRequestSpec {
      * instance is destroyed.
      */
     public Map<String, PaymentDetailsModifier> getModifiers() {
-        return mModifiers;
+        Map<String, PaymentDetailsModifier> modifiers = new ArrayMap<>();
+        PaymentDetails details = getPaymentDetails();
+        if (details.modifiers != null) {
+            for (int i = 0; i < details.modifiers.length; i++) {
+                PaymentDetailsModifier modifier = details.modifiers[i];
+                String method = modifier.methodData.supportedMethod;
+                modifiers.put(method, modifier);
+            }
+        }
+        return modifiers;
     }
 
     /**
@@ -102,7 +95,7 @@ public class PaymentRequestSpec {
      *         the instance is destroyed.
      */
     public String getId() {
-        return mId;
+        return getPaymentDetails().id;
     }
 
     /**
@@ -111,7 +104,10 @@ public class PaymentRequestSpec {
      * available after the instance is destroyed.
      */
     public List<PaymentShippingOption> getRawShippingOptions() {
-        return mRawShippingOptions;
+        PaymentDetails details = getPaymentDetails();
+        return Collections.unmodifiableList(details.shippingOptions != null
+                        ? Arrays.asList(details.shippingOptions)
+                        : new ArrayList<>());
     }
 
     /**
@@ -119,7 +115,10 @@ public class PaymentRequestSpec {
      * passed to the payment app. This value is still available after the instance is destroyed.
      */
     public List<PaymentItem> getRawLineItems() {
-        return mRawLineItems;
+        PaymentDetails details = getPaymentDetails();
+        return Collections.unmodifiableList(details.displayItems != null
+                        ? Arrays.asList(details.displayItems)
+                        : new ArrayList<>());
     }
 
     /**
@@ -127,57 +126,13 @@ public class PaymentRequestSpec {
      * to the payment app. This value is still available after the instance is destroyed.
      */
     public PaymentItem getRawTotal() {
-        return mRawTotal;
+        return getPaymentDetails().total;
     }
 
     /** @return The payment details specified in the payment request. */
     public PaymentDetails getPaymentDetails() {
-        return mPaymentDetails;
-    }
-
-    /**
-     * Sets the total, display line items, and shipping options based on input and returns the
-     * status boolean. That status is true for valid data, false for invalid data. If the input is
-     * invalid, disconnects from the client. Both raw and UI versions of data are updated.
-     *
-     * @param details The total, line items, and shipping options to parse, validate, and save in
-     *                member variables.
-     * @return True if the data is valid. False if the data is invalid.
-     */
-    public boolean parseAndValidateDetails(PaymentDetails details) {
-        if (!PaymentValidator.validatePaymentDetails(details)) return false;
-
-        if (details.total != null) {
-            mRawTotal = details.total;
-        }
-
-        if (mRawLineItems == null || details.displayItems != null) {
-            mRawLineItems = Collections.unmodifiableList(details.displayItems != null
-                            ? Arrays.asList(details.displayItems)
-                            : new ArrayList<>());
-        }
-
-        if (details.modifiers != null) {
-            if (details.modifiers.length == 0) mModifiers.clear();
-
-            for (int i = 0; i < details.modifiers.length; i++) {
-                PaymentDetailsModifier modifier = details.modifiers[i];
-                String method = modifier.methodData.supportedMethod;
-                mModifiers.put(method, modifier);
-            }
-        }
-
-        if (details.shippingOptions != null) {
-            mRawShippingOptions =
-                    Collections.unmodifiableList(Arrays.asList(details.shippingOptions));
-        } else if (mRawShippingOptions == null) {
-            mRawShippingOptions = Collections.unmodifiableList(new ArrayList<>());
-        }
-
-        assert mRawTotal != null;
-        assert mRawLineItems != null;
-
-        return true;
+        return PaymentDetails.deserialize(
+                ByteBuffer.wrap(PaymentRequestSpecJni.get().getPaymentDetails(mNativePointer)));
     }
 
     /**
@@ -186,7 +141,6 @@ public class PaymentRequestSpec {
      * @param details The updated payment details, e.g., the updated total amount.
      */
     public void updateWith(PaymentDetails details) {
-        mPaymentDetails = details;
         PaymentRequestSpecJni.get().updateWith(mNativePointer, details.serialize());
     }
 
@@ -236,5 +190,7 @@ public class PaymentRequestSpec {
         void recomputeSpecForDetails(long nativePaymentRequestSpec);
         String selectedShippingOptionError(long nativePaymentRequestSpec);
         void destroy(long nativePaymentRequestSpec);
+        byte[] getPaymentDetails(long nativePaymentRequestSpec);
+        byte[][] getMethodData(long nativePaymentRequestSpec);
     }
 }

components/payments/content/android/payment_request_spec.cc

@@ -90,6 +90,17 @@ PaymentRequestSpec::SelectedShippingOptionError(JNIEnv* env) {
       env, spec_->selected_shipping_option_error());
 }
 
+base::android::ScopedJavaLocalRef<jbyteArray>
+PaymentRequestSpec::GetPaymentDetails(JNIEnv* env) {
+  return base::android::ToJavaByteArray(
+      env, mojom::PaymentDetails::Serialize(&spec_->details_ptr()));
+}
+
+base::android::ScopedJavaLocalRef<jobjectArray>
+PaymentRequestSpec::GetMethodData(JNIEnv* env) {
+  return SerializeToJavaArrayOfByteArrays(env, spec_->method_data());
+}
+
 void PaymentRequestSpec::Destroy(JNIEnv* env) {
   delete this;
 }

components/payments/content/android/payment_request_spec.h

@@ -58,6 +58,12 @@ class PaymentRequestSpec {
   base::android::ScopedJavaLocalRef<jstring> SelectedShippingOptionError(
       JNIEnv* env);
 
+  // Returns the payment details.
+  base::android::ScopedJavaLocalRef<jbyteArray> GetPaymentDetails(JNIEnv* env);
+
+  // Returns the method data.
+  base::android::ScopedJavaLocalRef<jobjectArray> GetMethodData(JNIEnv* env);
+
   // Destroys this bridge.
   void Destroy(JNIEnv* env);
 

components/payments/content/payment_request_spec.h

@@ -198,6 +198,8 @@ class PaymentRequestSpec : public PaymentOptionsProvider,
       const;
 
   const mojom::PaymentDetails& details() const { return *details_.get(); }
+  const mojom::PaymentDetailsPtr& details_ptr() const { return details_; }
+
   const std::vector<mojom::PaymentMethodDataPtr>& method_data() const {
     return method_data_;
   }