Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
### Behavioral Changes

- Nav3: Returning to an Activity or Fragment with its own `SentryNavEffect` within the app (e.g., closing the Activity that covered it, swiping back to its ViewPager2 page, or dismissing a dialog over it) now starts a new navigation transaction for the current destination if the previous one finished, so spans produced upon return can now be consistently captured ([#6482](https://github.com/getsentry/sentry-java/pull/6482))
- Nav3: `ui.load` transactions are bound to `IScope` in preference to Nav3 transactions when transitioning between Activities, matching Nav2 behavior (API 29+) ([#6484](https://github.com/getsentry/sentry-java/pull/6484))

### Fixes

Expand Down
2 changes: 2 additions & 0 deletions sentry-android-navigation3/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ dependencies {
compileOnly(libs.androidx.compose.runtime)
compileOnly(libs.androidx.compose.ui)

testImplementation(projects.sentryAndroidCore)
testImplementation(projects.sentryTestSupport)
testImplementation(libs.androidx.compose.ui.test.junit4)
testImplementation(libs.androidx.fragment.ktx)
testImplementation(libs.androidx.test.core)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,59 @@ package io.sentry.compose.navigation3.lifecycle
import android.app.Activity
import android.app.Application
import android.app.Application.ActivityLifecycleCallbacks
import android.os.Build
import android.os.Bundle

/**
* Watches every Activity of the host application and reports the Activity lifecycle events that
* [NavLeaseCoordinator] acts on.
* [NavLeaseCoordinator] acts on, so that the coordinator needn't know how they differ across API
* levels.
*
* - [onActivityResumed] is called when any Activity resumes, including Activities without a
* `SentryNavEffect`.
* Each event is reported relative to `ActivityLifecycleIntegration`, which binds an Activity's
* `ui.load` transaction to the scope when the Activity is created and removes it when the Activity
* is destroyed:
*
* - [onActivityCreatedOrResumed] is called when an Activity is created and again when it resumes,
* with [ActivityEvent.CREATED] or [ActivityEvent.RESUMED] as its event. On API 29+, creation is
* reported before `ActivityLifecycleIntegration` tries to bind the Activity's `ui.load`
* transaction; below API 29, after. So below API 29, a withdrawal on creation comes too late for
* that `ui.load` transaction, which stays unbound (see "Non-candidate roots and lease withdrawal"
* in the package README).
*
* - [onActivityDestroyed] is called when an Activity is destroyed, after
* `ActivityLifecycleIntegration` has removed the Activity's `ui.load` transaction from the scope.
*
* **Android ordering assumptions**
*
* The watcher relies on the following callback orders, which Android doesn't document but AOSP has
* historically upheld:
*
* - `Application` calls `ActivityLifecycleCallbacks` in registration order. Below API 29,
* `ActivityLifecycleIntegration`'s callbacks run before this class's, provided the SDK is
* initialized before this class starts [watching][watch].
*
* - On API 29+, every `onActivityPreCreated` callback runs before any `onActivityCreated` callback,
* and every `onActivityDestroyed` callback before any `onActivityPostDestroyed` callback.
*
* **Lifetime**
*
* The callbacks are registered at most once per watcher and never unregistered. Like
* `defaultNavLeaseCoordinator`, which owns the watcher used in practice, they're meant to live as
* long as the process.
*/
internal class NavActivityWatcher(private val onActivityResumed: (activity: Activity) -> Unit) {
internal class NavActivityWatcher(
private val onActivityCreatedOrResumed: (activity: Activity, event: ActivityEvent) -> Unit,
private val onActivityDestroyed: (activity: Activity) -> Unit,
) {

/**
* The Activity lifecycle event passed to [onActivityCreatedOrResumed], described by [description]
* in past tense (e.g., "created") for log messages.
*/
enum class ActivityEvent(val description: String) {
CREATED("created"),
RESUMED("resumed"),
}

private var isWatching = false

Expand All @@ -33,11 +70,23 @@ internal class NavActivityWatcher(private val onActivityResumed: (activity: Acti

private inner class Callbacks : ActivityLifecycleCallbacks {

override fun onActivityResumed(activity: Activity) {
this@NavActivityWatcher.onActivityResumed(activity)
// Only called on API 29+, before any callback's onActivityCreated.
override fun onActivityPreCreated(activity: Activity, savedInstanceState: Bundle?) {
onActivityCreatedOrResumed(activity, ActivityEvent.CREATED)
}

// Covers API levels below 29, as they don't call onActivityPreCreated. Runs after
// ActivityLifecycleIntegration's onActivityCreated, which finds the slot taken and never binds
// the Activity's ui.load transaction. This is a known limitation (see the package README).
override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) {
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) {
onActivityCreatedOrResumed(activity, ActivityEvent.CREATED)
}
}

override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit
override fun onActivityResumed(activity: Activity) {
onActivityCreatedOrResumed(activity, ActivityEvent.RESUMED)
}

override fun onActivityStarted(activity: Activity) = Unit

Expand All @@ -47,6 +96,16 @@ internal class NavActivityWatcher(private val onActivityResumed: (activity: Acti

override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = Unit

override fun onActivityDestroyed(activity: Activity) = Unit
// Covers API levels below 29, which don't call onActivityPostDestroyed.
override fun onActivityDestroyed(activity: Activity) {
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) {
this@NavActivityWatcher.onActivityDestroyed(activity)
}
}

// Only called on API 29+, after every callback's onActivityDestroyed.
override fun onActivityPostDestroyed(activity: Activity) {
this@NavActivityWatcher.onActivityDestroyed(activity)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import androidx.lifecycle.LifecycleOwner
import io.sentry.IScope
import io.sentry.IScopes
import io.sentry.SentryLevel
import java.lang.ref.WeakReference

/**
* Manages a lease in order to determine which [NavScopes] instance can bind nav transactions and
Expand Down Expand Up @@ -57,9 +58,14 @@ internal class NavLeaseCoordinator {

/**
* Reports the host application's Activities to this coordinator so that it can detect whether a
* leaseholder is covered by a non-candidate root.
* leaseholder is covered by a non-candidate root, and retry a nav transaction blocked by a closed
* Activity's `ui.load` transaction.
*/
private val activityWatcher = NavActivityWatcher(onActivityResumed = ::onAnyActivityResumed)
private val activityWatcher =
NavActivityWatcher(
onActivityCreatedOrResumed = ::onActivityCreatedOrResumed,
onActivityDestroyed = ::onAnyActivityDestroyed,
)

/**
* Registers a [nav root][rootInfo] as a lease candidate until disposal or lifecycle destruction.
Expand All @@ -83,8 +89,8 @@ internal class NavLeaseCoordinator {
val initialResumeOrder = if (lifecycleState == Lifecycle.State.RESUMED) ++resumeOrder else 0
val candidate = Candidate(rootInfo, lifecycleState, initialResumeOrder, publisher, scopes)

// Only list the candidate once addObserver() succeeds, so a failure can't leak the host
// Activity.
// Only list the candidate once addObserver() succeeds, so a failure can't leave behind a
// candidate that never observes its nav root's lifecycle.
lifecycle.addObserver(candidate)
candidates += candidate
return candidate
Expand Down Expand Up @@ -175,23 +181,21 @@ internal class NavLeaseCoordinator {
/**
* The eligible candidate most recently observed to be in a resumed state, other than [excluding].
*
* If [resumedOnly] is set to `true`, only candidates that are resumed right now are considered;
* if `false`, started and paused candidates are also included. Candidates hosted by
* [excludingHostedBy] are skipped.
* Only candidates whose current state is at least [minimumState] are considered. Candidates
* hosted by [excludingHostedBy] are skipped.
*/
private fun mostRecentlyResumedCandidate(
excluding: Candidate? = null,
resumedOnly: Boolean = false,
minimumState: Lifecycle.State = Lifecycle.State.STARTED,
excludingHostedBy: Activity? = null,
): Candidate? =
candidates
.filter {
it !== excluding &&
(excludingHostedBy == null || it.hostActivity !== excludingHostedBy) &&
it.canReceiveLease &&
it.state.isAtLeast(Lifecycle.State.STARTED) &&
it.resumeOrder > 0 &&
(!resumedOnly || it.state == Lifecycle.State.RESUMED)
it.state.isAtLeast(minimumState) &&
it.resumeOrder > 0
}
.maxByOrNull { it.resumeOrder }

Expand Down Expand Up @@ -232,7 +236,7 @@ internal class NavLeaseCoordinator {
val next =
mostRecentlyResumedCandidate(
excluding = candidate,
resumedOnly = true,
minimumState = Lifecycle.State.RESUMED,
excludingHostedBy = candidate.hostActivity.takeIf { candidate.isHostActivityPaused },
)
when {
Expand Down Expand Up @@ -260,7 +264,9 @@ internal class NavLeaseCoordinator {
// beneath it), and that nav root isn't what the user last saw. A paused nav root that comes
// back to the foreground will acquire the lease when it resumes.
if (leaseHolder === candidate) {
mostRecentlyResumedCandidate(resumedOnly = true)?.let { next -> handOffLease(next, scope) }
mostRecentlyResumedCandidate(minimumState = Lifecycle.State.RESUMED)?.let { next ->
handOffLease(next, scope)
}
}
}

Expand Down Expand Up @@ -307,14 +313,17 @@ internal class NavLeaseCoordinator {
it.canReceiveLease && it.isActivityNavRoot(activity)
}

/** Withdraws the lease when the resumed [activity] appears to cover a non-resumed nav root. */
private fun onAnyActivityResumed(activity: Activity) {
// Assume the resumed `activity` is covering the current leaseholder if i) the leaseholder
// itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and iii)
// `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the leaseholder's
// own state rather than its host Activity's, as they differ for Fragment nav roots: a Fragment
// leaseholder that isn't resumed isn't what the user is interacting with, even if its host
// Activity is resumed.
/** Withdraws the lease when [activity] appears to cover a non-resumed nav root. */
private fun onActivityCreatedOrResumed(
activity: Activity,
event: NavActivityWatcher.ActivityEvent,
) {
// Assume the created or resumed `activity` is covering the current leaseholder if i) the
// leaseholder itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and
// iii) `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the
// leaseholder's own state rather than its host Activity's, as they differ for Fragment nav
// roots: a Fragment leaseholder that isn't resumed isn't what the user is interacting with,
// even if its host Activity is resumed.
val holder = leaseHolder ?: return
val hostActivity = holder.hostActivity ?: return
if (
Expand All @@ -329,11 +338,28 @@ internal class NavLeaseCoordinator {
withdrawLease(
scope,
withdrawalReason =
"${activity.javaClass.name} was resumed while the nav root wasn't resumed",
"${activity.javaClass.name} was ${event.description} while the nav root wasn't resumed",
)
}
}

/**
* After [activity] is destroyed, asks the leaseholder to retry publication if an Activity's
* `ui.load` blocked it. The publisher decides whether the blocker has left and retry is allowed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so my agent tells me that this retry isn't completely necessary.
for example, if you go to Activity B from nav screen A and press back quickly enough, the retry blocks A when it resumes

my clanker says it would simplify the code the remove the retry. ill let you decide!

*/
private fun onAnyActivityDestroyed(activity: Activity) {
val holder = leaseHolder ?: return
if (holder.hostActivity === activity) {
return
}

holder.withScopeOrNull { scope ->
if (scope != null) {
holder.publisher.retryTransactionPublication(scope)
}
}
}

/**
* A [nav root][rootInfo] eligible to hold the lease managed by [NavLeaseCoordinator].
*
Expand All @@ -347,7 +373,13 @@ internal class NavLeaseCoordinator {
private val scopes: IScopes,
) : NavLeaseRegistration, LifecycleEventObserver {

val hostActivity: Activity? = rootInfo.activity
// Held weakly because candidates live in a process-wide coordinator: if disposal fails before
// the candidate is removed, it must not keep its destroyed host Activity alive.
private val hostActivityRef: WeakReference<Activity>? = rootInfo.activity?.let(::WeakReference)

/** The Activity hosting this nav root, or `null` if it's unknown or has been collected. */
val hostActivity: Activity?
get() = hostActivityRef?.get()

/** Whether the host Activity is resumed; false when its lifecycle is unavailable. */
val isHostActivityResumed: Boolean
Expand Down Expand Up @@ -502,6 +534,12 @@ internal interface INavStatePublisher {
/** Publishes the candidate's staged nav state to [scope]. */
fun publishTo(scope: IScope)

/**
* Retries a nav transaction blocked by an Activity's `ui.load`, subject to
* [StagedNavTransaction.retryPublishIfBlocked]. Leaves other published nav state alone.
*/
fun retryTransactionPublication(scope: IScope)

/** Removes published nav state from [scope], retaining it for a later lease acquisition. */
fun unpublishFrom(scope: IScope)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ internal class NavScopes(
/** This instance's registration with the [coordinator], while it's attached. */
private var registration: NavLeaseRegistration? = null

private val stagedNavTransaction = StagedNavTransaction(delegate)
private val stagedNavTransaction =
StagedNavTransaction(delegate, delegate.options.monotonicTicker)
private val stagedNavData = StagedNavData()

private val navStatePublisher = NavStatePublisher()
Expand Down Expand Up @@ -154,6 +155,10 @@ internal class NavScopes(
stagedNavTransaction.publishTo(scope)
}

override fun retryTransactionPublication(scope: IScope) {
stagedNavTransaction.retryPublishIfBlocked(scope)
}

override fun unpublishFrom(scope: IScope) {
stagedNavTransaction.unpublishFrom(scope)
stagedNavData.clearPublishedStateFrom(scope)
Expand Down Expand Up @@ -228,7 +233,9 @@ internal class NavScopes(
override fun startNavTransaction(request: NavTransactionRequest) {
stagedNavTransaction.stage(request)
if (hasLease) {
stagedNavTransaction.publishTo(delegate)
// Don't retry b/c ui.load retry is only for returns to a nav root, not for navigation
// within it.
stagedNavTransaction.publishTo(delegate, canRetry = false)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return navigation cancels ui.load retry

Medium Severity

Returning from a covering Activity arms a one-shot ui.load retry, but a same-lease navigation before that Activity is destroyed cancels it. finishNavTransaction clears blockedPublication, and startNavTransaction then publishes with canRetry = false, so the new destination never re-arms. A Compose LaunchedEffect that navigates after resume (typical for activity results in Nav3) leaves that destination with no bound nav transaction until a later navigation.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 688b4a7. Configure here.

} else {
stagedNavData.deferPropagationContextRotation()
}
Expand Down
Loading
Loading