fix(android): [Part 3] Stop attributing Nav3 data to screens without a SentryNavEffect

CHANGELOG.md

@@ -17,6 +17,10 @@
 
 - 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))
 
+### Fixes
+
+- Nav3: Stop attributing errors and spans from an Activity or ViewPager2 page without a `SentryNavEffect` to the most recently visited Nav3 root ([#6483](https://github.com/getsentry/sentry-java/pull/6483))
+
 ## 8.63.0
 
 ### Features

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt

@@ -0,0 +1,52 @@
+package io.sentry.compose.navigation3.lifecycle
+
+import android.app.Activity
+import android.app.Application
+import android.app.Application.ActivityLifecycleCallbacks
+import android.os.Bundle
+
+/**
+ * Watches every Activity of the host application and reports the Activity lifecycle events that
+ * [NavLeaseCoordinator] acts on.
+ *
+ * - [onActivityResumed] is called when any Activity resumes, including Activities without a
+ *   `SentryNavEffect`.
+ *
+ * **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) {
+
+  private var isWatching = false
+
+  /** Starts watching the Activities of [application]. Does nothing if already watching. */
+  fun watch(application: Application) {
+    if (isWatching) {
+      return
+    }
+    isWatching = true
+    application.registerActivityLifecycleCallbacks(Callbacks())
+  }
+
+  private inner class Callbacks : ActivityLifecycleCallbacks {
+
+    override fun onActivityResumed(activity: Activity) {
+      this@NavActivityWatcher.onActivityResumed(activity)
+    }
+
+    override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit
+
+    override fun onActivityStarted(activity: Activity) = Unit
+
+    override fun onActivityPaused(activity: Activity) = Unit
+
+    override fun onActivityStopped(activity: Activity) = Unit
+
+    override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = Unit
+
+    override fun onActivityDestroyed(activity: Activity) = Unit
+  }
+}

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt

@@ -1,5 +1,6 @@
 package io.sentry.compose.navigation3.lifecycle
 
+import android.app.Activity
 import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.Lifecycle.Event.ON_DESTROY
 import androidx.lifecycle.Lifecycle.Event.ON_PAUSE
@@ -10,6 +11,7 @@ import androidx.lifecycle.LifecycleEventObserver
 import androidx.lifecycle.LifecycleOwner
 import io.sentry.IScope
 import io.sentry.IScopes
+import io.sentry.SentryLevel
 
 /**
  * Manages a lease in order to determine which [NavScopes] instance can bind nav transactions and
@@ -53,6 +55,12 @@ internal class NavLeaseCoordinator {
    */
   private var resumeOrder: Long = 0
 
+  /**
+   * Reports the host application's Activities to this coordinator so that it can detect whether a
+   * leaseholder is covered by a non-candidate root.
+   */
+  private val activityWatcher = NavActivityWatcher(onActivityResumed = ::onAnyActivityResumed)
+
   /**
    * Registers a [nav root][rootInfo] as a lease candidate until disposal or lifecycle destruction.
    *
@@ -70,11 +78,13 @@ internal class NavLeaseCoordinator {
       return null
     }
 
+    rootInfo.activity?.application?.let { activityWatcher.watch(it) }
+
     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 leave behind a
-    // candidate that never observes its nav root's lifecycle.
+    // Only list the candidate once addObserver() succeeds, so a failure can't leak the host
+    // Activity.
     lifecycle.addObserver(candidate)
     candidates += candidate
     return candidate
@@ -93,15 +103,15 @@ internal class NavLeaseCoordinator {
 
     val holder = leaseHolder
 
-    // A nav root that isn't resumed doesn't take the lease from a resumed leaseholder, as it isn't
-    // the one the user is interacting with (e.g., a preloaded ViewPager2 page next to the current
-    // page). Instead, it acquires the lease when it resumes.
-    val wouldDisplaceResumedHolder =
-      holder != null &&
-        holder.state == Lifecycle.State.RESUMED &&
-        candidate.state != Lifecycle.State.RESUMED
+    // A nav root that isn't resumed while either the leaseholder or its own host Activity is
+    // resumed isn't the one the user is interacting with (e.g., a preloaded ViewPager2 page next to
+    // the current page, which may be a page without a SentryNavEffect and so never a leaseholder).
+    // Instead, it acquires the lease when it resumes.
+    val isBesideResumedContent =
+      candidate.state != Lifecycle.State.RESUMED &&
+        (holder?.state == Lifecycle.State.RESUMED || candidate.isHostActivityResumed)
 
-    if (candidate.canAcquireLeaseOnFirstUpdate && !wouldDisplaceResumedHolder) {
+    if (candidate.canAcquireLeaseOnFirstUpdate && !isBesideResumedContent) {
       transferLeaseTo(candidate, scope)
     }
   }
@@ -136,19 +146,48 @@ internal class NavLeaseCoordinator {
     }
   }
 
+  /**
+   * Releases the lease without handing it off and unpublishes its nav state from [scope]. The
+   * candidate retains staged state for a later return. Without a scope, state remains published.
+   *
+   * Logs [withdrawalReason].
+   */
+  private fun withdrawLease(scope: IScope?, withdrawalReason: String) {
+    val holder = leaseHolder ?: return
+
+    leaseHolder = null
+    scope
+      ?.options
+      ?.logger
+      ?.log(
+        SentryLevel.DEBUG,
+        "Withdrew the Nav3 lease from a nav root hosted by %s: %s.",
+        holder.hostActivity?.javaClass?.name,
+        withdrawalReason,
+      )
+    if (scope != null && lastPublisher === holder) {
+      // Clear the last publisher before calling into the scope (see the class's exception policy).
+      lastPublisher = null
+      holder.publisher.unpublishFrom(scope)
+    }
+  }
+
   /**
    * 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.
+   * if `false`, started and paused candidates are also included. Candidates hosted by
+   * [excludingHostedBy] are skipped.
    */
   private fun mostRecentlyResumedCandidate(
     excluding: Candidate? = null,
     resumedOnly: Boolean = false,
+    excludingHostedBy: Activity? = null,
   ): Candidate? =
     candidates
       .filter {
         it !== excluding &&
+          (excludingHostedBy == null || it.hostActivity !== excludingHostedBy) &&
           it.canReceiveLease &&
           it.state.isAtLeast(Lifecycle.State.STARTED) &&
           it.resumeOrder > 0 &&
@@ -176,7 +215,9 @@ internal class NavLeaseCoordinator {
    * Records that [candidate] paused.
    *
    * If [candidate] holds the lease when it pauses, hands it to another resumed nav root (e.g., a
-   * Fragment whose dialog is being dismissed); otherwise the paused nav root keeps the lease.
+   * Fragment whose dialog is being dismissed), unless that nav root shares [candidate]'s pausing
+   * host Activity. If none exists, withdraws it when the host Activity remains resumed; otherwise
+   * the paused nav root keeps the lease.
    */
   private fun pauseCandidate(candidate: Candidate, scope: IScope?) {
     candidate.pause()
@@ -185,8 +226,22 @@ internal class NavLeaseCoordinator {
     // window or picture-in-picture mode), and any other paused nav root is beneath it. A displaced
     // nav root will acquire the lease again when it resumes.
     if (leaseHolder === candidate) {
-      mostRecentlyResumedCandidate(excluding = candidate, resumedOnly = true)?.let { next ->
-        handOffLease(next, scope)
+      // An Activity pauses before its Fragments, so when the host Activity is pausing, its other
+      // nav roots are still resumed but about to pause too. Handing them the lease would publish
+      // their nav state, and start a nav transaction, just as the app is backgrounded or covered.
+      val next =
+        mostRecentlyResumedCandidate(
+          excluding = candidate,
+          resumedOnly = true,
+          excludingHostedBy = candidate.hostActivity.takeIf { candidate.isHostActivityPaused },
+        )
+      when {
+        next != null -> handOffLease(next, scope)
+        candidate.isHostActivityResumed ->
+          withdrawLease(
+            scope,
+            withdrawalReason = "the nav root paused while its host Activity stayed resumed",
+          )
       }
     }
   }
@@ -242,6 +297,43 @@ internal class NavLeaseCoordinator {
     }
   }
 
+  /**
+   * Whether [activity] owns a candidate's nav root that
+   * [can receive the lease][Candidate.canReceiveLease].
+   *
+   * A returning Activity root takes the lease on resume and should not trigger withdrawal first.
+   */
+  private fun isActivityNavRoot(activity: Activity): Boolean = candidates.any {
+    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.
+    val holder = leaseHolder ?: return
+    val hostActivity = holder.hostActivity ?: return
+    if (
+      hostActivity === activity ||
+        holder.state == Lifecycle.State.RESUMED ||
+        isActivityNavRoot(activity)
+    ) {
+      return
+    }
+
+    holder.withScopeOrNull { scope ->
+      withdrawLease(
+        scope,
+        withdrawalReason =
+          "${activity.javaClass.name} was resumed while the nav root wasn't resumed",
+      )
+    }
+  }
+
   /**
    * A [nav root][rootInfo] eligible to hold the lease managed by [NavLeaseCoordinator].
    *
@@ -255,6 +347,19 @@ internal class NavLeaseCoordinator {
     private val scopes: IScopes,
   ) : NavLeaseRegistration, LifecycleEventObserver {
 
+    val hostActivity: Activity? = rootInfo.activity
+
+    /** Whether the host Activity is resumed; false when its lifecycle is unavailable. */
+    val isHostActivityResumed: Boolean
+      get() = hostActivityState == Lifecycle.State.RESUMED
+
+    /** Whether the host Activity isn't resumed; false when its lifecycle is unavailable. */
+    val isHostActivityPaused: Boolean
+      get() = hostActivityState.let { it != null && it != Lifecycle.State.RESUMED }
+
+    private val hostActivityState: Lifecycle.State?
+      get() = (hostActivity as? LifecycleOwner)?.lifecycle?.currentState
+
     /** Most recent resume order; zero until this candidate first resumes. */
     var resumeOrder: Long = initialResumeOrder
       private set
@@ -353,7 +458,7 @@ internal class NavLeaseCoordinator {
      * Runs [block] with the scope, or with `null` if [IScopes.configureScope] skips its callback.
      * Does not rerun a callback that throws.
      */
-    private fun withScopeOrNull(block: (IScope?) -> Unit) {
+    fun withScopeOrNull(block: (IScope?) -> Unit) {
       var callbackInvoked = false
       scopes.configureScope { scope ->
         callbackInvoked = true
@@ -364,6 +469,10 @@ internal class NavLeaseCoordinator {
         block(null)
       }
     }
+
+    /** Whether [activity] itself owns this nav root, rather than a Fragment it hosts. */
+    fun isActivityNavRoot(activity: Activity): Boolean =
+      hostActivity === activity && (activity as? LifecycleOwner)?.lifecycle === lifecycle
   }
 }
 

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md

@@ -56,8 +56,9 @@ A displaced nav root remains eligible for lease restoration until it stops (`ON_
 root's nav data is reinstated, and so is its nav transaction if unfinished. Otherwise, the nav root gets a new nav transaction (see
 [below](#returning-to-a-nav-root)).
 
-> † The exception is an incoming nav root that isn't resumed while the leaseholder is, e.g., a
-preloaded ViewPager2 Fragment page next to the current one. The user isn't interacting with such a
+> † The exception is an incoming nav root that isn't resumed while the leaseholder or the incoming
+nav root's host Activity is, e.g., a preloaded ViewPager2 Fragment page next to the current one,
+whether or not the current page has a `SentryNavEffect`. The user isn't interacting with such a
 nav root yet, so it acquires the lease when it resumes instead.
 
 The following diagram illustrates how handoff works:
@@ -144,6 +145,29 @@ Alternative at rollback point: B disposes before A stops
                              Finish txB if unfinished                 │
 ```
 
+### Non-candidate roots and lease withdrawal
+
+An Activity or Fragment without a `SentryNavEffect` (aka, a "non-candidate root") can't acquire the lease. When one replaces the
+leaseholder, the coordinator "withdraws" the lease instead, i.e., it unpublishes the leaseholder's nav state without handing off the lease,
+leaving the transaction unbound but running until the leaseholder resumes.
+
+Non-candidate roots never register, so the coordinator infers their presence from either of two signals:
+
+- another Activity resumes while the leaseholder isn't resumed, unless the resumed Activity is
+  itself a nav root, which takes the lease when it resumes; or
+
+- the leaseholder pauses while its host Activity stays resumed (e.g., a ViewPager2 swipe). (Note
+  that an Activity pauses before its Fragments do when the Activity is backgrounded or covered.)
+
+**Limitations**: Leaseholders whose host Activity is unknown are never withdrawn. Fragments that cover the leaseholder without pausing it
+(e.g., dialogs) go undetected. These two signals are deliberately the only ones: other surfaces that cover a leaseholder without pausing it
+(e.g., bottom sheets or Compose dialogs) aren't detected, and shouldn't be by adding signals here.
+
+**Limitation (before the covering Activity resumes)**: The lease is withdrawn only once the covering Activity resumes. Until then (i.e.,
+during its `onCreate` and `onStart`), errors and spans the covering Activity produces are still attributed to the leaseholder. For the same
+reason, the covering Activity's `ui.load` transaction isn't bound to the scope: `ActivityLifecycleIntegration` tries to bind it when the
+Activity is created, while the leaseholder's nav transaction still holds the scope's transaction slot.
+
 ### Returning to a nav root
 
 A nav root is "returned to" when its candidate acquires the lease at any time other than on its first back stack update, e.g., when the
@@ -167,3 +191,17 @@ gets a transaction on its first back stack update. See [#6454](https://github.co
 Bringing the app back to the foreground isn't a return either: the leaseholder keeps the lease while the app is in the background (see
 `stopCandidate`), so it starts no new nav transaction when it resumes. This matches the rest of the SDK, which starts no transaction when an
 existing Activity comes back to the foreground.
+
+### Android ordering assumptions
+
+`NavLeaseCoordinator` relies on the following sequences, which Android doesn't document but AOSP and androidx have historically upheld:
+
+- An Activity's `onActivityResumed` callbacks run before its `Lifecycle` dispatches
+  `ON_RESUME` (see `isActivityNavRoot`).
+
+- An Activity's `Lifecycle` dispatches `ON_PAUSE` before its Fragments'
+  lifecycles do. androidx's `ReportFragment` and `FragmentActivity` ensure this, not the
+  platform.
+
+Going forward, if an order changes, nav data may be attributed to the wrong nav root, or a nav transaction may go unbound, but nothing would
+throw.