[PRImpl] Move WebContents and JourneyLogger into CPRImpl

Change:
* Add null-check for
PRImpl's "WebContentsStatics.fromRenderFrameHost(renderFrameHost)".
And move it to CPRImpl.
* JourneyLogger's instantiation is moved from PRImpl's construction to
before CPRImpl's construction. Moving it earlier is because
JourneyLogger's role includes logging the error before CPRImpl's
construction.
* BrowserPaymentRequest's creation is no longer done in CPRImpl's
construction. This is because BrowserPaymentRequest's constructor
calls CPRImpl which is still partially initiated.
* The null-checks of client, methodData, details are moved from
CPRImpl#initAndValidate() to CPRImpl#createIfParamsValid(), because we
don't bother to create CPRImpl if render data is invalid.

Acronym:
CPRImpl = ComponentPaymentRequestImpl
PRImpl = PaymentRequestImpl

Bug: 1102522

Change-Id: I0faf70fe6a73b13858b30d1dadb30cf25857eda2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2357090
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798876}

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

@@ -199,8 +199,8 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
         return ComponentPaymentRequestImpl.createPaymentRequest(mRenderFrameHost,
                 /*isOffTheRecord=*/delegate.isOffTheRecord(webContents),
                 /*skipUiForBasicCard=*/delegate.skipUiForBasicCard(),
-                (renderFrameHost, componentPaymentRequest, isOffTheRecord, journeyLogger)
+                (renderFrameHost, componentPaymentRequest, isOffTheRecord)
                         -> new PaymentRequestImpl(renderFrameHost, componentPaymentRequest,
-                                isOffTheRecord, journeyLogger, delegate));
+                                isOffTheRecord, delegate));
     }
 }

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

@@ -63,7 +63,6 @@ import org.chromium.components.payments.Event;
 import org.chromium.components.payments.JourneyLogger;
 import org.chromium.components.payments.MethodStrings;
 import org.chromium.components.payments.NotShownReason;
-import org.chromium.components.payments.OriginSecurityChecker;
 import org.chromium.components.payments.PackageManagerDelegate;
 import org.chromium.components.payments.PayerData;
 import org.chromium.components.payments.PaymentApp;
@@ -83,7 +82,6 @@ import org.chromium.components.security_state.SecurityStateModel;
 import org.chromium.components.url_formatter.UrlFormatter;
 import org.chromium.content_public.browser.RenderFrameHost;
 import org.chromium.content_public.browser.WebContents;
-import org.chromium.content_public.browser.WebContentsStatics;
 import org.chromium.payments.mojom.CanMakePaymentQueryResult;
 import org.chromium.payments.mojom.HasEnrolledInstrumentQueryResult;
 import org.chromium.payments.mojom.PayerDetail;
@@ -340,20 +338,18 @@ public class PaymentRequestImpl
      * @param renderFrameHost The host of the frame that has invoked the PaymentRequest API.
      * @param componentPaymentRequestImpl The component side of the PaymentRequest implementation.
      * @param isOffTheRecord Whether the merchant page is shown in an off-the-record tab.
-     * @param journeyLogger The JourneyLogger that records the user journey of using the
-     *         PaymentRequest service.
      * @param delegate The delegate of this class.
      */
     public PaymentRequestImpl(RenderFrameHost renderFrameHost,
             ComponentPaymentRequestImpl componentPaymentRequestImpl, boolean isOffTheRecord,
-            JourneyLogger journeyLogger, Delegate delegate) {
+            Delegate delegate) {
         assert renderFrameHost != null;
         assert componentPaymentRequestImpl != null;
         assert delegate != null;
 
         mRenderFrameHost = renderFrameHost;
         mDelegate = delegate;
-        mWebContents = WebContentsStatics.fromRenderFrameHost(renderFrameHost);
+        mWebContents = componentPaymentRequestImpl.getWebContents();
         mPaymentRequestOrigin =
                 UrlFormatter.formatUrlForSecurityDisplay(mRenderFrameHost.getLastCommittedURL());
         mPaymentRequestSecurityOrigin = mRenderFrameHost.getLastCommittedOrigin();
@@ -362,7 +358,7 @@ public class PaymentRequestImpl
         mMerchantName = mWebContents.getTitle();
         mCertificateChain = CertificateChainHelper.getCertificateChain(mWebContents);
         mIsOffTheRecord = isOffTheRecord;
-        mJourneyLogger = journeyLogger;
+        mJourneyLogger = componentPaymentRequestImpl.getJourneyLogger();
         mPaymentUIsManager = new PaymentUIsManager(/*delegate=*/this,
                 /*params=*/this, mWebContents, mIsOffTheRecord, mJourneyLogger);
         mComponentPaymentRequestImpl = componentPaymentRequestImpl;
@@ -376,12 +372,6 @@ public class PaymentRequestImpl
         assert mComponentPaymentRequestImpl != null;
         mMethodData = new HashMap<>();
 
-        if (!OriginSecurityChecker.isOriginSecure(mWebContents.getLastCommittedUrl())) {
-            mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
-            disconnectFromClientWithDebugMessage(ErrorStrings.NOT_IN_A_SECURE_ORIGIN);
-            return false;
-        }
-
         mPaymentOptions = options;
         mRequestShipping = options != null && options.requestShipping;
         mRequestPayerName = options != null && options.requestPayerName;
@@ -1456,11 +1446,6 @@ public class PaymentRequestImpl
         return mPaymentHandlerHost;
     }
 
