material-foundation / material-remixer-android

Remixer for Android. Live adjustment of app variables.
Apache License 2.0
774 stars 54 forks source link

Ensure not leaking activities through variables. #25

Closed miguelandres closed 8 years ago

miguelandres commented 8 years ago

The way that we're thinking about Remixer objects, there is a Singleton that will persist across activities. This singleton will contain all Variables for any activity.

Since variables contain the callback and (most likely) these callbacks contain references to the activity (by virtue of being anonymous classes, or actually explicitly storing the activity) we would leak the activity.

I am actively thinking of ways to minimize this risk or make sure that we remove all remixes for inactive activities, that may mean adding requirements to call something during OnDestroy, or even consider making the only supported way....

being an activity lifecycle listener https://developer.android.com/reference/android/app/Application.ActivityLifecycleCallbacks.html could help

miguelandres commented 8 years ago

I have a plan now.

  1. We make Remixer a singleton
  2. We add a parameter to the Remix/Variable constructor that is the activity where it's being created. This will be saved as a weak reference so it doesn't leak. It also solves the problem of knowing which remixes correspond to which activity.
  3. We make Remixer an Application.ActivityLifecycleCallbacks and register it, clearing the callbacks once activities are destroyed. 3.1. We may even be able to run RemixerBinders on create without user intervention? Not sure that's the best idea ever, but it may be worth taking a look at. 3.2. This still needs some thought since this cannot actually be the remixer object because it's in remixer_core which can't depend on Android. We may need a proxy in remixer_ui.
  4. once the callbacks are cleared there should be no way to leak the activity. The Remix/Variable objects (which are significantly small) are kept, but those are necessary to perform saves.
miguelandres commented 8 years ago

This is done as of #29