-
Notifications
You must be signed in to change notification settings - Fork 169
fix: Handle configuration changes during WebAuth flow to prevent memory leak #941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
b6cacb3
8058fe8
8795d30
c0831d1
61e92fe
6ed2d1f
6b7675c
ad0df64
ec02d07
0886953
c84ed7a
196eca1
d063324
ebfed96
43d43ca
5eba7ac
b01245f
21a8532
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| package com.auth0.android.provider | ||
|
|
||
| import androidx.lifecycle.DefaultLifecycleObserver | ||
| import androidx.lifecycle.LifecycleOwner | ||
| import com.auth0.android.authentication.AuthenticationException | ||
| import com.auth0.android.callback.Callback | ||
|
|
||
| /** | ||
| * Wraps a user-provided callback and observes the Activity/Fragment lifecycle. | ||
| * When the host is destroyed (e.g. config change), [inner] is set to null so | ||
| * the destroyed Activity is no longer referenced by the SDK. | ||
| * | ||
| * If a result arrives after [inner] has been cleared, the [onDetached] lambda | ||
| * is invoked to cache the result for later recovery via consumePending*Result(). | ||
| * | ||
| * @param S the success type (Credentials for login, Void? for logout) | ||
| * @param inner the user's original callback | ||
| * @param lifecycleOwner the Activity or Fragment whose lifecycle to observe | ||
| * @param onDetached called when a result arrives but the callback is already detached | ||
| */ | ||
| internal class LifecycleAwareCallback<S>( | ||
| @Volatile private var inner: Callback<S, AuthenticationException>?, | ||
| lifecycleOwner: LifecycleOwner, | ||
| private val onDetached: (success: S?, error: AuthenticationException?) -> Unit, | ||
| ) : Callback<S, AuthenticationException>, DefaultLifecycleObserver { | ||
|
|
||
| init { | ||
| lifecycleOwner.lifecycle.addObserver(this) | ||
| } | ||
|
|
||
| override fun onSuccess(result: S) { | ||
| val cb = inner | ||
| if (cb != null) { | ||
| cb.onSuccess(result) | ||
| } else { | ||
| onDetached(result, null) | ||
| } | ||
| } | ||
|
|
||
| override fun onFailure(error: AuthenticationException) { | ||
| val cb = inner | ||
| if (cb != null) { | ||
| cb.onFailure(error) | ||
| } else { | ||
| onDetached(null, error) | ||
| } | ||
| } | ||
|
|
||
| override fun onDestroy(owner: LifecycleOwner) { | ||
| inner = null | ||
| owner.lifecycle.removeObserver(this) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import android.net.Uri | |
| import android.os.Bundle | ||
| import android.util.Log | ||
| import androidx.annotation.VisibleForTesting | ||
| import androidx.lifecycle.LifecycleOwner | ||
| import com.auth0.android.Auth0 | ||
| import com.auth0.android.annotation.ExperimentalAuth0Api | ||
| import com.auth0.android.authentication.AuthenticationException | ||
|
|
@@ -18,6 +19,7 @@ import kotlinx.coroutines.suspendCancellableCoroutine | |
| import kotlinx.coroutines.withContext | ||
| import java.util.Locale | ||
| import java.util.concurrent.CopyOnWriteArraySet | ||
| import java.util.concurrent.atomic.AtomicReference | ||
| import kotlin.coroutines.CoroutineContext | ||
| import kotlin.coroutines.resume | ||
| import kotlin.coroutines.resumeWithException | ||
|
|
@@ -39,6 +41,62 @@ public object WebAuthProvider { | |
| internal var managerInstance: ResumableManager? = null | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add back the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| private set | ||
|
|
||
| /** | ||
| * Represents a pending authentication or logout result that arrived while | ||
| * the original callback was no longer reachable (e.g. Activity destroyed | ||
| * during a configuration change). | ||
| */ | ||
| internal sealed class PendingResult<out S, out E> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be as E will always be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| data class Success<S>(val result: S) : PendingResult<S, Nothing>() | ||
| data class Failure<E>(val error: E) : PendingResult<Nothing, E>() | ||
| } | ||
|
|
||
| @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) | ||
| internal val pendingLoginResult = | ||
| AtomicReference<PendingResult<Credentials, AuthenticationException>?>(null) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
| @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) | ||
| internal val pendingLogoutResult = | ||
| AtomicReference<PendingResult<Void?, AuthenticationException>?>(null) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these two properties needs to be annotated with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
| /** | ||
| * Check for and consume a pending login result that arrived during a configuration change. | ||
| * Call this in your Activity's `onResume()` to recover results that were delivered while the | ||
| * Activity was being recreated (e.g. due to screen rotation). | ||
| * | ||
| * @param callback the callback to deliver the pending result to | ||
| * @return true if a pending result was found and delivered, false otherwise | ||
| */ | ||
| @JvmStatic | ||
| public fun consumePendingLoginResult(callback: Callback<Credentials, AuthenticationException>): Boolean { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename these two methods to something like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| val result = pendingLoginResult.getAndSet(null) ?: return false | ||
| when (result) { | ||
| is PendingResult.Success -> callback.onSuccess(result.result) | ||
| is PendingResult.Failure -> callback.onFailure(result.error) | ||
| } | ||
| resetManagerInstance() | ||
| return true | ||
| } | ||
|
|
||
| /** | ||
| * Check for and consume a pending logout result that arrived during a configuration change. | ||
| * Call this in your Activity's `onResume()` to recover results that were delivered while the | ||
| * Activity was being recreated (e.g. due to screen rotation). | ||
| * | ||
| * @param callback the callback to deliver the pending result to | ||
| * @return true if a pending result was found and delivered, false otherwise | ||
| */ | ||
| @JvmStatic | ||
| public fun consumePendingLogoutResult(callback: Callback<Void?, AuthenticationException>): Boolean { | ||
| val result = pendingLogoutResult.getAndSet(null) ?: return false | ||
| when (result) { | ||
| is PendingResult.Success -> callback.onSuccess(result.result) | ||
| is PendingResult.Failure -> callback.onFailure(result.error) | ||
| } | ||
| resetManagerInstance() | ||
| return true | ||
| } | ||
|
|
||
| @JvmStatic | ||
| public fun addCallback(callback: Callback<Credentials, AuthenticationException>) { | ||
| callbacks += callback | ||
|
|
@@ -242,6 +300,27 @@ public object WebAuthProvider { | |
| * @see AuthenticationException.isAuthenticationCanceled | ||
| */ | ||
| public fun start(context: Context, callback: Callback<Void?, AuthenticationException>) { | ||
| pendingLogoutResult.set(null) | ||
|
|
||
| val effectiveCallback = if (context is LifecycleOwner) { | ||
| LifecycleAwareCallback<Void?>( | ||
| inner = callback, | ||
| lifecycleOwner = context as LifecycleOwner, | ||
| onDetached = { _: Void?, error: AuthenticationException? -> | ||
| if (error != null) { | ||
| pendingLogoutResult.set(PendingResult.Failure(error)) | ||
| } else { | ||
| pendingLogoutResult.set(PendingResult.Success(null)) | ||
| } | ||
| } | ||
| ) | ||
| } else { | ||
| callback | ||
| } | ||
| startInternal(context, effectiveCallback) | ||
| } | ||
|
|
||
| internal fun startInternal(context: Context, callback: Callback<Void?, AuthenticationException>) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be private
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| resetManagerInstance() | ||
| if (!ctOptions.hasCompatibleBrowser(context.packageManager)) { | ||
| val ex = AuthenticationException( | ||
|
|
@@ -286,7 +365,7 @@ public object WebAuthProvider { | |
| ) { | ||
| return withContext(coroutineContext) { | ||
| suspendCancellableCoroutine { continuation -> | ||
| start(context, object : Callback<Void?, AuthenticationException> { | ||
| startInternal(context, object : Callback<Void?, AuthenticationException> { | ||
| override fun onSuccess(result: Void?) { | ||
| continuation.resume(Unit) | ||
| } | ||
|
|
@@ -592,6 +671,29 @@ public object WebAuthProvider { | |
| public fun start( | ||
| context: Context, | ||
| callback: Callback<Credentials, AuthenticationException> | ||
| ) { | ||
| pendingLoginResult.set(null) | ||
| val effectiveCallback = if (context is LifecycleOwner) { | ||
| LifecycleAwareCallback<Credentials>( | ||
| inner = callback, | ||
| lifecycleOwner = context as LifecycleOwner, | ||
| onDetached = { success: Credentials?, error: AuthenticationException? -> | ||
| if (success != null) { | ||
| pendingLoginResult.set(PendingResult.Success(success)) | ||
| } else if (error != null) { | ||
| pendingLoginResult.set(PendingResult.Failure(error)) | ||
| } | ||
| } | ||
| ) | ||
| } else { | ||
| callback | ||
| } | ||
| startInternal(context, effectiveCallback) | ||
| } | ||
|
|
||
| internal fun startInternal( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also be private
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| context: Context, | ||
| callback: Callback<Credentials, AuthenticationException> | ||
| ) { | ||
| resetManagerInstance() | ||
| if (!ctOptions.hasCompatibleBrowser(context.packageManager)) { | ||
|
|
@@ -665,7 +767,9 @@ public object WebAuthProvider { | |
| ): Credentials { | ||
| return withContext(coroutineContext) { | ||
| suspendCancellableCoroutine { continuation -> | ||
| start(context, object : Callback<Credentials, AuthenticationException> { | ||
| // Use startInternal directly — the anonymous callback captures only the | ||
| // coroutine continuation, not an Activity, so lifecycle wrapping is not needed | ||
| startInternal(context, object : Callback<Credentials, AuthenticationException> { | ||
| override fun onSuccess(result: Credentials) { | ||
| continuation.resume(result) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done