-    @Override
-    public JourneyLogger getJourneyLogger() {
-        return mJourneyLogger;
-    }
-
     @Override
     public void onDismiss() {
         mJourneyLogger.setAborted(AbortReason.ABORTED_BY_USER);

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

@@ -27,12 +27,10 @@ public interface BrowserPaymentRequest {
          *         the BrowserPaymentRequest instance.
          * @param isOffTheRecord Whether the merchant page is in an OffTheRecord (e.g., incognito,
          *         guest mode) Tab.
-         * @param journeyLogger The logger that records the user journey of PaymentRequest.
          * @return An instance of BrowserPaymentRequest, cannot be null.
          */
         BrowserPaymentRequest createBrowserPaymentRequest(RenderFrameHost renderFrameHost,
-                ComponentPaymentRequestImpl componentPaymentRequestImpl, boolean isOffTheRecord,
-                JourneyLogger journeyLogger);
+                ComponentPaymentRequestImpl componentPaymentRequestImpl, boolean isOffTheRecord);
     }
 
     /**
@@ -90,9 +88,6 @@ public interface BrowserPaymentRequest {
     /** The browser part of the {@link PaymentRequest#canMakePayment} implementation. */
     void canMakePayment();
 
-    /** @return The JourneyLogger of PaymentRequestImpl. */
-    JourneyLogger getJourneyLogger();
-
     /** Delegate to the same method of PaymentRequestImpl. */
     void disconnectFromClientWithDebugMessage(String debugMessage);
 

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

@@ -7,8 +7,10 @@ package org.chromium.components.payments;
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 
+import org.chromium.base.Log;
 import org.chromium.components.autofill.EditableOption;
 import org.chromium.content_public.browser.RenderFrameHost;
+import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.WebContentsStatics;
 import org.chromium.mojo.system.MojoException;
 import org.chromium.payments.mojom.PayerDetail;
