fix(native): reject stale event handles - #114
Conversation
Tag event handles by render generation so callbacks cannot resolve against a replacement table. Copy event tags while their native environment remains protected.
|
Adversarially reviewed with physical-device A/B against master. Request changes — the native mechanism is correct and its thesis is proven on hardware, but two consequences make it unreleasable as-is. What's proven right: generation scheme is sound (23-bit gen / 8-bit slot, always-positive, decode rejects ≤0, safe wraparound), bump/commit points are atomic under one tap_mutex hold with clear_taps→register→set_root serialized through Sender.commit, tag copy happens under the registry lock with defer-freed delivery envs on every path (UAF closed, no leak), and every tap-family sender resolves through the generation check. On the Moto, a 100-tap storm during ~2000 renders: master delivered 71 correct with 29 misroutes to wrong handlers; this PR delivered 62 with zero misroutes. That's the fix working. 1332 tests, 4/4 zig codec tests, all gates clean, based on the 0.7.37 tag. Blocking 1 — breaks the generated bridge's handle-stability contract. mob_new's MobBridge.kt.eex:295-302 keys lazyListStates by the on_end_reached handle, documented as 'stable within a screen', consumed via remember(handle). Generation bits change the handle every render: device A/B shows master retains scroll offset {0,40} across a re-render, this build resets to {0,0} after ONE re-render — plus one leaked LazyListState per re-render per list (map cleared only on navigation). mob_new #43 does not touch these lines. Needs a companion mob_new fix (key by stable :id) or an explicit slot-extraction contract, released in lockstep. Blocking 2 — silent drops include typed keystrokes. Controlled TextField round-trip with NO external ticks: typed 'hello', field and assigns show 'hell' (4/5 changes) — typing triggers the re-render that stales the next in-flight change event. Master: 5/5. Dropping rare misroutable taps is the right trade; dropping common-path text input is a UX regression. The change_str family likely needs stale-tolerant handling (e.g. same-slot-same-tag acceptance, or re-resolution by stable key) rather than blanket rejection — design call for the author. Major: (a) rejection is completely invisible — zero log lines anywhere in the diff or logcat; a debug-level trace in resolveActiveTapLocked is the difference between diagnosable and haunted; (b) component events remain outside the scheme — mob_send_component_event still bare-slot-indexes the 256-slot pool, and with #111's lowest-free-slot reuse an in-flight event for a reclaimed slot delivers to the slot's new owner — the exact race class this PR's title claims to close; (c) iOS has BOTH races unfixed (bare slot handles in mob_send_tap ~:209-226; enif_make_copy after unlock in mob_send_change ~:584-592) and the divergence is undocumented — parity rule requires at least a docs note + tracking issue. Minor: no CHANGELOG entry; gen-0 register silently returns the sentinel (worth an assert). Full device tables and file:line detail available; happy to re-review the revision — the storm numbers say this is worth landing once the contract break and the keystroke path are resolved. |
Accept in-flight change events only when their prior and current registrations match. Extend generation checks to iOS and persistent components.
GenericJam
changed the title
|
Addressed in 083451b and companion GenericJam/mob_new#44.
Verification is green across the full runtime and generator suites, native Android and iOS compilation, Kotlin generation compilation, codec tests, static checks, and a fresh adversarial rereview. Ready for another device pass. p.p. Claude |
|
Re-reviewed the revision with the prior findings as the verification matrix, paired with mob_new #44 on a physical Moto + iOS sim. All seven prior findings are genuinely fixed — highlights: keystrokes 5/5 idle and 10/10 under a 50ms storm on BOTH platforms (was 'hello'→'hell'); storm continuity 74/100 delivered vs 62 last round, zero misroutes both rounds, and every rejection now visible in logcat; component handles generation-tagged with stale→badarg and clean 256-exhaustion degrade; iOS at 92/100 under storm with zero wrong-handler dispatches and the #80 honesty matrix intact; lock ordering audit clean (env allocated before mutex, no nesting, settle counter honest). Gates: 1337 tests, 4/4 codec, all format/lint clean. Request changes — the revision introduced two new blockers: N1 (both platforms): build-window identity confusion in the change-acceptance path. nif_clear_taps advances tap_build_generation but never invalidates the building table's generation label (android/jni/mob_nif.zig:1049-1057 + ~:1713; ios/mob_nif.m:145-156 + ~:2268), so during the entire clear→register→set_root window resolveGenerationTapLocked can match a gen N−1 handle against the N+1 table being refilled — accepting exactly the input-A-removed/input-B-same-slot case the acceptance rule forbids, delivering A's in-flight text into B. Recurs every frame under a tick storm plus typing. One-line fix per platform: in nif_clear_taps, set the building table's generation label to 0 under the mutex (decode already rejects gen 0, making the building table unmatchable). N2 (device-proven on Android; iOS identical by construction): legitimate sheet dismissals are dropped under concurrent re-render, permanently desyncing state. ModalBottomSheet.onDismissRequest fires post-animation with a handle from an earlier composition; any commit landing mid-gesture makes it stale and the strict path drops it. Moto proof: a single 300ms tick overlapping the swipe → dismiss never delivered, logcat shows the rejection, the app's :show assign stays set while the sheet is visually gone — permanent desync (master delivered it). Fix direction: extend the pid+tag acceptance to on_dismiss (it's an identity-preserving user intent exactly like a change), or re-resolve the current handle native-side at dispatch. Note the class: any animation-deferred event has this exposure; immediate events only lose deliveries under storms, which is the intended trade. Verified no new holes elsewhere: the acceptance is scoped strictly to the change family (grep-verified — stale scroll/gesture/tap/dismiss cannot ride it), which is also why N2 exists. Changelog: structure is good; before release widen 'text, toggle, and slider' to the real change family (tab-select rides mob_send_change_str), and the 'only when identical PID and tag' sentence becomes true only after N1. Nit: stale deregister_component now badargs where it was silently ok — safe in-tree, worth a line in the notes. #44 is approved and shipping independently (new-template+old-mob degrades cleanly). Holding mob 0.7.38 for N1+N2 — both small native fixes; on the next push I'll re-run the sheet-under-tick and A/B-window probes specifically. |
Hide handler tables while they are rebuilt and allow identity-matched sheet dismissals to survive a concurrent render.
|
Addressed both follow-up blockers in 9c393cd.
TDD contract tests failed before the fix and now pass 12/12. The full suite passes 1,339 tests; both native targets compile; codec, format, static-analysis, warnings-as-errors, and diff gates pass. A fresh adversarial rereview approved the revision with no P0-P3 findings. Ready for the requested A/B-window and sheet-under-tick device probes. p.p. Claude |
|
Re-checked 9c393cd on the physical Moto with the round-2 probe matrix. N1 is fixed — verified in code on both platforms (building-table label zeroed under tap_mutex in clear_taps, restored only at the set_root commit; decode rejects gen 0; register/commit paths unaffected) and on device: A/B same-slot swap under storm+typing produced fully disjoint buffers, wrong=%{}, rejections logged. The changelog's acceptance sentence is now true, the diff is exactly scoped (+70/−8, nothing unrelated), and collateral is green: 1339 tests, 12/12 source-contract, storm 65/100 with zero misroutes and 100/100 accounted, keystrokes idle 5/5. Request changes — N2 is fixed for the common case but the fast-storm case still fails deterministically. Dismiss cases: (a) plain PASS, (b) 300ms tick overlapping the swipe PASS — delivered exactly once, show cleared (this was the round-2 blocker), (d) replaced-sheet stale dismissal still correctly a no-op (the #43 invariant holds). But (c) dismiss under a 60ms re-render storm: 3/3 deterministic FAIL — dA stays 0, one rejection logged, :show latched while the sheet is visually gone, permanent desync. Root cause: the pid+tag acceptance can cross exactly ONE render (only two tables exist), and an animation-deferred dismissHandle at fast cadence lags ≥2 generations, so no table matches; mob_new 0.4.30's MobSheet then latches visible=false and never re-presents, making any rejected dismissal permanent by construction. Fair note: this wasn't introduced by 9c393cd (dismiss was fully strict at 083451b), but on master the slot-resolved dismissal delivered, so vs master it remains a regression class and it's the last blocker. Recommended fix: native-side re-resolution at dispatch for identity events — when the generation match fails for the change/dismiss family, resolve the CURRENT registration at the same slot and accept iff pid+tag match it (this generalizes the existing acceptance from 'one render back' to 'any lag', keeps templates dumb, and preserves strict rejection when the registration genuinely changed — case (d) stays a no-op because the replacement's tag differs). The alternative — template-side acknowledge/re-present on rejected dismissal — pushes protocol into every generated app and still leaves the window visible to users as a flicker; I'd avoid it. One small notes-line to add either way: N1's zeroing slightly widens the under-storm rejection window for one-gen-old changes (~0–2 per 10 chars at 50ms cadence) — intended trade, worth stating. Everything else on this head is done — on the next push the re-check is just case (c) plus a spot-check that (d) still rejects. |
|
Fixed in 4bffc81. Each slot now carries the first generation in its continuous run of identical PID/tag registrations; commit-time propagation extends that range only while the route remains unchanged, so delayed change/dismiss callbacks can cross any number of renders while replacement handlers still reject. Registration now allocates and initializes tag state before publishing the slot count on both platforms. Verified with 1,340 ExUnit tests, 13/13 source contracts, 5/5 codec tests, Android aarch64 and iOS simulator object builds, native formatting, strict Credo, and warnings-as-errors; fresh adversarial re-review found no remaining P0-P3. Ready for fast-storm case (c) and replacement case (d) device probes. p.p. Claude |
|
Final re-check of 4bffc81 on the physical Moto. Approve — merging and releasing as 0.7.38. The run-generation mechanism is correct: run-start stamped per slot at registration, commit-time propagation under tap_mutex with enif_compare on the tag term, and runs are provably contiguous — propagation is bounded by the previous committed table, so a gap or a different occupant always resets the run (verified on device: a gap-straddling handle was explicitly rejected, and the analysis shows that reset is necessary, not just conservative — a gap-spanning run could misroute if the gap generation held a different handler). Predicate boundaries are wrap-aware with gen-0 rejected and the building-table zeroing intact; the only residual is 23-bit aliasing after ~8.4M consecutive identical renders, which is a pre-existing and practically unreachable class. Scope is unchanged: identity acceptance remains change-family + dismiss only. Device: dismiss under 60ms storm delivered exactly once, three from three, plus a 25ms-cadence margin run; replaced-sheet dismissal still a no-op across three swipe timings; 3+ seconds of continuous typing under storm spanned 147 generations at 32/32 with zero cross-buffer; storm taps 70/100 with zero misroutes and 100/100 accounted; storm typing now 10/10 with zero rejections — the one-gen-old losses from the previous round are gone, a measurable dividend of the run design. Suite 1340/0, codec 5/5 incl. the new wraparound/reset test, contract tests now pin the run logic and the alloc-before-publish ordering, all format/lint gates clean, cleanly based on the 0.7.37 tag. One doc note, non-gating: the publication-order change is real and correct (it fixes the alloc-failure half-written-slot path and makes count-covers-initialized-slots load-bearing for the propagation loop) but everything was already under tap_mutex, so the 'memory-ordering window' framing in the commit message overstates — worth a wording tweak someday, not worth a round trip. Thanks for a clean three-round convergence — the storm numbers from round one to now: 29% misroutes → zero, with input loss going from silent to logged to eliminated for identity events. |
* fix(android): reject stale event handles Tag event handles by render generation so callbacks cannot resolve against a replacement table. Copy event tags while their native environment remains protected. * fix(native): preserve event identity Accept in-flight change events only when their prior and current registrations match. Extend generation checks to iOS and persistent components. * fix(native): preserve delayed event identity Hide handler tables while they are rebuilt and allow identity-matched sheet dismissals to survive a concurrent render. * fix(native): preserve identity across renders
1 participant
Summary
Generated Android projects pair this with the released generator correction so lazy-list state remains stable when event generations change.
Testing