Calculate skip UI and build UI after payment instruments are ready.

This cl cleans up the order in which we calculate whether or not should
skip UI, building UI, and showing it on Android.
Without this change it is possible that ShouldSkipUI is true but we
still show the payment sheet UI.

After this change we build the UI and calculate ShouldSkipUI after both
of the following conditions are met:
1-request.show is called.
2-All instruments are ready.

To properly handle concurrent request.show attempts, both setting and
reading "sShowingPaymentRequest" should happen in .show().

Bug: 984694
Change-Id: I7ccae4b1346c5206184217f7c721bcc9db9b3df5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1877047Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711692}

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

@@ -202,6 +202,12 @@ public class PaymentRequestImpl
          * the browser UI has closed.
          */
         void onCompleteReplied();
+
+        /**
+         * Called when the renderer is closing the mojo connection (e.g. upon show promise
+         * rejection).
+         */
+        void onRendererClosedMojoConnection();
     }
 
     /**
@@ -454,6 +460,12 @@ public class PaymentRequestImpl
     /** A helper to manage the Skip-to-GPay experimental flow. */
     private SkipToGPayHelper mSkipToGPayHelper;
 
+    /**
+     * When true skip UI is available for non-url based payment method identifiers (e.g.
+     * basic-card).
+     */
+    private boolean mSkipUiForNonUrlPaymentMethodIdentifiers;
+
     /**
      * Builds the PaymentRequest service implementation.
      *
@@ -492,6 +504,8 @@ public class PaymentRequestImpl
         mJourneyLogger = new JourneyLogger(mIsIncognito, mWebContents);
         mCurrencyFormatterMap = new HashMap<>();
 
+        mSkipUiForNonUrlPaymentMethodIdentifiers = mDelegate.skipUiForBasicCard();
+
         if (sObserverForTest != null) sObserverForTest.onPaymentRequestCreated(this);
     }
 
@@ -657,18 +671,21 @@ public class PaymentRequestImpl
         }
         mJourneyLogger.setRequestedPaymentMethodTypes(mMerchantSupportsAutofillPaymentInstruments,
                 requestedMethodGoogle, requestedMethodOther);
-        calculateWhetherShouldSkipShowingPaymentRequestUi(mDelegate.skipUiForBasicCard());
     }
 
     /**
      * Calculate whether the browser payment sheet should be skipped directly into the payment app.
-     *
-     * @param skipUiForNonUrlPaymentMethodIdentifiersForTest Whether non-URL payment method
-     *                                                       identifiers should skip UI. Used only
-     *                                                       in tests.
      */
-    private void calculateWhetherShouldSkipShowingPaymentRequestUi(
-            boolean skipUiForNonUrlPaymentMethodIdentifiersForTest) {
+    private void calculateWhetherShouldSkipShowingPaymentRequestUi() {
+        // This should be called after all payment instruments are ready and request.show() is
+        // called, since only then whether or not should skip payment sheet UI is determined.
+        assert isFinishedQueryingPaymentApps();
+        assert mIsCurrentPaymentRequestShowing;
+
+        assert mPaymentMethodsSection != null;
+        PaymentInstrument selectedInstrument =
+                (PaymentInstrument) mPaymentMethodsSection.getSelectedItem();
+
         // If there is a single payment method and the merchant has not requested any other
         // information, we can safely go directly to the payment app instead of showing
         // Payment Request UI.
@@ -682,7 +699,14 @@ public class PaymentRequestImpl
                 // the payment request UI, thus can't be skipped.
                 && mMethodData.keySet().iterator().next() != null
                 && (mMethodData.keySet().iterator().next().startsWith(UrlConstants.HTTPS_URL_PREFIX)
-                        || skipUiForNonUrlPaymentMethodIdentifiersForTest);
+                        || mSkipUiForNonUrlPaymentMethodIdentifiers)
+                // Skip to payment app only if it is the only available payment instrument, and it
+                // can be pre-selected.
+                && mPaymentMethodsSection.getSize() == 1
+                && selectedInstrument != null
+                // Skip to payment app only if user gesture is provided when it is required to
+                // skip-UI.
+                && (mIsUserGestureShow || !selectedInstrument.isUserGestureRequiredToSkipUi());
     }
 
     /** @return Whether the UI was built. */
@@ -725,7 +749,6 @@ public class PaymentRequestImpl
                     activity, mAutofillProfiles, mContactEditor, mJourneyLogger);
         }
 