@@ -36,19 +38,20 @@ import java.util.List;
  * {@link ComponentPaymentRequestImpl#close()}, after which no usage is allowed.
  */
 public class ComponentPaymentRequestImpl {
+    private static final String TAG = "CompPaymentRequest";
     private static PaymentRequestServiceObserverForTest sObserverForTest;
     private static NativeObserverForTest sNativeObserverForTest;
     private final Runnable mOnClosedListener;
+    private final WebContents mWebContents;
+    private final JourneyLogger mJourneyLogger;
     private boolean mSkipUiForNonUrlPaymentMethodIdentifiers;
     private PaymentRequestLifecycleObserver mPaymentRequestLifecycleObserver;
     private boolean mHasClosed;
 
-    // After create(), mClient is null only when it has closed.
-    @Nullable
+    // mClient is null only when it has closed.
     private PaymentRequestClient mClient;
 
-    // After the constructor, mBrowserPaymentRequest is null only when it has closed.
-    @Nullable
+    // mBrowserPaymentRequest is null when it has closed or is uninitiated.
     private BrowserPaymentRequest mBrowserPaymentRequest;
 
     /**
@@ -173,11 +176,45 @@ public class ComponentPaymentRequestImpl {
         assert browserPaymentRequestFactory != null;
         assert onClosedListener != null;
 
-        ComponentPaymentRequestImpl instance = new ComponentPaymentRequestImpl(renderFrameHost,
-                isOffTheRecord, skipUiForBasicCard, browserPaymentRequestFactory, onClosedListener);
+        WebContents webContents = WebContentsStatics.fromRenderFrameHost(renderFrameHost);
+        if (webContents == null) {
+            abortBeforeInstantiation(client, /*journeyLogger=*/null, ErrorStrings.NO_WEB_CONTENTS,
+                    AbortReason.INVALID_DATA_FROM_RENDERER);
+            return null;
+        }
+
+        JourneyLogger journeyLogger = new JourneyLogger(isOffTheRecord, webContents);
+
+        if (client == null) {
+            abortBeforeInstantiation(/*client=*/null, journeyLogger, ErrorStrings.INVALID_STATE,
+                    AbortReason.INVALID_DATA_FROM_RENDERER);
+            return null;
+        }
+
+        if (!OriginSecurityChecker.isOriginSecure(webContents.getLastCommittedUrl())) {
+            abortBeforeInstantiation(client, journeyLogger, ErrorStrings.NOT_IN_A_SECURE_ORIGIN,
+                    AbortReason.INVALID_DATA_FROM_RENDERER);
+            return null;
+        }
+
+        if (methodData == null) {
+            abortBeforeInstantiation(client, journeyLogger,
+                    ErrorStrings.INVALID_PAYMENT_METHODS_OR_DATA,
+                    AbortReason.INVALID_DATA_FROM_RENDERER);
+            return null;
+        }
+
+        if (details == null) {
+            abortBeforeInstantiation(client, journeyLogger, ErrorStrings.INVALID_PAYMENT_DETAILS,
+                    AbortReason.INVALID_DATA_FROM_RENDERER);
+            return null;
+        }
+
+        ComponentPaymentRequestImpl instance = new ComponentPaymentRequestImpl(client,
+                renderFrameHost, webContents, journeyLogger, skipUiForBasicCard, onClosedListener);
         instance.onCreated();
