feat(android): [Part 4] Prefer Activity ui.load transactions over Nav3 transactions

CHANGELOG.md

@@ -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
 

sentry-android-navigation3/build.gradle.kts

@@ -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)

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

@@ -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
 
@@ -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
 
@@ -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)
+    }
   }
 }

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

@@ -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
@@ -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.
@@ -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
@@ -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 }
 
@@ -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 {
@@ -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)
+      }
     }
   }
 
@@ -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 (
@@ -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.
+   */
+  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].
    *
@@ -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
@@ -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)
 

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

@@ -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()
@@ -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)
@@ -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)
       } else {
         stagedNavData.deferPropagationContextRotation()
       }