-        setShowingPaymentRequest(this);
         mUI = new PaymentRequestUI(activity, this, mRequestShipping,
                 /* requestShippingOption= */ mRequestShipping,
                 mRequestPayerName || mRequestPayerPhone || mRequestPayerEmail,
@@ -846,6 +869,7 @@ public class PaymentRequestImpl
             return;
         }
 
+        setShowingPaymentRequest(this);
         mIsCurrentPaymentRequestShowing = true;
         mJourneyLogger.setTriggerTime();
         if (disconnectIfNoPaymentMethodsSupported()) return;
@@ -861,9 +885,14 @@ public class PaymentRequestImpl
         mIsUserGestureShow = isUserGesture;
         mWaitForUpdatedDetails = waitForUpdatedDetails;
 
-        if (!mShouldSkipShowingPaymentRequestUi && mSkipToGPayHelper == null) {
+        if (isFinishedQueryingPaymentApps()) {
+            // Calculate skip ui and build ui only after all payment instruments are ready and
+            // request.show() is called.
+            calculateWhetherShouldSkipShowingPaymentRequestUi();
             if (!buildUI(chromeActivity)) return;
-            mUI.show();
+            if (!mShouldSkipShowingPaymentRequestUi && mSkipToGPayHelper == null) {
+                mUI.show();
+            }
         }
 
         triggerPaymentAppUiSkipIfApplicable(chromeActivity);
@@ -891,39 +920,28 @@ public class PaymentRequestImpl
                 && isFinishedQueryingPaymentApps() && mIsCurrentPaymentRequestShowing
                 && !mWaitForUpdatedDetails) {
             assert !mPaymentMethodsSection.isEmpty();
+            assert mUI != null;
 
             if (isMicrotransactionUiApplicable()) {
                 triggerMicrotransactionUi(chromeActivity);
                 return;
             }
 
+            assert !mPaymentMethodsSection.isEmpty();
             PaymentInstrument selectedInstrument =
                     (PaymentInstrument) mPaymentMethodsSection.getSelectedItem();
-            if (!buildUI(chromeActivity)) return;
-
-            // Do not skip to payment app if it is not the only one, it's not pre-selected, or if
-            // skip-UI requires a user gesture in show(), which was not present.
-            // Note that ServiceWorkerPaymentApp cannot be pre-selected if its name and/or icon is
-            // missing.
-            if (mPaymentMethodsSection.getSize() > 1 || selectedInstrument == null
-                    || (selectedInstrument.isUserGestureRequiredToSkipUi()
-                            && !mIsUserGestureShow)) {
-                mUI.show();
-            } else {
-                dimBackgroundIfNotBottomSheetPaymentHandler(selectedInstrument);
-                mDidRecordShowEvent = true;
-                mShouldRecordAbortReason = true;
-                mJourneyLogger.setEventOccurred(Event.SKIPPED_SHOW);
-                assert mRawTotal != null;
-                // The total amount in details should be finalized at this point. So it is safe to
-                // record the triggered transaction amount.
-                assert !mWaitForUpdatedDetails;
-                mJourneyLogger.recordTransactionAmount(
-                        mRawTotal.amount.currency, mRawTotal.amount.value, false /*completed*/);
-
-                onPayClicked(null /* selectedShippingAddress */, null /* selectedShippingOption */,
-                        selectedInstrument);
-            }
+            dimBackgroundIfNotBottomSheetPaymentHandler(selectedInstrument);
+            mDidRecordShowEvent = true;
+            mShouldRecordAbortReason = true;
+            mJourneyLogger.setEventOccurred(Event.SKIPPED_SHOW);
+            assert mRawTotal != null;
+            // The total amount in details should be finalized at this point. So it is safe to
+            // record the triggered transaction amount.
+            assert !mWaitForUpdatedDetails;
+            mJourneyLogger.recordTransactionAmount(
+                    mRawTotal.amount.currency, mRawTotal.amount.value, false /*completed*/);
+            onPayClicked(null /* selectedShippingAddress */, null /* selectedShippingOption */,
+                    selectedInstrument);
         }
     }
 
@@ -953,7 +971,6 @@ public class PaymentRequestImpl
                     mCurrencyFormatterMap.get(mRawTotal.amount.currency),
                     mUiShoppingCart.getTotal(), this::onMicrotransactionUiConfirmed,
                     this::onMicrotransactionUiDismissed)) {
-            setShowingPaymentRequest(this);
             mDidRecordShowEvent = true;
             mShouldRecordAbortReason = true;
             mJourneyLogger.setEventOccurred(Event.SHOWN);
@@ -1274,10 +1291,10 @@ public class PaymentRequestImpl
             return;
         }
 
-        if (mRequestShipping) createShippingSection(chromeActivity, mAutofillProfiles);
-
-        if (!mShouldSkipShowingPaymentRequestUi) {
-            enableUserInterfaceAfterPaymentRequestUpdateEvent();
+        // Do not create shipping section When UI is not built yet. This happens when the show
+        // promise gets resolved before all instruments are ready.
+        if (mUI != null && mRequestShipping) {
+            createShippingSection(chromeActivity, mAutofillProfiles);
         }
 
         mWaitForUpdatedDetails = false;
@@ -1294,6 +1311,10 @@ public class PaymentRequestImpl
         }
 
         triggerPaymentAppUiSkipIfApplicable(chromeActivity);
+
+        if (isFinishedQueryingPaymentApps() && !mShouldSkipShowingPaymentRequestUi) {
+            enableUserInterfaceAfterPaymentRequestUpdateEvent();
+        }
     }
 
     /**
@@ -1561,7 +1582,12 @@ public class PaymentRequestImpl
     public void getDefaultPaymentInformation(Callback<PaymentInformation> callback) {
         mPaymentInformationCallback = callback;
 
-        if (mPaymentMethodsSection == null || mWaitForUpdatedDetails) return;
+        // mUI.show() is called only after request.show() is called and all payment instruments are
+        // ready.
+        assert mIsCurrentPaymentRequestShowing;
+        assert isFinishedQueryingPaymentApps();
+
+        if (mWaitForUpdatedDetails) return;
 
         mHandler.post(() -> {
             if (mUI != null) providePaymentInformation();
@@ -2233,6 +2259,7 @@ public class PaymentRequestImpl
         if (mClient == null) return;
         closeClient();
         mJourneyLogger.setAborted(AbortReason.MOJO_RENDERER_CLOSING);
+        if (sObserverForTest != null) sObserverForTest.onRendererClosedMojoConnection();
         closeUIAndDestroyNativeObjects(/*immediateClose=*/true);
         if (mNativeObserverForTest != null) mNativeObserverForTest.onConnectionTerminated();
     }
@@ -2383,13 +2410,18 @@ public class PaymentRequestImpl
 
         updateInstrumentModifiedTotals();
 
-        // UI has requested the full list of payment instruments. Provide it now.
-        if (mPaymentInformationCallback != null && !mWaitForUpdatedDetails) {
-            providePaymentInformation();
-        }
-
         SettingsAutofillAndPaymentsObserver.getInstance().registerObserver(this);
 
+        if (mIsCurrentPaymentRequestShowing) {
+            // Calculate skip ui and build ui only after all payment instruments are ready and
+            // request.show() is called.
+            calculateWhetherShouldSkipShowingPaymentRequestUi();
+            if (!buildUI(chromeActivity)) return;
+            if (!mShouldSkipShowingPaymentRequestUi && mSkipToGPayHelper == null) {
+                mUI.show();
+            }
+        }
+
         triggerPaymentAppUiSkipIfApplicable(chromeActivity);
     }
 
@@ -2618,7 +2650,6 @@ public class PaymentRequestImpl
         if (mMicrotransactionUi != null) {
             mMicrotransactionUi.hide();
             mMicrotransactionUi = null;
-            setShowingPaymentRequest(null);
         }
 
         if (mUI != null) {
@@ -2630,9 +2661,9 @@ public class PaymentRequestImpl
                 closeClient();
             });
             mUI = null;
-            setShowingPaymentRequest(null);
         }
 
