PermissionManager's PermissionsResultAction List needs to WeakReference its elements
diegotori opened this issue · 4 comments
Right now, as it is written, your current PermissionsResultAction list is creating strong references to said elements. This creates issues in that when creating an anonymous callback from an Activity, it implicitly creates a reference to that Activity (according to your example). As a result, it prevents it from garbage collection, until it is eventually removed from that list.
This problem is further compounded if one were to change the device's orientation multiple times, creating multiple adds from previous Activity instances, thus keeping strong refs to them, and thus leaking memory heavily.
My suggestion would be to wrap each PermissionsResultAction element in your List as a WeakReference:
private final List<WeakReference<PermissionsResultAction>> mPendingActions = new ArrayList<>(1);Then, when adding new actions, you would simply do the following:
mPendingActions.add(new WeakReference<PermissionsResultAction>(action));And finally, when calling back to them, you would simply check if there's currently a valid reference to them:
while (iterator.hasNext()) {
final WeakReference<PermissionsResultAction> callbackRef = iterator.next();
final PermissionsResultAction callback= callbackRef.get();
if (callback != null && callback.onResult(permissions[n], results[n])) {
iterator.remove();
}
mPendingRequests.remove(permissions[n]);
}As a result, you create a clean break from your observers that may otherwise not exist when it comes time to call them, and enable their references to get GC'd quickly.
Note that iterator block was my best guess in handling when to remove that Iterator's element given this new model. You may have to polish it slightly to determine when it gets removed given the valid state of a callback reference.
I've been thinking of using WeakReferences as a way to prevent this potential memory leak, what you have is great, thank you. The only problem would be that since there is no hard reference to the PermissionsResultAction, if garbage collection were to occur between the the initial request and the response, then the action would be lost to garbage collection. If we could force the enclosing Activity/Class to have a hard reference to any PermissionsResultAction it passes, that would be ideal as then once the object becomes eligible for garbage collection then so would the PermissionsResultAction, but no sooner. I'm not sure whether this is possible though.
An alternative I've been thinking about would be to use a Map of Activity/Fragments to the PermissionsResultAction(s) that they request. Then there would be a notifyDestroy(Activity/Fragment) method that would be called from the onDestroy of the requesting Activity/Fragment that would remove any references that still exist in the list. Further, if a WeakHashMap<Activity, List<PermissionsResultAction> is used, then this would ensure that if the enclosing class is not an Activity but is just some generic class, that the PermissionsResultActions and enclosing class will be able to be garbage collected when the Activity/Fragment is destroyed even if notifyDestroy was not called. Using a WeakHashMap seems optional though if the user is expected to properly notify the PermissionsManager in onDestroy.
So I see two options:
- Ensures that the enclosing instance of the calling class does not leak without any change from the user of the library. Risks losing an action if the system gc's the reference between request and callback.
- Require the user to notify the PermissionsManager of the onDestroy event. Risks leaking references if the user forgets to notify the PermissionsManager.
thoughts on if the second option would be any good?
Looking at ActivityCompat.java and Activity.java from both support 23.0 and Marshmallow respectively, onRequestPermissionsResult() gets called similarly to onActivityResult() in the same method that dispatches the Activity's result. In the support library, it calls it from the Main handler, which only runs posted Runnables right after onStart(), since by that point, the UI is about to display in onResume().
With that said, if someone were to always register a weak-ref'd callback in onCreate(), then it'll be valid by the time the callback is invoked through the PermissionManager after an orientation change (i.e. activity ref -> weak-ref'd callback in onCreate -> onStart -> onResume -> config change calling onPause, onStop, and onDestroy and thus freeing the callback to GC -> new activity ref -> new weak-ref'd callback in onCreate -> onStart -> onRequestPermissionsResult with a valid ref, onResume).
In other words, we shouldn't concern ourselves with a dead Activity reference that contained an anon callback. We should instead only callback to what can be called back. The first option is always the path of least resistance because it always assumes the worst that a dev can do, which in this case, acts as an insurance policy against memory leaks caused by devs copy-pasting the snippet and nothing else.
Those are good points, I'm going to switch to using WeakReferences instead of hard references.