Reland "PaymentRequest: Fix a memory leak in retry()"

Reason for revert:
A crash occurs because two animations runs in the same time.
In the current implementation, the ViewStack class seems to be able to
run only one animation per one time. So, before one animation finishes,
if another animation runs, it might cause a crash.

The ShowInitialPaymentSheet() doesn't make animation internally but the
GoBackToPaymentSheet() do. So, after retry() calls, if shipping address
or contact editor runs, it might cause a crash because two animations
runs in the same time.

To resolve this issue, this CL adds a parameter to
GoBackToPaymentSheet() to control whether animation runs or not. If the
`animate` parameter is false, just runs without animation. We don't need
to run animation when retry() calls because the payment sheet is already
hidden and the processing spinner is showing.

Original change's description:
> In the current implementation, memory leaks occur becuase the internal
> view_stack_ for payment sheet grows incrementally whenver calling
> retry(). So, this patch uses GoBackToPaymentSheet() instead of
> ShowInitialPaymentSheet() in RetryDialog(). The method has a logic to
> reduce stack's size internally.
>
> Bug: 861704
> Change-Id: I2cb94772485165d1fa96463f16f39edf20ccf7bc
> Reviewed-on: https://chromium-review.googlesource.com/1255082
> Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
> Commit-Queue: Jinho Bang <jinho.bang@samsung.com>
> Cr-Commit-Position: refs/heads/master@{#595644}

Bug: 861704
Change-Id: I5ab64423697aa11de97bf68ec9d9f262d7676bb5
Reviewed-on: https://chromium-review.googlesource.com/c/1258813Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Jinho Bang <jinho.bang@samsung.com>
Cr-Commit-Position: refs/heads/master@{#596232}

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

@@ -344,7 +344,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestContactInfoEditorTest,
       "    phone: 'PHONE ERROR'"
       "  }"
       "}",
-      DialogEvent::CONTACT_INFO_EDITOR_OPENED);
+      DialogEvent::CONTACT_INFO_EDITOR_OPENED, dialog_view());
 
   EXPECT_EQ(base::ASCIIToUTF16("EMAIL ERROR"),
             GetErrorLabelForType(autofill::EMAIL_ADDRESS));
@@ -381,7 +381,7 @@ IN_PROC_BROWSER_TEST_F(
       "    phone: 'PHONE ERROR'"
       "  }"
       "}",
-      DialogEvent::CONTACT_INFO_EDITOR_OPENED);
+      DialogEvent::CONTACT_INFO_EDITOR_OPENED, dialog_view());
 
   EXPECT_EQ(base::ASCIIToUTF16("EMAIL ERROR"),
             GetErrorLabelForType(autofill::EMAIL_ADDRESS));
@@ -411,7 +411,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestContactInfoEditorTest,
       "    name: 'NAME ERROR',"
       "    phone: 'PHONE ERROR'"
       "  }"
-      "}");
+      "}",
+      dialog_view());
 
   const int kErrorLabelOffset =
       static_cast<int>(DialogViewID::ERROR_LABEL_OFFSET);

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

@@ -638,10 +638,13 @@ void PaymentRequestBrowserTestBase::PayWithCreditCard(
 }
 
 void PaymentRequestBrowserTestBase::RetryPaymentRequest(
-    const std::string& validation_errors) {
-  ResetEventWaiterForSequence(
-      {DialogEvent::PROCESSING_SPINNER_HIDDEN, DialogEvent::SPEC_DONE_UPDATING,
-       DialogEvent::PROCESSING_SPINNER_HIDDEN, DialogEvent::DIALOG_OPENED});
+    const std::string& validation_errors,
+    PaymentRequestDialogView* dialog_view) {
+  EXPECT_EQ(2U, dialog_view->view_stack_for_testing()->size());
+  ResetEventWaiterForSequence({DialogEvent::PROCESSING_SPINNER_HIDDEN,
+                               DialogEvent::SPEC_DONE_UPDATING,
+                               DialogEvent::PROCESSING_SPINNER_HIDDEN,
+                               DialogEvent::BACK_TO_PAYMENT_SHEET_NAVIGATION});
 
   ASSERT_TRUE(content::ExecuteScript(GetActiveWebContents(),
                                      "retry(" + validation_errors + ");"));
@@ -651,11 +654,13 @@ void PaymentRequestBrowserTestBase::RetryPaymentRequest(
 
 void PaymentRequestBrowserTestBase::RetryPaymentRequest(
     const std::string& validation_errors,
-    const DialogEvent& dialog_event) {
+    const DialogEvent& dialog_event,
+    PaymentRequestDialogView* dialog_view) {
+  EXPECT_EQ(2U, dialog_view->view_stack_for_testing()->size());
   ResetEventWaiterForSequence(
       {DialogEvent::PROCESSING_SPINNER_HIDDEN, DialogEvent::SPEC_DONE_UPDATING,
-       DialogEvent::PROCESSING_SPINNER_HIDDEN, DialogEvent::DIALOG_OPENED,
-       dialog_event});
+       DialogEvent::PROCESSING_SPINNER_HIDDEN,
+       DialogEvent::BACK_TO_PAYMENT_SHEET_NAVIGATION, dialog_event});
 
   ASSERT_TRUE(content::ExecuteScript(GetActiveWebContents(),
                                      "retry(" + validation_errors + ");"));

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

@@ -209,9 +209,11 @@ class PaymentRequestBrowserTestBase
   void PayWithCreditCardAndWait(const base::string16& cvc,
                                 PaymentRequestDialogView* dialog_view);
   void PayWithCreditCard(const base::string16& cvc);
-  void RetryPaymentRequest(const std::string& validation_errors);
   void RetryPaymentRequest(const std::string& validation_errors,
-                           const DialogEvent& dialog_event);
+                           PaymentRequestDialogView* dialog_view);
+  void RetryPaymentRequest(const std::string& validation_errors,
+                           const DialogEvent& dialog_event,
+                           PaymentRequestDialogView* dialog_view);
 
   // Getting/setting the |value| in the textfield of a given |type|.
   base::string16 GetEditorTextfieldValue(autofill::ServerFieldType type);

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