+        setShowingPaymentRequest(null);
         mIsCurrentPaymentRequestShowing = false;
 
         if (mPaymentMethodsSection != null) {
@@ -2708,8 +2739,7 @@ public class PaymentRequestImpl
 
     @VisibleForTesting
     /* package */ void setSkipUIForNonURLPaymentMethodIdentifiersForTest() {
-        calculateWhetherShouldSkipShowingPaymentRequestUi(
-                true /* skipUiForNonUrlPaymentMethodIdentifiersForTest */);
+        mSkipUiForNonUrlPaymentMethodIdentifiers = true;
     }
 
     /**

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsTest.java

@@ -352,8 +352,8 @@ public class PaymentRequestMetricsTest implements MainActivityStartCallback {
                         "PaymentRequest.TransactionAmount.Completed"));
 
         // Make sure the events were logged correctly.
-        int expectedSample = Event.HAD_NECESSARY_COMPLETE_SUGGESTIONS | Event.REQUEST_SHIPPING
-                | Event.REQUEST_METHOD_GOOGLE | Event.COULD_NOT_SHOW;
+        int expectedSample =
+                Event.REQUEST_SHIPPING | Event.REQUEST_METHOD_GOOGLE | Event.COULD_NOT_SHOW;
         Assert.assertEquals(1,
                 RecordHistogram.getHistogramValueCountForTesting(
                         "PaymentRequest.Events", expectedSample));
@@ -393,8 +393,8 @@ public class PaymentRequestMetricsTest implements MainActivityStartCallback {
                         "PaymentRequest.TransactionAmount.Completed"));
 
         // Make sure the events were logged correctly.
-        int expectedSample = Event.HAD_NECESSARY_COMPLETE_SUGGESTIONS | Event.REQUEST_SHIPPING
-                | Event.REQUEST_METHOD_OTHER | Event.COULD_NOT_SHOW;
+        int expectedSample =
+                Event.REQUEST_SHIPPING | Event.REQUEST_METHOD_OTHER | Event.COULD_NOT_SHOW;
         Assert.assertEquals(1,
                 RecordHistogram.getHistogramValueCountForTesting(
                         "PaymentRequest.Events", expectedSample));

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShowPromiseInvalidDetailsTest.java

@@ -41,7 +41,7 @@ public class PaymentRequestShowPromiseInvalidDetailsTest implements MainActivity
     @MediumTest
     @Feature({"Payments"})
     public void testReject() throws TimeoutException {
-        mRule.openPageAndClickNodeAndWait("buy", mRule.getDismissed());
+        mRule.openPageAndClickNodeAndWait("buy", mRule.getRendererClosedMojoConnection());
         mRule.expectResultContains(new String[] {"Total amount value should be non-negative"});
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShowPromiseRejectTest.java

@@ -43,7 +43,7 @@ public class PaymentRequestShowPromiseRejectTest implements MainActivityStartCal
     public void testReject() throws TimeoutException {
         mRule.installPaymentApp("basic-card", PaymentRequestTestRule.HAVE_INSTRUMENTS,
                 PaymentRequestTestRule.IMMEDIATE_RESPONSE);
-        mRule.openPageAndClickNodeAndWait("buy", mRule.getDismissed());
+        mRule.openPageAndClickNodeAndWait("buy", mRule.getRendererClosedMojoConnection());
         mRule.expectResultContains(new String[] {"AbortError"});
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShowPromiseUnsupportedTest.java

@@ -41,7 +41,7 @@ public class PaymentRequestShowPromiseUnsupportedTest implements MainActivitySta
     @MediumTest
     @Feature({"Payments"})
     public void testReject() throws TimeoutException {
-        mRule.openPageAndClickNodeAndWait("buy", mRule.getDismissed());
+        mRule.openPageAndClickNodeAndWait("buy", mRule.getShowFailed());
         mRule.expectResultContains(
                 new String[] {"NotSupportedError: Payment method not supported"});
     }

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestRule.java

@@ -118,6 +118,7 @@ public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
     final CallbackHelper mExpirationMonthChange;
     final CallbackHelper mPaymentResponseReady;
     final CallbackHelper mCompleteReplied;
+    final CallbackHelper mRendererClosedMojoConnection;
     PaymentRequestImpl mPaymentRequest;
     PaymentRequestUI mUI;
 
@@ -158,6 +159,7 @@ public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
         mCanMakePaymentQueryResponded = new CallbackHelper();
         mHasEnrolledInstrumentQueryResponded = new CallbackHelper();
         mCompleteReplied = new CallbackHelper();
+        mRendererClosedMojoConnection = new CallbackHelper();
         mWebContentsRef = new AtomicReference<>();
         mTestFilePath = testFileName.equals("about:blank") || testFileName.startsWith("data:")
                 ? testFileName
@@ -247,6 +249,9 @@ public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
     public CallbackHelper getCompleteReplied() {
         return mCompleteReplied;
     }
+    public CallbackHelper getRendererClosedMojoConnection() {
+        return mRendererClosedMojoConnection;
+    }
     public PaymentRequestUI getPaymentRequestUI() {
         return mUI;
     }
@@ -1090,6 +1095,12 @@ public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
         mCompleteReplied.notifyCalled();
     }
 
+    @Override
+    public void onRendererClosedMojoConnection() {
+        ThreadUtils.assertOnUiThread();
+        mRendererClosedMojoConnection.notifyCalled();
+    }
+
     /**
      * Listens for UI notifications.
      */