Skip to content

Fix re-entrant exclusive-access crash in drag handle hit test - #771

Merged
lawrencecchen merged 1 commit into
mainfrom
fix-drag-handle-reentrant-crash
Mar 3, 2026
Merged

lawrencecchen merged 1 commit into
mainfrom
fix-drag-handle-reentrant-crash

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Mar 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #761.

SIGABRT crash caused by re-entrant calls to windowDragHandleShouldCaptureHit during the sibling hit-test walk. When sibling.hitTest() triggers a SwiftUI layout pass, AppKit calls back into the drag handle's hit resolution while the outer invocation is still walking siblings. This violates Swift's exclusive-access rules on SwiftUI view state.

The fix adds a module-level re-entrancy guard (_windowDragHandleIsResolvingSiblingHits) that bails out on nested calls to the sibling walk. Main-thread only, no lock needed.

Crash stack: DraggableView.hitTest -> windowDragHandleShouldCaptureHit -> sibling.hitTest -> SwiftUI body eval -> hitTest (re-entry) -> exclusive-access violation -> SIGABRT.

Test plan

  • Added testDragHandleSiblingHitTestReentrancyDoesNotCrash regression test that simulates the re-entrant call path
  • Existing drag handle tests continue to pass (same 4 pre-existing failures on main)
  • Reproduced on macOS Sequoia 15.1.1 VM, deployed fix, verified crash no longer occurs

When sibling.hitTest() triggers a SwiftUI layout pass during the
drag handle's sibling walk, AppKit can call back into
windowDragHandleShouldCaptureHit before the outer invocation
finishes. This re-entry accesses SwiftUI view state that is already
held exclusively, causing a Swift runtime SIGABRT.

Add a module-level re-entrancy guard that bails out (returns false)
on nested calls to the sibling walk. Since hitTest is always called
on the main thread, a simple Bool flag is sufficient.

Crash was reproduced on macOS Sequoia 15.1.1 (24B91) in a UTM VM.
The crash stack: DraggableView.hitTest -> windowDragHandleShouldCaptureHit
-> sibling.hitTest -> SwiftUI body evaluation -> hitTest (re-entry)
-> exclusive-access violation -> SIGABRT.
@vercel

vercel Bot commented Mar 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Building Building Preview, Comment Mar 3, 2026 3:01am

@coderabbitai

coderabbitai Bot commented Mar 3, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b3f6f8c and 568da85.

📒 Files selected for processing (2)
  • Sources/WindowDragHandleView.swift
  • cmuxTests/CmuxWebViewKeyEquivalentTests.swift

📝 Walkthrough

Walkthrough

A re-entrancy guard mechanism is added to WindowDragHandleView to prevent SwiftUI from re-entering the sibling-hit testing path, with an accompanying test that verifies the guard prevents crashes during re-entrant scenarios.

Changes

Cohort / File(s) Summary
Re-entrancy Guard
Sources/WindowDragHandleView.swift
Introduces private boolean flag _windowDragHandleIsResolvingSiblingHits to block re-entrant hit-test calls. Guard is set before sibling-walk begins and reset via defer. Early return with DEBUG log if re-entrancy detected.
Re-entrancy Test
cmuxTests/CmuxWebViewKeyEquivalentTests.swift
Adds test helper class ReentrantSiblingView and test testDragHandleSiblingHitTestReentrancyDoesNotCrash to verify re-entrancy guard prevents crashes and yields false result during re-entrant hit-test attempts.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Poem

🐰 A guard stands tall at the gate so wise,
Preventing re-entry before SwiftUI cries,
With defer in hand, it resets with grace,
No crashes shall happen in this sacred space! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'Fix re-entrant exclusive-access crash in drag handle hit test' clearly and accurately describes the main change: fixing a re-entrancy crash in the drag handle hit-testing logic.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-drag-handle-reentrant-crash

Comment @coderabbitai help to get the list of available commands and usage tips.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

@greptile-apps

greptile-apps Bot commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixed a SIGABRT crash on macOS Sequoia caused by re-entrant calls to windowDragHandleShouldCaptureHit during sibling hit-test walks. When sibling.hitTest() triggers SwiftUI layout, AppKit can call back into the drag handle's hit resolution before the outer invocation completes, violating Swift's exclusive-access rules.

  • Added _windowDragHandleIsResolvingSiblingHits guard flag set before the sibling walk
  • Re-entrant calls now bail out early (return false) instead of crashing
  • Used defer to ensure flag is always reset, even on early returns
  • Added ReentrantSiblingView test helper and regression test validating the fix

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • Focused fix for a specific crash scenario with clear root cause analysis, proper guard implementation using defer pattern, and comprehensive regression test coverage that validates both outer and re-entrant call behavior
  • No files require special attention

Important Files Changed

Filename Overview
Sources/WindowDragHandleView.swift Added module-level re-entrancy guard to prevent SIGABRT crash when sibling hit-test triggers SwiftUI layout
cmuxTests/CmuxWebViewKeyEquivalentTests.swift Added regression test simulating re-entrant call path that previously caused crash

Sequence Diagram

sequenceDiagram
    participant User
    participant DraggableView
    participant Guard as Re-entrancy Guard
    participant Function as windowDragHandleShouldCaptureHit
    participant Sibling as ReentrantSiblingView
    
    User->>DraggableView: Mouse down event
    DraggableView->>Function: hitTest(point)
    Function->>Guard: Check _windowDragHandleIsResolvingSiblingHits
    Guard-->>Function: false (not set)
    Function->>Guard: Set flag = true
    
    Note over Function: Start sibling walk
    Function->>Sibling: hitTest(pointInSibling)
    
    Note over Sibling: Triggers SwiftUI layout
    Sibling->>Function: Re-entrant call (same drag handle)
    Function->>Guard: Check _windowDragHandleIsResolvingSiblingHits
    Guard-->>Function: true (already set!)
    Function-->>Sibling: Return false (bail out, no crash)
    
    Sibling-->>Function: Return nil
    Note over Function: Continue sibling walk
    Function->>Guard: defer resets flag = false
    Function-->>DraggableView: Return true (capture hit)
Loading

Last reviewed commit: 568da85

@lawrencecchen
lawrencecchen merged commit 5bbdd87 into main Mar 3, 2026
12 checks passed
@lawrencecchen
lawrencecchen deleted the fix-drag-handle-reentrant-crash branch March 3, 2026 03:20
swannysec added a commit to swannysec/crux that referenced this pull request Mar 3, 2026
swannysec added a commit to swannysec/crux that referenced this pull request Mar 3, 2026
bn-l pushed a commit to bn-l/cmux that referenced this pull request Apr 3, 2026
…ow-ai#771)

When sibling.hitTest() triggers a SwiftUI layout pass during the
drag handle's sibling walk, AppKit can call back into
windowDragHandleShouldCaptureHit before the outer invocation
finishes. This re-entry accesses SwiftUI view state that is already
held exclusively, causing a Swift runtime SIGABRT.

Add a module-level re-entrancy guard that bails out (returns false)
on nested calls to the sibling walk. Since hitTest is always called
on the main thread, a simple Bool flag is sufficient.

Crash was reproduced on macOS Sequoia 15.1.1 (24B91) in a UTM VM.
The crash stack: DraggableView.hitTest -> windowDragHandleShouldCaptureHit
-> sibling.hitTest -> SwiftUI body evaluation -> hitTest (re-entry)
-> exclusive-access violation -> SIGABRT.

This branch was successfully deployed

1 active deployment
Preview — 568da856 Deployed Mar 3, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cmux quietly closes and disappears after a few seconds

1 participant