fix: Handle configuration changes during WebAuth flow to prevent memory leak#941
Conversation
…ry leak and lost results - Add LifecycleAwareCallback to null the user callback on onDestroy, eliminating the Activity memory leak during rotation - Cache results arriving after destroy in AtomicReference (pendingLoginResult / pendingLogoutResult) for recovery via consumePendingLoginResult / consumePendingLogoutResult in onResume - Revert OAuthManager and LogoutManager to hold a strong callback reference (leak prevention is now LifecycleAwareCallback's responsibility) - Add 19 unit tests covering config-change caching, double-consume protection, observer lifecycle, and stale-result clearing
…nfiguration-changes-correctly-on-device-rotation-and-causes-a-memory-leak
| * @param onDetached called when a result arrives but the callback is already detached | ||
| */ | ||
| internal class LifecycleAwareCallback<S>( | ||
| @Volatile private var inner: Callback<S, AuthenticationException>?, |
There was a problem hiding this comment.
- AuthenticationException can be non nullable
- inner feels too vague. Rename this to something like delegateCallback, wrappedCallback ,targetCallback etc
- Do we need this to be @volatile ?
| * the original callback was no longer reachable (e.g. Activity destroyed | ||
| * during a configuration change). | ||
| */ | ||
| internal sealed class PendingResult<out S, out E> { |
There was a problem hiding this comment.
This can be
internal sealed class PendingResult<out S> {
data class Success<S>(val result: S) : PendingResult<S>()
data class Failure(val error: AuthenticationException) : PendingResult<Nothing>()
}
as E will always be AuthenticationException . No need to define a generic parameter for that
|
|
||
| @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) | ||
| internal val pendingLoginResult = | ||
| AtomicReference<PendingResult<Credentials, AuthenticationException>?>(null) |
There was a problem hiding this comment.
AuthenticationException>? can be non-nullable . Ok with the above suggested change this might not be required at all
|
|
||
| @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) | ||
| internal val pendingLogoutResult = | ||
| AtomicReference<PendingResult<Void?, AuthenticationException>?>(null) |
There was a problem hiding this comment.
Do these two properties needs to be annotated with @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) ? They need to be private properties and needs to be tested via the public methods, in this case consumePendingLoginResult and consumePendingLogoutResult . Refactr the existing tests so that we test them via public methods
| * @return true if a pending result was found and delivered, false otherwise | ||
| */ | ||
| @JvmStatic | ||
| public fun consumePendingLoginResult(callback: Callback<Credentials, AuthenticationException>): Boolean { |
There was a problem hiding this comment.
Can we rename these two methods to something like resumePendingLoginResult resumePendingLogoutResult
| private const val KEY_BUNDLE_OAUTH_MANAGER_STATE = "oauth_manager_state" | ||
|
|
||
| private val callbacks = CopyOnWriteArraySet<Callback<Credentials, AuthenticationException>>() | ||
| internal val callbacks = CopyOnWriteArraySet<Callback<Credentials, AuthenticationException>>() |
There was a problem hiding this comment.
Why is this internal now?
There was a problem hiding this comment.
callback was registered after registerCallbacks() or removed after onDestroy). Since internal is module-scoped, it's not visible to SDK consumers.
| data class Failure(val error: AuthenticationException) : PendingResult<Nothing>() | ||
| } | ||
|
|
||
| internal val pendingLoginResult = AtomicReference<PendingResult<Credentials>?>(null) |
| loginCallback: Callback<Credentials, AuthenticationException>, | ||
| logoutCallback: Callback<Void?, AuthenticationException>? = null, | ||
| ) { | ||
| // Process-death recovery: register and auto-remove on destroy |
There was a problem hiding this comment.
This will attach callbacks on each onResume call in the the client application.
Ideally this method should be invoked in the onCreate to avoid duplicate addition and we are clearing all references in the onDestroy. We should listen to the onResume callback and have the pendingLoginResult code triggered from there
| * @param logoutCallback receives logout results recovered after a configuration change | ||
| */ | ||
| @JvmStatic | ||
| public fun attach( |
There was a problem hiding this comment.
can we rename this to something more intuituev
There was a problem hiding this comment.
remaned registerCallbacks()
| startInternal(context, effectiveCallback) | ||
| } | ||
|
|
||
| internal fun startInternal(context: Context, callback: Callback<Void?, AuthenticationException>) { |
…ults on onResume, and make LogoutBuilder.startInternal private
| startInternal(context, effectiveCallback) | ||
| } | ||
|
|
||
| internal fun startInternal( |
…y, add @volatile to delegateCallback, make login startInternal private
…s on new flow start, make pending fields internal for direct test access
| val resultMissing = authenticationIntent.data == null | ||
| if (resultMissing) { | ||
| setResult(RESULT_CANCELED) | ||
| if (!resultMissing) { |
There was a problem hiding this comment.
How will this change behave when user back presses the browser ? With this change, the AuthenticationActivity stays in an invisible state doing nothing and user might get stuck
2 participants
Changes
When the Activity is destroyed during a WebAuth login or logout flow due to a configuration change (e.g. device rotation, locale change, dark mode toggle), the SDK previously retained a strong reference to the destroyed Activity via the static WebAuthProvider.managerInstance → OAuthManager/LogoutManager → callback chain, causing a memory leak. Additionally, the authentication result was delivered to the destroyed Activity's callback, which could crash or silently fail.
Root cause: WebAuthProvider.managerInstance is a static field. OAuthManager and LogoutManager held a strong reference to the callback, which captured the Activity/Fragment this. On configuration change, the old Activity was destroyed but never garbage collected because of this reference chain.
What changed:
LifecycleAwareCallback (new): A DefaultLifecycleObserver wrapper around the user's callback. It observes the host Activity/Fragment lifecycle and sets delegateCallback = null immediately and deterministically when onDestroy fires — breaking the memory leak chain without relying on GC timing. If a result arrives after onDestroy, it is routed to the onDetached lambda instead of the dead callback.
WebAuthProvider: login().start() and logout().start() now wrap the callback in LifecycleAwareCallback when the context is a LifecycleOwner. Added the following new public API:
LogoutBuilder.startInternal changed from internal to private.
V4_MIGRATION_GUIDE.md: Updated with accurate description and usage example for registerCallbacks().
Usage:
The suspend fun await() API is unaffected since it does not go through LifecycleAwareCallback.
Testing
Unit tests added (19 new tests in WebAuthProviderTest.kt):
All existing unit tests pass without modification.
Manual testing performed on device (Android 14):
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors