Skip to content

Commit cf074b6

Browse files
amillezclaude
andcommitted
fix(android): Invalid window token from SCVH teardown race (round 2)
v0.1.5 added `unscheduleScvhTraversals` before `host.release()`, which catches Choreographer `TraversalRunnable`s queued *before* teardown. Two paths still slipped through and crashed in production: 1. Reflective unschedule unavailable on the device — the SCVH-field probe fails or its `unscheduleTraversals` invocation throws, latches `scvhReflectionDisabled = true`, and every later detach goes ahead with no cancellation at all. 2. Even when reflection succeeds, `host.release()` removes the WWM token inside an *asynchronously*-dispatched `doDie()` (via `ViewRootImpl.die(false)` → `MSG_DIE`). Between our cancel and `doDie()` actually running, framework code paths (`dispatchDetachedFromWindow` from the activity-side `removeView`, animation cleanup, focus changes) can call `requestLayout()` on the SCVH's content tree and schedule a *fresh* Choreographer callback. If `doDie()` then wins the race, the queued callback fires on a dead window, calls `WindowlessWindowManager.relayout`, and throws `IllegalArgumentException: Invalid window token (never added or removed already)` from `Choreographer.doFrame` — outside any try/catch of ours. Layered defences in `detachCoverView`: - `FreezableFrameLayout` content root whose `requestLayout()` / `invalidate()` no-op while a `frozen` flag is set. Set before any teardown step; from that point no layout request reaches `ViewRootImpl.scheduleTraversals` and no new Choreographer callback can be queued during teardown. - `coverDetaching` re-entrance guard + snapshot-then-null-out of shared state. Any synchronous re-entrant call (animation cancel listener, detach dispatch) sees cleared fields and the guard, and bails before double-removing or double-releasing. - `unscheduleScvhTraversals` called both before `removeView` AND inside `safeReleaseScvh` (defence in depth). - Prefer `removeViewImmediate` so `dispatchDetachedFromWindow` runs inline while the freeze is in place; fall back to async `removeView` on OEM impls where immediate removal throws. - `setCoverVisibility` checks `view.windowToken != null` before `updateViewLayout` — same WMS/WWM path that throws once the token is gone. - When `unscheduleScvhTraversals` latches `scvhReflectionDisabled`, also latch `scvhDisabled` so future attaches use the legacy WindowManager path (no `WindowlessWindowManager` involved). We lose the SF-direct alpha toggle on that device, but eliminate the crash surface entirely. Verified with `./gradlew :react-native-cover:compileDebugKotlin` from the example app — builds cleanly; only pre-existing `val defaultDisplay: Display!` deprecation warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1650639 commit cf074b6

1 file changed

Lines changed: 232 additions & 27 deletions

File tree

android/src/main/java/com/margelo/nitro/cover/HybridCover.kt

Lines changed: 232 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,58 @@ private data class ImageConfig(
4747
var bitmap: Bitmap? = null,
4848
)
4949

