[WebPayments] Desktop: Adding validation and binding checks

chrome/browser/payments/payment_request_impl.cc

 // 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 "chrome/browser/payments/payment_request_impl.h"
 
 #include <map>
 
 #include "base/lazy_instance.h"
 #include "chrome/browser/ui/browser_dialogs.h"
+#include "components/payments/payment_details_validation.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 
 namespace {
 
 class PaymentRequestFactory {
  public:
   bool AssignPaymentRequest(
       content::WebContents* web_contents,
       mojo::InterfaceRequest<payments::mojom::PaymentRequest> request) {
@@ skipping 34 matching lines @@
 
 PaymentRequestImpl::~PaymentRequestImpl() {}
 
 void PaymentRequestImpl::Init(
     payments::mojom::PaymentRequestClientPtr client,
     std::vector<payments::mojom::PaymentMethodDataPtr> methodData,
     payments::mojom::PaymentDetailsPtr details,
     payments::mojom::PaymentOptionsPtr options) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   client_ = std::move(client);
+  std::string err_msg;
+  if (!validatePaymentDetails(details, &err_msg)) {

[please use gerrit instead, 2017/01/05 20:11:58]

Also:

LOG(ERROR) << err_msg;

By the way, you may also want to close binding_ and call that
UnassignPaymentRequest thingy.

You don't have to call client_->OnError(), because that tells renderer that an
error occurred. In fact, if this validation error happens in the browser, then
the renderer is already compromised. So there's no need to talk back to it.
Therefore, client_.reset() is sufficient.

+    client_->OnError(payments::mojom::PaymentErrorReason::USER_CANCEL);
+    client_.reset();
+  }
 }
 
 void PaymentRequestImpl::Show() {
-  chrome::ShowPaymentRequestDialog(this);
+  if (client_.is_bound() && binding_.is_bound())
+    chrome::ShowPaymentRequestDialog(this);
 }

[please use gerrit instead, 2017/01/05 20:11:58]

Can you do this? it should ensure that PaymentRequestImpl is deleted, I think.
krb@ originally designed the factory and knows best how it works, but I hear
mathp@ wants to improve on it.

if (!client_.is_bound() || !binding_.is_bound()) {
  payment_request_factory.Get().UnassignPaymentRequest(web_contents_);
  return;
}


// The rest of code.

 
 void PaymentRequestImpl::Cancel() {
   client_->OnError(payments::mojom::PaymentErrorReason::USER_CANCEL);
 }
 
 void PaymentRequestImpl::OnError() {
   binding_.Close();
   // TODO(krb): Close the dialog here, but avoid double-free
   payment_request_factory.Get().UnassignPaymentRequest(web_contents_);
 }
 
 }  // namespace payments
 
 void CreatePaymentRequestHandler(
     content::WebContents* web_contents,
     mojo::InterfaceRequest<payments::mojom::PaymentRequest> request) {
   payment_request_factory.Get().AssignPaymentRequest(web_contents,
                                                      std::move(request));
 }