-        boolean valid = instance.initAndValidate(
-                client, methodData, details, options, googlePayBridgeEligible);
+        boolean valid = instance.initAndValidate(renderFrameHost, browserPaymentRequestFactory,
+                methodData, details, options, googlePayBridgeEligible, isOffTheRecord);
         if (!valid) {
             instance.close();
             return null;
@@ -190,15 +227,27 @@ public class ComponentPaymentRequestImpl {
         sObserverForTest.onPaymentRequestCreated(this);
     }
 
-    private ComponentPaymentRequestImpl(RenderFrameHost renderFrameHost, boolean isOffTheRecord,
-            boolean skipUiForBasicCard, BrowserPaymentRequest.Factory browserPaymentRequestFactory,
-            Runnable onClosedListener) {
+    /** Abort the request, used before this class's instantiation. */
+    private static void abortBeforeInstantiation(@Nullable PaymentRequestClient client,
+            @Nullable JourneyLogger journeyLogger, String debugMessage, int reason) {
+        Log.d(TAG, debugMessage);
+        if (client != null) client.onError(reason, debugMessage);
+        if (journeyLogger != null) journeyLogger.setAborted(reason);
+        if (sNativeObserverForTest != null) sNativeObserverForTest.onConnectionTerminated();
+    }
+
+    private ComponentPaymentRequestImpl(PaymentRequestClient client,
+            RenderFrameHost renderFrameHost, WebContents webContents, JourneyLogger journeyLogger,
+            boolean skipUiForBasicCard, Runnable onClosedListener) {
+        assert client != null;
+        assert renderFrameHost != null;
+        assert webContents != null;
+        assert journeyLogger != null;
+
         mSkipUiForNonUrlPaymentMethodIdentifiers = skipUiForBasicCard;
-        JourneyLogger journeyLogger = new JourneyLogger(
-                isOffTheRecord, WebContentsStatics.fromRenderFrameHost(renderFrameHost));
-        mBrowserPaymentRequest = browserPaymentRequestFactory.createBrowserPaymentRequest(
-                renderFrameHost, this, isOffTheRecord, journeyLogger);
-        assert mBrowserPaymentRequest != null;
+        mClient = client;
+        mWebContents = webContents;
+        mJourneyLogger = journeyLogger;
         mOnClosedListener = onClosedListener;
         mHasClosed = false;
     }
@@ -219,24 +268,12 @@ public class ComponentPaymentRequestImpl {
         return sNativeObserverForTest;
     }
 
-    private boolean initAndValidate(@Nullable PaymentRequestClient client,
-            @Nullable PaymentMethodData[] methodData, @Nullable PaymentDetails details,
-            @Nullable PaymentOptions options, boolean googlePayBridgeEligible) {
-        if (client == null) {
-            abortForInvalidDataFromRenderer(ErrorStrings.INVALID_STATE);
-            return false;
-        }
-        mClient = client;
-        if (methodData == null) {
-            abortForInvalidDataFromRenderer(ErrorStrings.INVALID_PAYMENT_METHODS_OR_DATA);
-            return false;
-        }
-        if (details == null) {
-            abortForInvalidDataFromRenderer(ErrorStrings.INVALID_PAYMENT_DETAILS);
-            return false;
-        }
-
-        assert mBrowserPaymentRequest != null;
+    private boolean initAndValidate(RenderFrameHost renderFrameHost,
+            BrowserPaymentRequest.Factory factory, @Nullable PaymentMethodData[] methodData,
+            @Nullable PaymentDetails details, @Nullable PaymentOptions options,
+            boolean googlePayBridgeEligible, boolean isOffTheRecord) {
+        mBrowserPaymentRequest =
+                factory.createBrowserPaymentRequest(renderFrameHost, this, isOffTheRecord);
         return mBrowserPaymentRequest.initAndValidate(
                 methodData, details, options, googlePayBridgeEligible);
     }
@@ -327,7 +364,7 @@ public class ComponentPaymentRequestImpl {
         // Every caller should stop referencing this class once close() is called.
         assert mBrowserPaymentRequest != null;
 
-        mBrowserPaymentRequest.getJourneyLogger().setAborted(AbortReason.MOJO_RENDERER_CLOSING);
+        mJourneyLogger.setAborted(AbortReason.MOJO_RENDERER_CLOSING);
         close();
         if (sObserverForTest != null) {
             sObserverForTest.onRendererClosedMojoConnection();
@@ -346,7 +383,7 @@ public class ComponentPaymentRequestImpl {
         // Every caller should stop referencing this class once close() is called.
         assert mBrowserPaymentRequest != null;
 
-        mBrowserPaymentRequest.getJourneyLogger().setAborted(AbortReason.MOJO_CONNECTION_ERROR);
+        mJourneyLogger.setAborted(AbortReason.MOJO_CONNECTION_ERROR);
         close();
         if (sNativeObserverForTest != null) {
             sNativeObserverForTest.onConnectionTerminated();
@@ -361,8 +398,7 @@ public class ComponentPaymentRequestImpl {
         // Every caller should stop referencing this class once close() is called.
         assert mBrowserPaymentRequest != null;
 
-        mBrowserPaymentRequest.getJourneyLogger().setAborted(
-                AbortReason.INVALID_DATA_FROM_RENDERER);
+        mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
         mBrowserPaymentRequest.disconnectFromClientWithDebugMessage(debugMessage);
     }
 
@@ -516,4 +552,17 @@ public class ComponentPaymentRequestImpl {
 
         mClient.warnNoFavicon();
     }
+
+    /**
+     * @return The logger of the user journey of the Android PaymentRequest service, cannot be
+     *         null.
+     */
+    public JourneyLogger getJourneyLogger() {
+        return mJourneyLogger;
+    }
+
+    /** @return The WebContents of the merchant's page, cannot be null. */
+    public WebContents getWebContents() {
+        return mWebContents;
+    }
 }

components/payments/core/error_strings.cc

@@ -24,6 +24,7 @@ const char kMethodNameRequired[] = "Method name required.";
 const char kMissingDetailsFromPaymentApp[] = "Payment app returned invalid response. Missing field \"details\".";
 const char kMissingMethodNameFromPaymentApp[] = "Payment app returned invalid response. Missing field \"methodName\".";
 const char kNotInASecureOrigin[] = "Not in a secure origin.";
+const char kNoWebContents[] = "The frame that initiated payment is not associated with any web page.";
 const char kPayerEmailEmpty[] = "Payment app returned invalid response. Missing field \"payerEmail\".";
 const char kPayerNameEmpty[] = "Payment app returned invalid response. Missing field \"payerName\".";
 const char kPayerPhoneEmpty[] = "Payment app returned invalid response. Missing field \"payerPhone\".";

components/payments/core/error_strings.h

@@ -58,6 +58,9 @@ extern const char kMissingMethodNameFromPaymentApp[];
 // The PaymentRequest API is available only on secure origins.
 extern const char kNotInASecureOrigin[];
 
+// WebContents is not available from RenderFrameHost.
+extern const char kNoWebContents[];
+
 // The payment handler responded with an empty "payer name" field.
 extern const char kPayerNameEmpty[];