50+
/// Cover-content container used as the root of every cover view tree.
51+
/// Carries a `frozen` flag that, while set, no-ops `requestLayout` and
52+
/// `invalidate` calls — breaking the layout-request chain before it
53+
/// reaches the surrounding `ViewRootImpl` and triggers a Choreographer
54+
/// `scheduleTraversals`.
55+
///
56+
/// Why this exists (regression guard for "Invalid window token" crashes
57+
/// reported from the SCVH teardown race — see `detachCoverView`):
58+
///
59+
/// `safeReleaseScvh` already cancels in-flight TraversalRunnables on
60+
/// the SCVH's internal `ViewRootImpl` via reflection. But that helper
61+
/// can be defeated two ways:
62+
///
63+
/// 1. Reflection blocked on this device → `scvhReflectionDisabled`
64+
/// latches true and no cancel ever lands. (We now also latch
65+
/// `scvhDisabled` in that case to avoid the SCVH path entirely,
66+
/// but the legacy non-SCVH path is the cure for *future*
67+
/// attaches — it doesn't undo damage already in flight.)
68+
/// 2. Even when reflection succeeds, the gap between our cancel and
69+
/// the WWM token removal inside `host.release() → die() → doDie()`
70+
/// (which posts MSG_DIE to run later) is wide enough for fresh
71+
/// `requestLayout` calls — fired by `dispatchDetachedFromWindow`
72+
/// propagating from the activity-side `removeView`, by animation
73+
/// cleanup, or by focus changes — to schedule a brand-new
74+
/// Choreographer callback. If MSG_DIE then runs first, the queued
75+
/// callback fires on a dead window, calls `WWM.relayout`, and
76+
/// throws `IllegalArgumentException: Invalid window token`.
77+
///
78+
/// Overriding `requestLayout()` to be a no-op while frozen breaks the
79+
/// chain: child views can still call `requestLayout`, but the call stops
80+
/// at this container and never reaches `ViewRootImpl.scheduleTraversals`.
81+
/// Frozen views also skip `invalidate()` for parity; `invalidate` alone
82+
/// doesn't schedule a traversal, but suppressing it during teardown
83+
/// avoids spurious redraw work on a surface we're about to release.
84+
///
85+
/// The freeze is one-way per view instance: once a cover content view
86+
/// is being torn down, it's discarded. `attachCover` always builds a
87+
/// fresh container, so a future show() starts from `frozen = false`.
88+
private class FreezableFrameLayout(context: Context) : FrameLayout(context) {
89+
@Volatile var frozen: Boolean = false
90+
91+
override fun requestLayout() {
92+
if (frozen) return
93+
super.requestLayout()
94+
}
95+
96+
override fun invalidate() {
97+
if (frozen) return
98+
super.invalidate()
99+
}
100+
}
101+
50102
/// Default blur intensity when the JS caller passes `undefined` to
51103
/// `setBlur(style)`. Mirrors the iOS default.
52104
private const val DEFAULT_BLUR_INTENSITY: Float = 0.4f
@@ -185,6 +237,18 @@ class HybridCover : HybridCoverSpec() {
185237
/// toggle can cancel a still-running fade before starting its own.
186238
private var scvhAnimator: android.animation.ValueAnimator? = null
187239

240+
/// Re-entrance guard for `detachCoverView`. Teardown calls
241+
/// `WindowManager.removeViewImmediate` and `safeReleaseScvh`, both of
242+
/// which can dispatch detach callbacks / animation cancellations on
243+
/// the main thread synchronously. If any of those handlers calls back
244+
/// into a path that triggers another `detachCoverView` (e.g. a
245+
/// listener firing `disable()`), the second call would double-remove
246+
/// the same view (logged-but-harmless IAE) and double-release the
247+
/// same SCVH host (may double-`die()` the internal ViewRootImpl,
248+
/// which is precisely the kind of post-removal-relayout we're trying
249+
/// to prevent). Flag is checked at entry and cleared in `finally`.
250+
private var coverDetaching: Boolean = false
251+
188252
/// API 30+ fast-path. The cover content (FrameLayout with color /
189253
/// image / blur) is hosted inside a `SurfaceControlViewHost`, which
190254
/// renders it into a `SurfaceControl` WE own. The cover Window's
@@ -969,12 +1033,22 @@ class HybridCover : HybridCoverSpec() {
9691033
}
9701034
}
9711035
if (!found) {
972-
Log.w(TAG, "unscheduleScvhTraversals: no ViewRootImpl field found on SurfaceControlViewHost; disabling reflection")
1036+
Log.w(TAG, "unscheduleScvhTraversals: no ViewRootImpl field found on SurfaceControlViewHost; disabling reflection AND SCVH")
9731037
scvhReflectionDisabled = true
1038+
// Without a working unschedule, every SCVH teardown on this
1039+
// device is exposed to the WWM-relayout-after-token-removed
1040+
// crash. The legacy non-SCVH attach path uses a regular
1041+
// activity-WM window (no WindowlessWindowManager involved),
1042+
// so falling back to it on every subsequent attach eliminates
1043+
// the race entirely — at the cost of the SF-direct alpha
1044+
// toggle the snapshot race fix relies on. We accept that
1045+
// trade-off rather than risk recurring process crashes.
1046+
scvhDisabled = true
9741047
}
9751048
} catch (e: Throwable) {
976-
Log.w(TAG, "unscheduleScvhTraversals: reflection failed (${e.javaClass.simpleName}): ${e.message}")
1049+
Log.w(TAG, "unscheduleScvhTraversals: reflection failed (${e.javaClass.simpleName}): ${e.message}; disabling reflection AND SCVH")
9771050
scvhReflectionDisabled = true
1051+
scvhDisabled = true
9781052
}
9791053
}
9801054

