Implement IsReadyToPay handling

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

 // Copyright 2017 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.
 
 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;
 
 import java.io.IOException;
 import java.io.StringWriter;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
 /** The point of interaction with a locally installed 3rd party native Android payment app. */
 public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
         WindowAndroid.IntentCallback {
     private static final String EXTRA_METHOD_NAME = "methodName";
     private static final String EXTRA_DATA = "data";
     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;
 
     /**
      * Builds the point of interaction with a locally installed 3rd party native Android payment
      * app.
      *
      * @param webContents The web contents.
      * @param packageName The name of the package of the payment app.
      * @param activity    The name of the payment activity in the payment app.
      * @param label       The UI label to use for the payment app.
      * @param icon        The icon to use in UI for the payment app.
      */
     public AndroidPaymentApp(WebContents webContents, String packageName, String activity,
             String label, Drawable icon) {
         super(packageName, label, null, icon);
         mHandler = new Handler();
         mWebContents = webContents;
         mPayIntent = new Intent();
+        mIsReadyToPayIntent = new Intent();
+        mIsReadyToPayIntent.setPackage(packageName);
         mPayIntent.setClassName(packageName, activity);
         mMethodNames = new HashSet<>();
     }
 
     /** @param methodName A payment method that this app supports, e.g., "https://bobpay.com". */
     public void addMethodName(String methodName) {
         mMethodNames.add(methodName);
     }
 
     /** @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);
+                }
             }
         });
     }
 
     @Override
     public boolean supportsMethodsAndData(Map<String, PaymentMethodData> methodsAndData) {
         assert methodsAndData != null;
         Set<String> methodNames = new HashSet<>(methodsAndData.keySet());
         methodNames.retainAll(getAppMethodNames());
         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();
     }
 
     @Override
     public Set<String> getAppMethodNames() {
         return Collections.unmodifiableSet(mMethodNames);
     }
 
@@ skipping 105 matching lines @@
             mInstrumentDetailsCallback.onInstrumentDetailsReady(
                     data.getExtras().getString(EXTRA_METHOD_NAME),
                     data.getExtras().getString(EXTRA_INSTRUMENT_DETAILS));
         }
         mInstrumentDetailsCallback = null;
     }
 
     @Override
     public void dismissInstrument() {}
 }