PaymentRequest: Ignore shipping options if there are duplicated IDs.

third_party/WebKit/Source/modules/payments/PaymentRequest.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/payments/PaymentRequest.h"
 
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/JSONValuesForV8.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "bindings/core/v8/ScriptState.h"
@@ skipping 205 matching lines @@
 
 void validateDisplayItems(const HeapVector<PaymentItem>& items,
                           ExceptionState& exceptionState) {
   for (const auto& item : items) {
     validateShippingOptionOrPaymentItem(item, exceptionState);
     if (exceptionState.hadException())
       return;
   }
 }
 
-void validateShippingOptions(const HeapVector<PaymentShippingOption>& options,
-                             ExceptionState& exceptionState) {
+void validateAndFixupShippingOptions(PaymentDetails& details,

[please use gerrit instead, 2016/10/12 01:28:02]

nit: Pass in only "HeapVector<PaymentShippingOption>& options", so it's clear
that this function acts only on the shipping options. You can reset it via
"options = HeapVector<PaymentShippingOption>();"

[zino, 2016/10/12 18:05:24]

Done.

+                                     ExceptionState& exceptionState) {
   HashSet<String> uniqueIds;
-  for (const auto& option : options) {
+  for (const auto& option : details.shippingOptions()) {
     if (!option.hasId() || option.id().isEmpty()) {
       exceptionState.throwTypeError("ShippingOption id required");
       return;
     }
 
     if (uniqueIds.contains(option.id())) {
-      exceptionState.throwTypeError(
-          "Duplicate shipping option identifiers are not allowed");
+      details.setShippingOptions(HeapVector<PaymentShippingOption>());
       return;
     }
     uniqueIds.add(option.id());
 
     validateShippingOptionOrPaymentItem(option, exceptionState);
     if (exceptionState.hadException())
       return;
   }
 }
 
@@ skipping 36 matching lines @@
     }
 
     if (modifier.hasAdditionalDisplayItems()) {
       validateDisplayItems(modifier.additionalDisplayItems(), exceptionState);
       if (exceptionState.hadException())
         return;
     }
   }
 }
 
-void validatePaymentDetails(const PaymentDetails& details,
-                            ExceptionState& exceptionState) {
+void validateAndFixupPaymentDetails(PaymentDetails& details,
+                                    ExceptionState& exceptionState) {
   if (!details.hasTotal()) {
     exceptionState.throwTypeError("Must specify total");
     return;
   }
 
   validateShippingOptionOrPaymentItem(details.total(), exceptionState);
   if (exceptionState.hadException())
     return;
 
   if (details.total().amount().value()[0] == '-') {
     exceptionState.throwTypeError("Total amount value should be non-negative");
     return;
   }
 
   if (details.hasDisplayItems()) {
     validateDisplayItems(details.displayItems(), exceptionState);
     if (exceptionState.hadException())
       return;
   }
 
   if (details.hasShippingOptions()) {
-    validateShippingOptions(details.shippingOptions(), exceptionState);
+    validateAndFixupShippingOptions(details, exceptionState);
     if (exceptionState.hadException())
       return;
   }
 
   if (details.hasModifiers()) {
     validatePaymentDetailsModifiers(details.modifiers(), exceptionState);
   }
 
   String errorMessage;
   if (!PaymentsValidators::isValidErrorMsgFormat(details.error(),
@@ skipping 189 matching lines @@
   V8PaymentDetails::toImpl(detailsScriptValue.isolate(),
                            detailsScriptValue.v8Value(), details,
                            exceptionState);
   if (exceptionState.hadException()) {
     m_showResolver->reject(
         DOMException::create(SyntaxError, exceptionState.message()));
     clearResolversAndCloseMojoConnection();
     return;
   }
 
-  validatePaymentDetails(details, exceptionState);
+  validateAndFixupPaymentDetails(details, exceptionState);
   if (exceptionState.hadException()) {
     m_showResolver->reject(
         DOMException::create(SyntaxError, exceptionState.message()));
     clearResolversAndCloseMojoConnection();
     return;
   }
 
   if (m_options.requestShipping())
     m_shippingOption = getSelectedShippingOption(details);
 
@@ skipping 44 matching lines @@
     return;
   }
 
   if (!scriptState->domWindow()->frame() ||
       !scriptState->domWindow()->frame()->isMainFrame()) {
     exceptionState.throwSecurityError(
         "Must be in a top-level browsing context");
     return;
   }
 
-  validatePaymentDetails(details, exceptionState);
+  PaymentDetails fixedDetails(details);
+  validateAndFixupPaymentDetails(fixedDetails, exceptionState);
   if (exceptionState.hadException())
     return;
 
   if (details.hasError() && !details.error().isEmpty()) {

[please use gerrit instead, 2016/10/12 01:28:02]

To ensure correctness, stop using "details" immediately after you make a
writable copy. So please use "fixedDetails" on this line.

[zino, 2016/10/12 18:05:24]

Done.

     exceptionState.throwTypeError("Error value should be empty");
     return;
   }
 
   if (m_options.requestShipping()) {
-    m_shippingOption = getSelectedShippingOption(details);
+    m_shippingOption = getSelectedShippingOption(fixedDetails);
     m_shippingType = getValidShippingType(m_options.shippingType());
   }
 
   scriptState->domWindow()->frame()->interfaceProvider()->getInterface(
       mojo::GetProxy(&m_paymentProvider));
   m_paymentProvider.set_connection_error_handler(convertToBaseCallback(
       WTF::bind(&PaymentRequest::OnError, wrapWeakPersistent(this),
                 mojom::blink::PaymentErrorReason::UNKNOWN)));
   m_paymentProvider->Init(
       m_clientBinding.CreateInterfacePtrAndBind(),
       mojo::WTFArray<mojom::blink::PaymentMethodDataPtr>::From(
           validatedMethodData),
-      mojom::blink::PaymentDetails::From(details),
+      mojom::blink::PaymentDetails::From(fixedDetails),
       mojom::blink::PaymentOptions::From(m_options));
 }
 
 void PaymentRequest::contextDestroyed() {
   clearResolversAndCloseMojoConnection();
 }
 
 void PaymentRequest::OnShippingAddressChange(
     mojom::blink::PaymentAddressPtr address) {
   DCHECK(m_showResolver);
@@ skipping 157 matching lines @@
   m_completeTimer.stop();
   m_completeResolver.clear();
   m_showResolver.clear();
   m_abortResolver.clear();
   if (m_clientBinding.is_bound())
     m_clientBinding.Close();
   m_paymentProvider.reset();
 }
 
 }  // namespace blink