@@ -189,7 +189,7 @@ void PaymentRequestDialogView::ShowPaymentHandlerScreen(
 
 void PaymentRequestDialogView::RetryDialog() {
   HideProcessingSpinner();
-  ShowInitialPaymentSheet();
+  GoBackToPaymentSheet(false /* animate */);
 
   if (request_->spec()->has_shipping_address_error()) {
     autofill::AutofillProfile* profile =
@@ -265,11 +265,11 @@ void PaymentRequestDialogView::GoBack() {
     observer_for_testing_->OnBackNavigation();
 }
 
-void PaymentRequestDialogView::GoBackToPaymentSheet() {
+void PaymentRequestDialogView::GoBackToPaymentSheet(bool animate) {
   // This assumes that the Payment Sheet is the first view in the stack. Thus if
   // there is only one view, we are already showing the payment sheet.
   if (view_stack_->size() > 1)
-    view_stack_->PopMany(view_stack_->size() - 1);
+    view_stack_->PopMany(view_stack_->size() - 1, animate);
 
   if (observer_for_testing_)
     observer_for_testing_->OnBackToPaymentSheetNavigation();

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

@@ -125,7 +125,7 @@ class PaymentRequestDialogView : public views::DialogDelegateView,
 
   void Pay();
   void GoBack();
-  void GoBackToPaymentSheet();
+  void GoBackToPaymentSheet(bool animate = true);
   void ShowContactProfileSheet();
   void ShowOrderSummary();
   void ShowShippingProfileSheet();

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

@@ -1286,7 +1286,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestShippingAddressEditorTest,
       "    city: 'CITY ERROR'"
       "  }"
       "}",
-      DialogEvent::SHIPPING_ADDRESS_EDITOR_OPENED);
+      DialogEvent::SHIPPING_ADDRESS_EDITOR_OPENED, dialog_view());
 
   EXPECT_EQ(base::ASCIIToUTF16("ADDRESS LINE ERROR"),
             GetErrorLabelForType(autofill::ADDRESS_HOME_STREET_ADDRESS));
@@ -1322,7 +1322,7 @@ IN_PROC_BROWSER_TEST_F(
       "    city: 'CITY ERROR'"
       "  }"
       "}",
-      DialogEvent::SHIPPING_ADDRESS_EDITOR_OPENED);
+      DialogEvent::SHIPPING_ADDRESS_EDITOR_OPENED, dialog_view());
 
   EXPECT_EQ(base::ASCIIToUTF16("ADDRESS LINE ERROR"),
             GetErrorLabelForType(autofill::ADDRESS_HOME_STREET_ADDRESS));
@@ -1349,7 +1349,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestShippingAddressEditorTest,
       "    addressLine: 'ADDRESS LINE ERROR',"
       "    city: 'CITY ERROR'"
       "  }"
-      "}");
+      "}",
+      dialog_view());
 
   const int kErrorLabelOffset =
       static_cast<int>(DialogViewID::ERROR_LABEL_OFFSET);

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

@@ -60,21 +60,23 @@ void ViewStack::Push(std::unique_ptr<views::View> view, bool animate) {
   }
 }
 
-void ViewStack::Pop() {
+void ViewStack::Pop(bool animate) {
   DCHECK_LT(1u, size());  // There must be at least one view left after popping.
 
-  gfx::Rect destination = bounds();
-  destination.set_origin(gfx::Point(width(), 0));
-
   // Set the second-to-last view as visible, since it is about to be revealed
   // when the last view animates out.
   stack_[size() - 2]->SetVisible(true);
 
-  slide_out_animator_->AnimateViewTo(
-      stack_.back().get(), destination);
+  if (animate) {
+    gfx::Rect destination = bounds();
+    destination.set_origin(gfx::Point(width(), 0));
+    slide_out_animator_->AnimateViewTo(stack_.back().get(), destination);
+  } else {
+    stack_.pop_back();
+  }
 }
 
-void ViewStack::PopMany(int n) {
+void ViewStack::PopMany(int n, bool animate) {
   DCHECK_LT(static_cast<size_t>(n), size());  // The stack can never be empty.
 
   size_t pre_size = stack_.size();
@@ -83,7 +85,7 @@ void ViewStack::PopMany(int n) {
   stack_.erase(stack_.end() - n, stack_.end() - 1);
   DCHECK_EQ(pre_size - n + 1, stack_.size());
 
-  Pop();
+  Pop(animate);
 }
 
 size_t ViewStack::size() const {

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

@@ -33,11 +33,15 @@ class ViewStack : public views::BoundsAnimatorObserver,
 
   // Removes a view from the stack, animates it out of view, and makes sure
   // it's properly deleted after the animation.
-  void Pop();
+  // If |animate| is false, the view will simply be gone to the hierarchy
+  // without the sliding animation.
+  void Pop(bool animate = true);
 
   // Removes |n| views from the stack but only animates the topmost one. The end
   // result is an animation from the top-most view to the destination view.
-  void PopMany(int n);
+  // If |animate| is false, the view will simply be gone to the hierarchy
+  // without the sliding animation.
+  void PopMany(int n, bool animate = true);
 
   size_t size() const;