From 437f55b441c3eac25129fac90aec14dd326875b8 Mon Sep 17 00:00:00 2001 From: Michael Shafrir Date: Thu, 11 Apr 2019 10:22:50 -0400 Subject: [PATCH] Use strong references to callbacks in Stripe and CustomerSession **Summary** `Stripe` and `CustomerSession` methods accept callback objects that typically hold references to `Activity` objects. The best practice in Android is that references to `Activity` objects should be weakly held for long-running background tasks, because the `Activity` may go away (e.g. user leaves activity before task is complete). The change was first introduced in #666. After this change, users began reporting that callback objects would mysteriously be garbage-collected by the OS. I was able to reproduce this as well, and found a work-around by moving anonymous inline classes to instance variables - see 2b74328. Android will GC objects that are no longer strongly referenced. When inlined, the callback objects were no longer strongly referenced once the method completed (e.g. in 2b74328, the left-side of `AddSourceActivity#onActionSave`); moving these objects to be instance variables meant that they were now strongly-referenced by the Activity. This explains why the GC issues were resolved with this change. In conclusion, the correct solution is to have the callback objects weakly-reference `Activity` objects, rather than weakly-reference the callback objects themselves. **Motivation** ANDROID-340 --- .../com/stripe/android/CustomerSession.java | 41 ++++++++----------- .../main/java/com/stripe/android/Stripe.java | 38 ++++++++--------- 2 files changed, 33 insertions(+), 46 deletions(-) diff --git a/stripe/src/main/java/com/stripe/android/CustomerSession.java b/stripe/src/main/java/com/stripe/android/CustomerSession.java index 482a05421f4..076b77b3f9d 100644 --- a/stripe/src/main/java/com/stripe/android/CustomerSession.java +++ b/stripe/src/main/java/com/stripe/android/CustomerSession.java @@ -17,8 +17,6 @@ import com.stripe.android.model.ShippingInformation; import com.stripe.android.model.Source; -import java.lang.ref.Reference; -import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Arrays; import java.util.Calendar; @@ -79,10 +77,10 @@ public class CustomerSession @Nullable private Customer mCustomer; private long mCustomerCacheTime; @Nullable private Context mContext; - @NonNull private final Map> - mCustomerRetrievalListenerRefs = new HashMap<>(); - @NonNull private final Map> - mSourceRetrievalListenerRefs = new HashMap<>(); + @NonNull private final Map mCustomerRetrievalListeners = + new HashMap<>(); + @NonNull private final Map mSourceRetrievalListeners = + new HashMap<>(); @NonNull private final EphemeralKeyManager mEphemeralKeyManager; @NonNull private final Handler mUiThreadHandler; @@ -134,8 +132,8 @@ public static void endCustomerSession() { @VisibleForTesting static void clearInstance() { if (mInstance != null) { - mInstance.mCustomerRetrievalListenerRefs.clear(); - mInstance.mSourceRetrievalListenerRefs.clear(); + mInstance.mCustomerRetrievalListeners.clear(); + mInstance.mSourceRetrievalListeners.clear(); } cancelCallbacks(); setInstance(null); @@ -202,7 +200,7 @@ public void retrieveCurrentCustomer(@NonNull CustomerRetrievalListener listener) mCustomer = null; final String operationId = UUID.randomUUID().toString(); - mCustomerRetrievalListenerRefs.put(operationId, new WeakReference<>(listener)); + mCustomerRetrievalListeners.put(operationId, listener); mEphemeralKeyManager.retrieveEphemeralKey(operationId, null, null); } } @@ -217,7 +215,7 @@ public void updateCurrentCustomer(@NonNull CustomerRetrievalListener listener) { mCustomer = null; final String operationId = UUID.randomUUID().toString(); - mCustomerRetrievalListenerRefs.put(operationId, new WeakReference<>(listener)); + mCustomerRetrievalListeners.put(operationId, listener); mEphemeralKeyManager.retrieveEphemeralKey(operationId, null, null); } @@ -256,7 +254,7 @@ public void addCustomerSource( final String operationId = UUID.randomUUID().toString(); if (listener != null) { - mSourceRetrievalListenerRefs.put(operationId, new WeakReference<>(listener)); + mSourceRetrievalListeners.put(operationId, listener); } mEphemeralKeyManager.retrieveEphemeralKey(operationId, ACTION_ADD_SOURCE, arguments); } @@ -278,7 +276,7 @@ public void deleteCustomerSource( final String operationId = UUID.randomUUID().toString(); if (listener != null) { - mSourceRetrievalListenerRefs.put(operationId, new WeakReference<>(listener)); + mSourceRetrievalListeners.put(operationId, listener); } mEphemeralKeyManager.retrieveEphemeralKey(operationId, ACTION_DELETE_SOURCE, arguments); } @@ -319,7 +317,7 @@ public void setCustomerDefaultSource( final String operationId = UUID.randomUUID().toString(); if (listener != null) { - mCustomerRetrievalListenerRefs.put(operationId, new WeakReference<>(listener)); + mCustomerRetrievalListeners.put(operationId, listener); } mEphemeralKeyManager.retrieveEphemeralKey(operationId, ACTION_SET_DEFAULT_SOURCE, arguments); @@ -629,16 +627,15 @@ public void handleMessage(@NonNull Message msg) { private void handleRetrievalError(@Nullable String operationId, @NonNull StripeException exception, int errorType) { - final WeakReference listenerRef; + final RetrievalListener listener; if (errorType == SOURCE_ERROR) { - listenerRef = mSourceRetrievalListenerRefs.remove(operationId); + listener = mSourceRetrievalListeners.remove(operationId); } else if (errorType == CUSTOMER_ERROR) { - listenerRef = mCustomerRetrievalListenerRefs.remove(operationId); + listener = mCustomerRetrievalListeners.remove(operationId); } else { - listenerRef = null; + listener = null; } - final RetrievalListener listener = listenerRef != null ? listenerRef.get() : null; if (listener != null) { final int errorCode = exception.getStatusCode() == null ? 400 @@ -758,16 +755,12 @@ private void sendErrorIntent(@NonNull StripeException exception) { @Nullable private CustomerRetrievalListener getCustomerRetrievalListener(@Nullable String operationId) { - final Reference listenerRef = - mCustomerRetrievalListenerRefs.remove(operationId); - return listenerRef != null ? listenerRef.get() : null; + return mCustomerRetrievalListeners.remove(operationId); } @Nullable private SourceRetrievalListener getSourceRetrievalListener(@Nullable String operationId) { - final Reference listenerRef = - mSourceRetrievalListenerRefs.remove(operationId); - return listenerRef != null ? listenerRef.get() : null; + return mSourceRetrievalListeners.remove(operationId); } public interface CustomerRetrievalListener extends RetrievalListener { diff --git a/stripe/src/main/java/com/stripe/android/Stripe.java b/stripe/src/main/java/com/stripe/android/Stripe.java index 8d540f51358..1d3dc24ab2c 100644 --- a/stripe/src/main/java/com/stripe/android/Stripe.java +++ b/stripe/src/main/java/com/stripe/android/Stripe.java @@ -947,7 +947,7 @@ private static class CreateSourceTask extends AsyncTask mSourceCallbackRef; + @NonNull private final SourceCallback mSourceCallback; CreateSourceTask(@NonNull Context context, @NonNull StripeApiHandler apiHandler, @@ -960,7 +960,7 @@ private static class CreateSourceTask extends AsyncTask(sourceCallback); + mSourceCallback = sourceCallback; } @Override @@ -981,13 +981,10 @@ protected ResponseWrapper doInBackground(Void... params) { @Override protected void onPostExecute(@NonNull ResponseWrapper responseWrapper) { - final SourceCallback sourceCallback = mSourceCallbackRef.get(); - if (sourceCallback != null) { - if (responseWrapper.source != null) { - sourceCallback.onSuccess(responseWrapper.source); - } else if (responseWrapper.error != null) { - sourceCallback.onError(responseWrapper.error); - } + if (responseWrapper.source != null) { + mSourceCallback.onSuccess(responseWrapper.source); + } else if (responseWrapper.error != null) { + mSourceCallback.onError(responseWrapper.error); } } } @@ -999,7 +996,7 @@ private static class CreateTokenTask extends AsyncTask mCallbackRef; + @NonNull private final TokenCallback mCallback; @Nullable private final StripeApiHandler.LoggingResponseListener mLoggingResponseListener; CreateTokenTask(@NonNull Context context, @@ -1008,7 +1005,7 @@ private static class CreateTokenTask extends AsyncTask(context); mApiHandler = apiHandler; @@ -1017,7 +1014,7 @@ private static class CreateTokenTask extends AsyncTask(callback); + mCallback = callback; } @Override @@ -1043,16 +1040,13 @@ protected void onPostExecute(@NonNull ResponseWrapper result) { } private void tokenTaskPostExecution(@NonNull ResponseWrapper result) { - final TokenCallback callback = mCallbackRef.get(); - if (callback != null) { - if (result.token != null) { - callback.onSuccess(result.token); - } else if (result.error != null) { - callback.onError(result.error); - } else { - callback.onError(new RuntimeException("Somehow got neither a token response or" - + " an error response")); - } + if (result.token != null) { + mCallback.onSuccess(result.token); + } else if (result.error != null) { + mCallback.onError(result.error); + } else { + mCallback.onError(new RuntimeException( + "Somehow got neither a token response or an error response")); } } }