fix: stale notification blocks + unified surface teardown (mechanism 2) - #258
Merged
Conversation
NotificationCenter.removeObserver does not cancel a block already handed off to the main OperationQueue. During rapid window create/close, viewDidMoveToWindow can tear down and re-register windowObservers for a new window while a block registered against the previous window is still sitting in the main queue; when it runs, it now no-ops instead of acting on stale state. Each addObserver(queue: .main) block in viewDidMoveToWindow now captures the generation live at its own registration and checks it against the instance's current generation before doing anything. Live (current) blocks always match, so #241 occlusion-heal semantics on didBecomeKey / didChangeOcclusionState are unaffected.
teardownSurface() and deinit each had their own copy of the snapshot-callback-context / snapshot-tap-context / nil-surface / free-on-deferred-Task logic. Unify them into a single performSurfaceTeardown(reason:) that snapshots and nils every piece of handoff state a second call could act on (surface, surfaceCallbackContext, outputTapContext, reviveDescriptor), so calling it from either site can never double-free or double-release: a second call sees surface == nil and no-ops. Also extends the reviveDescriptor fd leak-guard (previously deinit-only) to teardownSurface(), now nil-ing it after close() so a repeat call is safe. liveSurfaceForGhosttyAccess(reason:), releaseSurfaceForTesting(), and replaceSurfaceWithFreedPointerForTesting() are intentionally not folded in — each has real semantic differences (documented inline) that would be lost by unification: not freeing a possibly-reowned pointer, keeping portalLifecycleState live for a recreate-after-release test fixture, and deliberately leaving a dangling surface pointer to simulate an out-of-band free.
Same rapid create/close shape covers the windowObserverGeneration guard in GhosttySurfaceScrollView (a second, distinct teardown-race mechanism from the callback-context race this test already targets). Making the exact NotificationCenter enqueue race deterministic would need a dedicated seam into GhosttySurfaceScrollView's private observer bookkeeping that doesn't exist yet, so per policy this bumps the existing tripwire's iteration count modestly instead of adding a new seam-dependent test.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Second half of the teardown-race fix. #254 closed the IO-thread callback-context path; the re-land gate (#257) then caught a second mechanism, root-caused from 7 CI minidumps at address level: 6/7 crashes at one byte-identical call site, main thread, ARC op on reused memory, delivered via a queue:.main NotificationCenter block — one fault address literally contained ASCII 'ndowObse' from the reused windowObserver memory. Foundation's documented gotcha: removeObserver does not cancel a block already handed to the main OperationQueue, so during rapid window teardown a stale block runs against reused memory.
Fixes:
Local repro: 8/8 clean before AND after (the race never fired locally — visible windows, different timing). The proof is #257's 3×-green gate on CI's always-occluded environment, which re-runs with this fix included immediately after this merges.
Test plan