Implement IsReadyToPay handling

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

Index: chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
index b9c40ded9b8ee5e88e283281fc072f93c802fef0..5e63a84cae9e6cd9c83443300e46ad8c4808ffca 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
@@ -5,15 +5,23 @@
 package org.chromium.chrome.browser.payments;
 
 import android.app.Activity;
+import android.content.ComponentName;
+import android.content.Context;
 import android.content.Intent;
+import android.content.ServiceConnection;
 import android.graphics.drawable.Drawable;
 import android.os.Bundle;
 import android.os.Handler;
+import android.os.IBinder;
+import android.os.RemoteException;
 import android.util.JsonWriter;
 
+import org.chromium.IsReadyToPayService;
+import org.chromium.IsReadyToPayServiceCallback;
 import org.chromium.chrome.R;
 import org.chromium.content.browser.ContentViewCore;
 import org.chromium.content_public.browser.WebContents;
+

[please use gerrit instead, 2017/01/09 19:02:14]

No need for newline here.

[rwlbuis, 2017/01/10 16:40:00]

Done.

 import org.chromium.payments.mojom.PaymentItem;
 import org.chromium.payments.mojom.PaymentMethodData;
 import org.chromium.ui.base.WindowAndroid;
@@ -35,13 +43,16 @@ public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
     private static final String EXTRA_ORIGIN = "origin";
     private static final String EXTRA_DETAILS = "details";
     private static final String EXTRA_INSTRUMENT_DETAILS = "instrumentDetails";
+    private static final String READY_TO_PAY = "readyToPay";

[please use gerrit instead, 2017/01/09 19:02:14]

Unused.

[rwlbuis, 2017/01/10 16:40:00]

Done.

     private static final String EMPTY_JSON_DATA = "{}";
 
     private final Handler mHandler;
     private final WebContents mWebContents;
+    private final Intent mIsReadyToPayIntent;
     private final Intent mPayIntent;
     private final Set<String> mMethodNames;
-    private String mIsReadyToPayService;
+    private IsReadyToPayService mIsReadyToPayService;
+    private InstrumentsCallback mInstrumentsCallback;
     private InstrumentDetailsCallback mInstrumentDetailsCallback;
 
     /**
@@ -60,6 +71,8 @@ public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
         mHandler = new Handler();
         mWebContents = webContents;
         mPayIntent = new Intent();
+        mIsReadyToPayIntent = new Intent();
+        mIsReadyToPayIntent.setPackage(packageName);
         mPayIntent.setClassName(packageName, activity);
         mMethodNames = new HashSet<>();
     }
@@ -70,19 +83,22 @@ public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
     }
 
     /** @param service The name of the "is ready to pay" service in the payment app. */

[please use gerrit instead, 2017/01/09 19:02:14]

Update JavaDoc.

[rwlbuis, 2017/01/10 16:40:00]

Done.

