Skip to content

Commit a010c9e

Browse files
committed
fix(android): Keep the nav leaseholder as the published data owner
transferLeaseTo assigned the lease before clearing the previous owner's published nav data. When that callback threw, the new leaseholder was recorded against a stale publisher, so a later handoff cleared the wrong owner's data and never cleared the leaseholder's own. Always record the incoming candidate as the publisher once it takes the lease. Add regression coverage for handoff cleanup failures. Refs JAVA-274
1 parent bc5ca85 commit a010c9e

3 files changed

Lines changed: 187 additions & 38 deletions

File tree

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

Lines changed: 66 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,19 @@ import io.sentry.SpanStatus
1616
* Note: "Owner" below refers to the `LifecycleOwner` whose lifecycle events the candidate's
1717
* `NavScopes` relays.
1818
*
19+
* **Exception semantics**
20+
*
21+
* Every method in this class with an `IScope` parameter can throw arbitrary exceptions due to:
22+
*
23+
* 1. the [onPublishData][registerCandidate] and [onClearPublishedData][registerCandidate] callbacks
24+
* supplied at registration;
25+
* 2. the `IScope` the method is handed, whose mutators notify every observer registered through
26+
* `SentryOptions.addScopeObserver` – including the host app's – in an unguarded loop; and
27+
* 3. `finish()` on a retained nav transaction, which may run the host app's event processors.
28+
*
29+
* This class restores its own bookkeeping before the exception leaves, so the coordinator remains
30+
* usable.
31+
*
1932
* **Thread safety**
2033
*
2134
* This class is ***not*** thread-safe. All `NavScopes` instances using this coordinator must
@@ -83,6 +96,8 @@ internal class NavLeaseCoordinator {
8396
*
8497
* A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the
8598
* candidate remains eligible for fallback once a scope becomes available.
99+
*
100+
* Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc.
86101
*/
87102
fun resumeCandidate(owner: Any, scope: IScope?) {
88103
findCandidate(owner)?.let {
@@ -104,6 +119,8 @@ internal class NavLeaseCoordinator {
104119
*
105120
* A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the
106121
* candidate remains eligible for fallback once a scope becomes available.
122+
*
123+
* Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc.
107124
*/
108125
fun pauseCandidate(owner: Any, scope: IScope?) {
109126
findCandidate(owner)?.let {
@@ -132,6 +149,8 @@ internal class NavLeaseCoordinator {
132149
*
133150
* A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the
134151
* candidate remains eligible for fallback once a scope becomes available.
152+
*
153+
* Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc.
135154
*/
136155
fun stopCandidate(owner: Any, scope: IScope?) {
137156
findCandidate(owner)?.let {
@@ -156,36 +175,34 @@ internal class NavLeaseCoordinator {
156175
* left to time out, its published data is left in place, and the lease is released rather than
157176
* handed off.
158177
*
159-
* Coordinator references are released even if scope cleanup or lease handoff throws. The
160-
* exception is left to the caller to handle; remaining scope cleanup may be incomplete.
178+
* Coordinator references are released even if scope cleanup or lease handoff throws. See
179+
* "Exception semantics" in the class KDoc. The remaining scope cleanup is then skipped.
161180
*/
162181
fun unregisterCandidate(owner: Any, scope: IScope?) {
163-
findCandidate(owner)?.let {
164-
try {
165-
finishTransaction(it, scope)
166-
candidates.remove(it)
182+
val candidate = findCandidate(owner) ?: return
167183

168-
if (publishedDataCandidate === it) {
169-
publishedDataCandidate = null
170-
if (scope != null) {
171-
it.onClearPublishedData(scope)
172-
}
173-
}
184+
try {
185+
finishTransaction(candidate, scope)
186+
candidates.remove(candidate)
174187

175-
if (leaseHolder === it) {
176-
handOffLease(mostRecentlyResumedCandidate(), scope)
177-
}
178-
} finally {
179-
// Scope observers and transaction callbacks can throw. Never retain a departing owner
180-
// just because cleanup failed, and preserve any successor selected during handoff.
181-
candidates.remove(it)
182-
it.transaction = null
183-
if (publishedDataCandidate === it) {
184-
publishedDataCandidate = null
185-
}
186-
if (leaseHolder === it) {
187-
leaseHolder = null
188-
}
188+
if (publishedDataCandidate === candidate) {
189+
publishedDataCandidate = null
190+
scope?.let { candidate.onClearPublishedData(it) }
191+
}
192+
193+
if (leaseHolder === candidate) {
194+
handOffLease(mostRecentlyResumedCandidate(), scope)
195+
}
196+
} finally {
197+
// Scope observers and transaction callbacks can throw. Never retain a departing owner
198+
// just because cleanup failed, and preserve any successor selected during handoff.
199+
candidates.remove(candidate)
200+
candidate.transaction = null
201+
if (publishedDataCandidate === candidate) {
202+
publishedDataCandidate = null
203+
}
204+
if (leaseHolder === candidate) {
205+
leaseHolder = null
189206
}
190207
}
191208
}
@@ -203,6 +220,8 @@ internal class NavLeaseCoordinator {
203220
* If the lease transfers, ownership and transaction binding are updated before the candidate's
204221
* staged nav data is published. Publication completes before this method returns, so the caller
205222
* can then apply the back-stack update. Later calls for the same [owner] do nothing.
223+
*
224+
* Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc.
206225
*/
207226
fun onBackStackUpdate(owner: Any, scope: IScope) {
208227
val candidate = findCandidate(owner) ?: return
@@ -293,6 +312,14 @@ internal class NavLeaseCoordinator {
293312
}
294313
}
295314

315+
/**
316+
* Hands the lease to [candidate], or releases it given `null`.
317+
*
318+
* Maintains the invariant that, while a lease is held, the leaseholder is also the candidate
319+
* recorded as having published the shared scope's nav data. The invariant holds even when a
320+
* publish or clear callback throws part way through the handoff. See "Exception semantics" in the
321+
* class KDoc.
322+
*/
296323
private fun transferLeaseTo(candidate: Candidate?, scope: IScope) {
297324
if (leaseHolder === candidate) {
298325
return
@@ -330,11 +357,20 @@ internal class NavLeaseCoordinator {
330357
// previous owner's values don't linger where the candidate has nothing staged to replace
331358
// them.
332359
val previousPublisher = publishedDataCandidate
333-
if (previousPublisher != null && previousPublisher !== candidate) {
334-
previousPublisher.onClearPublishedData(scope)
360+
try {
361+
if (previousPublisher != null && previousPublisher !== candidate) {
362+
previousPublisher.onClearPublishedData(scope)
363+
}
364+
publishedDataCandidate = candidate
365+
candidate.onPublishData(scope)
366+
} finally {
367+
// Scope observers can throw. A candidate owns the scope's nav data from the moment it
368+
// takes the lease, so record it as the publisher even if clearing the previous owner's
369+
// data threw. Leaving the previous owner recorded here would strand both owners' values
370+
// b/c the next handoff would clear data the candidate may have already replaced, and would
371+
// never clear the candidate's own.
372+
publishedDataCandidate = candidate
335373
}
336-
publishedDataCandidate = candidate
337-
candidate.onPublishData(scope)
338374
}
339375
}
340376

‎sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import io.sentry.Breadcrumb
1515
import io.sentry.Hint
1616
import io.sentry.IScope
1717
import io.sentry.IScopes
18-
import io.sentry.ITransaction
1918
import io.sentry.Scope
2019
import io.sentry.ScopeCallback
2120
import io.sentry.SentryOptions
@@ -550,13 +549,6 @@ class SentryNavEffectTest {
550549
return navigationContext[BACKSTACK_KEY] as? List<Map<String, Any?>>
551550
}
552551

553-
private fun ITransaction.navigationBackStack(): List<Map<String, Any?>>? {
554-
val navigationContext = contexts[NAVIGATION_CONTEXT_KEY] as? Map<*, *> ?: return null
555-
556-
@Suppress("UNCHECKED_CAST")
557-
return navigationContext[BACKSTACK_KEY] as? List<Map<String, Any?>>
558-
}
559-
560552
private companion object {
561553
const val NAVIGATION_CONTEXT_KEY = "Navigation"
562554
const val BACKSTACK_KEY = "Back Stack"

‎sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import org.mockito.kotlin.verify
1717
import org.mockito.kotlin.verifyNoInteractions
1818
import org.mockito.kotlin.whenever
1919

20+
@Suppress("LargeClass")
2021
class NavLeaseCoordinatorTest {
2122

2223
private val owner = Any()
@@ -267,6 +268,126 @@ class NavLeaseCoordinatorTest {
267268
assertThat(fixture.cleared).isEmpty()
268269
}
269270

271+
/**
272+
* An owner that leaves must not leave its nav data behind on the shared scope.
273+
*
274+
* The coordinator records which owner last published nav data. Each handoff uses that record to
275+
* clear the previous owner's screen and back stack, then publishes the incoming owner's. If the
276+
* clear attempt throws, the coordinator must still record the incoming owner as the one that last
277+
* published.
278+
*
279+
* This test runs two handoffs over three owners. The first owner publishes; the second takes the
280+
* lease; and then the first owner's clear attempt throws. The third then takes the lease. The
281+
* test verifies that third handoff clears the second owner's data, and not the first's. That lets
282+
* us ensure the second problem listed below doesn't happen.
283+
*
284+
* In the setup above, if the coordinator didn't record the second owner's data after the first
285+
* owner's clear attempt threw, two things would happen when handing off to the third owner:
286+
*
287+
* 1. Harmless: The handoff logic would try to clear the first owner's data again. That'd remove
288+
* nothing because the scope no longer holds the values the first owner published.
289+
* 2. Problematic: The handoff would never clear the second owner's data. Instead, the second
290+
* owner's data would stay on the scope, so later events would report stale nav data.
291+
*/
292+
@Test
293+
fun `if a clear attempt throws during a handoff, the next handoff clears the new leaseholder's nav data`() {
294+
val fixture = Fixture.create()
295+
val navLeaseCoordinator = fixture.navLeaseCoordinator
296+
val failure = IllegalStateException("clear failed")
297+
var failClear = true
298+
val firstOwner = previous
299+
val secondOwner = incoming
300+
val thirdOwner = owner
301+
fixture.acquireLease(
302+
firstOwner,
303+
onClearPublishedData = {
304+
if (failClear) {
305+
failClear = false
306+
throw failure
307+
}
308+
fixture.cleared += firstOwner
309+
},
310+
)
311+
fixture.register(secondOwner, Lifecycle.State.RESUMED)
312+
313+
val thrown =
314+
assertThrows(IllegalStateException::class.java) {
315+
navLeaseCoordinator.onBackStackUpdate(secondOwner, fixture.scope)
316+
}
317+
318+
assertThat(thrown).isSameInstanceAs(failure)
319+
assertThat(navLeaseCoordinator.hasLease(secondOwner)).isTrue()
320+
321+
fixture.acquireLease(thirdOwner)
322+
323+
assertThat(fixture.cleared).containsExactly(secondOwner)
324+
}
325+
326+
/**
327+
* The same guarantee as
328+
* [if a clear attempt throws during a handoff, the next handoff clears the new leaseholder's nav data],
329+
* but for a failure while publishing instead of while clearing.
330+
*
331+
* This case already worked before the fix, because the coordinator records the incoming owner
332+
* before it publishes. The test exists to catch a future change that moves the record after the
333+
* publish.
334+
*/
335+
@Test
336+
fun `if a publish attempt throws during a handoff, the next handoff clears the new leaseholder's nav data`() {
337+
val fixture = Fixture.create()
338+
val navLeaseCoordinator = fixture.navLeaseCoordinator
339+
val failure = IllegalStateException("publish failed")
340+
val firstOwner = previous
341+
val secondOwner = incoming
342+
val thirdOwner = owner
343+
fixture.acquireLease(firstOwner)
344+
navLeaseCoordinator.registerCandidate(
345+
secondOwner,
346+
Lifecycle.State.RESUMED,
347+
onPublishData = { throw failure },
348+
onClearPublishedData = { fixture.cleared += secondOwner },
349+
)
350+
351+
val thrown =
352+
assertThrows(IllegalStateException::class.java) {
353+
navLeaseCoordinator.onBackStackUpdate(secondOwner, fixture.scope)
354+
}
355+
356+
assertThat(thrown).isSameInstanceAs(failure)
357+
assertThat(navLeaseCoordinator.hasLease(secondOwner)).isTrue()
358+
359+
fixture.acquireLease(thirdOwner)
360+
361+
assertThat(fixture.cleared).containsExactly(firstOwner, secondOwner).inOrder()
362+
}
363+
364+
@Test
365+
fun `unregistering an owner that threw while clearing its nav data preserves the new leaseholder's nav data`() {
366+
val fixture = Fixture.create()
367+
val navLeaseCoordinator = fixture.navLeaseCoordinator
368+
val failure = IllegalStateException("clear failed")
369+
var failClear = true
370+
fixture.acquireLease(
371+
previous,
372+
onClearPublishedData = {
373+
if (failClear) {
374+
failClear = false
375+
throw failure
376+
}
377+
fixture.cleared += previous
378+
},
379+
)
380+
fixture.register(incoming, Lifecycle.State.RESUMED)
381+
assertThrows(IllegalStateException::class.java) {
382+
navLeaseCoordinator.onBackStackUpdate(incoming, fixture.scope)
383+
}
384+
385+
navLeaseCoordinator.unregisterCandidate(previous, fixture.scope)
386+
387+
assertThat(navLeaseCoordinator.hasLease(incoming)).isTrue()
388+
assertThat(fixture.cleared).isEmpty()
389+
}
390+
270391
@Test
271392
fun `later back stack updates do not republish nav data or reclaim the nav lease`() {
272393
val fixture = Fixture.create()

0 commit comments

Comments
 (0)