@@ -1143,10 +1217,20 @@ class HybridCover : HybridCoverSpec() {
11431217
else params.flags or invisibleFlags
11441218
if (newFlags != params.flags) {
11451219
params.flags = newFlags
1146-
try {
1147-
activity.windowManager.updateViewLayout(view, params)
1148-
} catch (e: Throwable) {
1149-
Log.w(TAG, "setCoverVisibility: updateViewLayout failed: $e")
1220+
// Skip the WindowManager update if the view has been detached
1221+
// out from under us mid-flip — `updateViewLayout` would route
1222+
// through WindowManagerGlobal to the same WMS/WWM path that
1223+
// throws "Invalid window token" once the token is gone. The
1224+
// cover state we set above (alpha) is moot at this point since
1225+
// detach is imminent or already done.
1226+
if (view.windowToken == null) {
1227+
Log.w(TAG, "setCoverVisibility: skipping updateViewLayout — windowToken is null")
1228+
} else {
1229+
try {
1230+
activity.windowManager.updateViewLayout(view, params)
1231+
} catch (e: Throwable) {
1232+
Log.w(TAG, "setCoverVisibility: updateViewLayout failed: $e")
1233+
}
11501234
}
11511235
}
11521236

@@ -1255,30 +1339,151 @@ class HybridCover : HybridCoverSpec() {
12551339
/// Pure detach helper — no state-machine logic. Used by both
12561340
/// removeCoverImmediately (full teardown) and attachCover (re-mount
12571341
/// on a new parent token).
1342+
///
1343+
/// Ordering matters here. The teardown sequence has to defend against
1344+
/// a multi-layer race that surfaces as
1345+
///
1346+
/// IllegalArgumentException: Invalid window token (never added or
1347+
/// removed already)
1348+
/// at WindowlessWindowManager.relayout
1349+
/// at ViewRootImpl.relayoutWindow → performTraversals
1350+
/// at Choreographer.doFrame
1351+
///
1352+
/// on production devices (most often during rapid mount/unmount,
1353+
/// backgrounding, and activity recreation). The race:
1354+
///
1355+
/// 1. `host.release()` ultimately removes our window from the
1356+
/// SCVH's `WindowlessWindowManager`, but the removal happens
1357+
/// inside an asynchronously-dispatched `doDie()` (via
1358+
/// `ViewRootImpl.die(false)` → `MSG_DIE`), not synchronously.
1359+
/// 2. Between the call to `release()` and `doDie()` actually
1360+
/// running, the SCVH's content tree is still attached and
1361+
/// anything that calls `requestLayout()` on it schedules a
1362+
/// fresh Choreographer traversal.
1363+
/// 3. `dispatchDetachedFromWindow` (fired by removeView on the
1364+
/// enclosing SurfaceView), animation cancellations, and
1365+
/// focus changes all CAN call `requestLayout` during teardown.
1366+
/// 4. If `doDie()` runs first and removes the WWM token, then the
1367+
/// queued traversal fires, calls `WWM.relayout`, finds the
1368+
/// token absent, and throws — outside any try/catch of ours
1369+
/// because it's dispatched from `Looper.loop`.
1370+
///
1371+
/// Mitigation, in order:
1372+
///
1373+
/// - Mark the SCVH content `FreezableFrameLayout` as `frozen` BEFORE
1374+
/// touching anything else; from this point on, no `requestLayout`
1375+
/// reaches the SCVH's `ViewRootImpl.scheduleTraversals`.
1376+
/// - Null out the shared state fields BEFORE running teardown, so
1377+
/// any synchronous re-entrant call (animation cancel listener,
1378+
/// detached-from-window handler) sees a clean state and bails.
1379+
/// - Cancel SCVH traversals BEFORE removeView. The detach dispatch
1380+
/// itself can run framework cleanup that schedules a traversal;
1381+
/// starting from an empty queue narrows the post-cancel window.
1382+
/// - Use `removeViewImmediate` where possible so detach dispatch
1383+
/// runs inline and any layout requests it makes are short-circuited
1384+
/// by the freeze flag while we still own the SC.
1385+
/// - `safeReleaseScvh` runs `unscheduleScvhTraversals` again before
1386+
/// `host.release()` — defence in depth against any traversal that
1387+
/// slipped past the freeze (e.g. framework internals that don't
1388+
/// route through our `requestLayout` override).
1389+
///
1390+
/// The `coverDetaching` flag protects against re-entrance from any of
1391+
/// the synchronous dispatch points above.
12581392
private fun detachCoverView() {
1393+
if (coverDetaching) return
12591394
val view = coverView ?: return
1260-
scvhAnimator?.cancel()
1261-
scvhAnimator = null
1262-
view.animate().cancel()
1263-
view.animate().setListener(null)
1264-
val activity = coverHostActivityRef?.get()
1395+
coverDetaching = true
12651396
try {
1266-
activity?.windowManager?.removeView(view)
1267-
} catch (_: IllegalArgumentException) {
1268-
// Panel was already detached (e.g. host activity finished).
1397+
// Snapshot to locals BEFORE clearing shared state. A re-entrant
1398+
// call that arrives mid-teardown will see the cleared fields,
1399+
// hit the early return at the top, and won't try to double-remove
1400+
// or double-release the same view / host.
1401+
val host = scvhHost
1402+
val activity = coverHostActivityRef?.get()
1403+
val content = coverContent
1404+
1405+
// Freeze the content view first so any `requestLayout` fired
1406+
// during the rest of teardown (detach dispatch, animation
1407+
// cancellation, focus changes) is a no-op and can't schedule a
1408+
// new Choreographer traversal on the SCVH's WindowlessViewRoot.
1409+
// Only meaningful on the SCVH path; on the legacy path the
1410+
// content view IS the cover window root and there's no WWM
1411+
// relayout race to defend against, but freezing is harmless
1412+
// (the view is being discarded anyway).
1413+
(content as? FreezableFrameLayout)?.frozen = true
1414+
1415+
scvhAnimator?.cancel()
1416+
scvhAnimator = null
1417+
view.animate().cancel()
1418+
view.animate().setListener(null)
1419+
1420+
// Clear shared state now — re-entrant detach calls return at the
1421+
// top, listeners that reach into `coverView`/`scvhHost` see null.
1422+
coverView = null
1423+
coverContent = null
1424+
coverHostActivityRef = null
1425+
coverAttachedToken = null
1426+
coverSurfaceControl = null
1427+
scvhHost = null
1428+
scvhSurfaceControl = null
1429+
scvhAlphaState = 0f
1430+
1431+
// Cancel pending traversals on the SCVH's internal ViewRootImpl
1432+
// BEFORE removing the enclosing SurfaceView. Removing the parent
1433+
// dispatches detached-from-window up the cover content tree
1434+
// (focus loss, hover exit, accessibility events), each of which
1435+
// can `requestLayout` despite the freeze on some Android builds
1436+
// (e.g. framework-internal `forceLayout` paths that bypass our
1437+
// override). Starting from a clear Choreographer queue narrows
1438+
// the post-removal blast radius. The reflective probe latches
1439+
// off on permanent failure, so the second call below is cheap.
1440+
if (host != null) unscheduleScvhTraversals(host)
1441+
1442+
// Prefer `removeViewImmediate` so dispatchDetachedFromWindow runs
1443+
// inline. With the freeze in place, any layout requests it makes
1444+
// are short-circuited; with async `removeView`, the dispatch
1445+
// happens at a later vsync and the freeze must still hold then.
1446+
// Fall back to async `removeView` on OEM WindowManager impls
1447+
// where immediate removal throws (some Samsung / MIUI builds
1448+
// reject removeViewImmediate when the view is in a transitional
1449+
// state).
1450+
if (view.windowToken != null) {
1451+
val removed = try {
1452+
activity?.windowManager?.removeViewImmediate(view)
1453+
true
1454+
} catch (e: Throwable) {
1455+
Log.w(TAG, "detachCoverView: removeViewImmediate failed (${e.javaClass.simpleName}): ${e.message}; falling back to removeView")
1456+
false
1457+
}
1458+
if (!removed) {
1459+
try {
1460+
activity?.windowManager?.removeView(view)
1461+
} catch (_: IllegalArgumentException) {
1462+
// Panel already detached (host activity finished, etc.).
1463+
} catch (e: Throwable) {
1464+
Log.w(TAG, "detachCoverView: removeView failed: $e")
1465+
}
1466+
}
1467+
}
1468+
1469+
// Release the SCVH AFTER the SurfaceView is detached. The
1470+
// SurfacePackage was reparented into the SurfaceView; with the
1471+
// SurfaceView gone, releasing the host lets SurfaceFlinger
1472+
// reclaim the SC cleanly. `safeReleaseScvh` re-runs the
1473+
// unschedule helper internally — that's deliberate, it catches
1474+
// any traversal queued by the detach dispatch we just ran.
1475+
if (host != null) {
1476+
try {
1477+
safeReleaseScvh(host)
1478+
} catch (e: Throwable) {
1479+
// `safeReleaseScvh` already swallows; double-guard in case
1480+
// a future refactor lets something escape.
1481+
Log.w(TAG, "detachCoverView: safeReleaseScvh threw: $e")
1482+
}
1483+
}
1484+
} finally {
1485+
coverDetaching = false
12691486
}
1270-
// Release the SCVH host AFTER removeView. SCVH's SurfacePackage
1271-
// was reparented into the SurfaceView, which is now detached, so
1272-
// it's safe to tear down the host and let SF reclaim the SC.
1273-
scvhHost?.let { host -> safeReleaseScvh(host) }
1274-
scvhHost = null
1275-
scvhSurfaceControl = null
1276-
scvhAlphaState = 0f
1277-
coverView = null
1278-
coverContent = null
1279-
coverHostActivityRef = null
1280-
coverAttachedToken = null
1281-
coverSurfaceControl = null
12821487
}
12831488

12841489
/// Apply alpha to the cover's `SurfaceControl` directly via a
@@ -1318,7 +1523,7 @@ class HybridCover : HybridCoverSpec() {
13181523
}
13191524

13201525
private fun buildCoverView(activity: Activity): View {
1321-
val container = FrameLayout(activity).apply {
1526+
val container = FreezableFrameLayout(activity).apply {
13221527
isClickable = true
13231528
isFocusable = true
13241529
contentDescription = COVER_LABEL

0 commit comments

Comments
 (0)