-    public void setIsReadyToPayService(String service) {
-        mIsReadyToPayService = service;
+    public void setIsReadyToPayAction(String className) {
+        mIsReadyToPayIntent.setClassName(mIsReadyToPayIntent.getPackage(), className);
     }
 
     @Override
-    public void getInstruments(Map<String, PaymentMethodData> methodData, String origin,
+    public void getInstruments(final Map<String, PaymentMethodData> methodData, final String origin,
             final InstrumentsCallback callback) {
+        mInstrumentsCallback = callback;
         mHandler.post(new Runnable() {

[please use gerrit instead, 2017/01/09 19:02:14]

Use the handler only to make sendInstrumentsReady() asynchronous. Intents are
already asynchronous, so there's no need for the handler in that case.

[rwlbuis, 2017/01/10 16:40:00]

Done.

[please use gerrit instead, 2017/01/10 16:49:22]

You seem to have forgotten to update the code.

[rwlbuis, 2017/01/10 19:31:05]

Oops! Done.

             @Override
             public void run() {
-                List<PaymentInstrument> instruments = new ArrayList<>();
-                instruments.add(AndroidPaymentApp.this);
-                callback.onInstrumentsReady(AndroidPaymentApp.this, instruments);
+                if (mIsReadyToPayIntent.getComponent() != null) {
+                    isReadyToPay(origin, methodData.get(mMethodNames.iterator().next()));
+                } else {
+                    sendInstrumentsReady(AndroidPaymentApp.this);
+                }
             }
         });
     }
@@ -95,6 +111,81 @@ public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
         return !methodNames.isEmpty();
     }
 
+    private void sendInstrumentsReady(PaymentInstrument instrument) {
+        List<PaymentInstrument> instruments = null;
+        if (instrument != null) {
+            instruments = new ArrayList<>();
+            instruments.add(instrument);
+        }
+        mInstrumentsCallback.onInstrumentsReady(AndroidPaymentApp.this, instruments);

[please use gerrit instead, 2017/01/09 19:02:14]

|this| is sufficient here, because you're not in an inner class.

[rwlbuis, 2017/01/10 16:40:00]

Done.

+    }
+
+    private void isReadyToPay(String origin, PaymentMethodData data) {
+        Bundle extras = new Bundle();
+        extras.putString(EXTRA_METHOD_NAME, mMethodNames.iterator().next());
+        extras.putString(EXTRA_ORIGIN, origin);
+        extras.putString(EXTRA_DATA, data == null ? EMPTY_JSON_DATA : data.stringifiedData);
+        mIsReadyToPayIntent.putExtras(extras);
+
+        if (mIsReadyToPayService != null) {
+            sendIsReadyToPay();
+        } else {
+            try {
+                ContentViewCore contentView = ContentViewCore.fromWebContents(mWebContents);
+                if (contentView == null) {
+                    notifyError();
+                    return;
+                }
+
+                WindowAndroid window = contentView.getWindowAndroid();
+                if (window == null) {
+                    notifyError();
+                    return;
+                }
+
+                window.getApplicationContext().bindService(
+                        mIsReadyToPayIntent, mServiceConnection, Context.BIND_AUTO_CREATE);
+            } catch (SecurityException e) {

[please use gerrit instead, 2017/01/09 19:02:14]

Only bindService() can throw a SecurityException. Let's not check
fromWebContents() and getWindowAndroid() for SecurityException as well.

[rwlbuis, 2017/01/10 16:40:00]

Done.

+                sendInstrumentsReady(null);
+            }
+        }
+    }
+
+    private final ServiceConnection mServiceConnection = new ServiceConnection() {

[please use gerrit instead, 2017/01/09 19:02:14]

Variables should be above methods.

[rwlbuis, 2017/01/10 16:40:00]

Done.

+        @Override
+        public void onServiceConnected(ComponentName name, IBinder service) {
+            mIsReadyToPayService = IsReadyToPayService.Stub.asInterface(service);
+            if (mIsReadyToPayService == null) {
+                sendInstrumentsReady(null);
+            } else {
+                sendIsReadyToPay();
+            }
+        }
+
+        @Override
+        public void onServiceDisconnected(ComponentName name) {
+            sendInstrumentsReady(null);
+        }
+    };
+    private void sendIsReadyToPay() {
+        assert mIsReadyToPayService != null;
+        final IsReadyToPayServiceCallback.Stub callback = new IsReadyToPayServiceCallback.Stub() {

[please use gerrit instead, 2017/01/09 19:02:14]

The "final" keyword seems unnecessary.

[rwlbuis, 2017/01/10 16:40:00]

Done.

+            @Override
+            public void handleIsReadyToPay(boolean isReadyToPay) throws RemoteException {
+                if (isReadyToPay) {
+                    sendInstrumentsReady(AndroidPaymentApp.this);
+                } else {
+                    sendInstrumentsReady(null);
+                }
+            }
+        };
+        try {
+            mIsReadyToPayService.isReadyToPay(callback);
+        } catch (Exception e) {

[please use gerrit instead, 2017/01/09 19:02:14]

We prefer explicit exception types.

[rwlbuis, 2017/01/10 16:40:00]

This is actually an important part of this patch. It is possible for the Service
to misbehave (I caused an explicit null pointer crash in BobPay test app) and
the browser will crash:
1-10 11:22:32.831  3792  3792 D AndroidRuntime: Shutting down VM^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime: FATAL EXCEPTION: main^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime: Process: org.chromium.chrome,
PID: 3792^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime: java.lang.NullPointerException:
Attempt to invoke interface method 'void
org.chromium.IsReadyToPayServiceCallback.handleIsReadyToPay(boolean)' on a null
object reference^M 
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
android.os.Parcel.readException(Parcel.java:1626)^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
android.os.Parcel.readException(Parcel.java:1573)^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
org.chromium.IsReadyToPayService$Stub$Proxy.isReadyToPay(IsReadyToPayService.java:90)^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
org.chromium.chrome.browser.payments.AndroidPaymentApp.sendIsReadyToPay(AndroidPaymentApp.java:182)^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
org.chromium.chrome.browser.payments.AndroidPaymentApp.access$200(AndroidPaymentApp.java:39)^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
org.chromium.chrome.browser.payments.AndroidPaymentApp$1.onServiceConnected(AndroidPaymentApp.java:64)^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
android.app.LoadedApk$ServiceDispatcher.doConnected(LoadedApk.java:1335)^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
android.app.LoadedApk$ServiceDispatcher$RunConnection.run(LoadedApk.java:1352)^M
01-10 11:22:32.831  3792  3792 E AndroidRuntime:        at
android.os.Handler.handleCallback(Handler.java:739)^M

I think there are some things to consider. We could assume:
1) pay Services that go through stores will not make such mistakes
2) pay Services will catch any Exceptions they cause (but how to enforce?)

I am quite sure there can be many more potential exceptions besides null pointer
exceptions, so spelling them out may be impractical. Or should these Exceptions
be caught at a higher level?

[please use gerrit instead, 2017/01/10 16:49:22]

Please make sure in your patch that the payment app is not able to crash the
browser by checking every piece of data that you receive from it for validity
before processing it.
We cannot assume that the payment app will behave sanely. In fact, we should
suspect malice. Therefore, validate all your inputs.
Does Chrome rethrow exceptions from the payment app?
Exceptions should be handled at the lowest possible level, as a rule of thumb.

[rwlbuis, 2017/01/10 19:31:05]

For reference I found a description of the exception problems I saw while
developing, here:
https://blog.classycode.com/dealing-with-exceptions-in-aidl-9ba904c6d63#.qwqv...
I am not sure there are inputs in this case except the boolean? Let me know if I
can check more. I agree that payment app could be malicious or do unexpected
things like have incompatible AIDL files.
See the link above. *some* exceptions are passed on from he stub to the caller.
Not sure what Chrome ends up doing but ultimately it will crash the client i.e.
browser exactly as described in that link.

In my last patch I added catching of RuntimeExceptions which would cover all 7
exceptions 
mentioned in the article. I verified that this catches my explicit null pointer
crash in my BobPay test app.

+            sendInstrumentsReady(null);
+        }
+    }
+
     @Override
     public String getAppIdentifier() {
         return getIdentifier();