Skip to content

cmux NEXT browser: a page under a hidden ancestor takes no focus (focus loop hang) - #27873

Merged
austinywang merged 4 commits into
feat-cmux-nextfrom
feat-cmux-next-focus-loop-hang
Oct 30, 2026
Merged

austinywang merged 4 commits into
feat-cmux-nextfrom
feat-cmux-next-focus-loop-hang

Conversation

@austinywang

@austinywang austinywang commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #27869

What

cmux NEXT nightly 1.0.0-nightly.4093446036501 hung forever after a workspace switch while a Chromium page had the keyboard. The main thread ran at ~95% CPU inside a single display-link callback and never recovered.

The loop:

  1. An ancestor of the page's view is hidden.
  2. Chromium orders the page window out inside AppKit's hide walk, before CEFTabContentView.viewDidHide conceals the page.
  3. The cmux window becomes key, and ChildPageFocus.keysReturned re-sends the focus target.
  4. CEFTab.setFocused(true) still sees the page as shown (visibleTab === self && !isOccluded). It calls cmux_shim_set_focus, which orders the page window front again.
  5. This repeats inside one FocusCoordinator.send drain. AppKit logged about 57 page-window key changes a second.

Fix

CEFTab now has one isShown check, used by setFocused, grantPendingFocusIfShown and attach. It also requires the page's container to be in a window with no hidden ancestor. That check reads each ancestor's own isHidden, not isHiddenOrHasHiddenAncestor, because AppKit updates that flag view by view during the hide walk. A focus request made in the middle of a hide becomes pendingFocus, and the existing viewDidUnhide → contentDidAppear → present → grantPendingFocusIfShown path grants it when the page shows again. Outside a hide walk, visibleTab === self already implies both new conditions, so behavior elsewhere doesn't change.

Tests

CEFDeferredFocusTests gains two behavior tests at the CEFTab boundary, using the existing stand-in shim:

  • a view hidden in the same walk, beside the page, asks for focus;
  • the same request from inside the page's own view, which is where Chromium's view sits.

Each test expects no grant while hidden and exactly one grant on unhide.

Command: cmux-ci run --class light --script scripts/ci/package-test-lane.sh --arg=suite --arg=Packages/macOS/CmuxNext --arg=CEFDeferredFocusTests,ChildPageFocusKeyChangeTests

The base has a test-target compile red (RemoteViewVNCTests.swift:130, fixed by #27862). The fleet runs therefore use side branches that carry that one-file fix: side/focusloop-red (the test commit) and side/focusloop-green (the head).

Observed / Not run

  • Observed: the hang, from a live sample of the hung process, AppKit unified logs, and the onset time.
  • Observed: an independent correctness review subagent found nothing blocking. Its one test note led to the second test.
  • Red, at the test commit (side/focusloop-red @ 4c657921, step 8605113ebede759c818778d8): focusAskedWhileAnAncestorHidesIsGrantedWhenItShowsAgain failed, (FocusSpyShim.granted → [10]).isEmpty → false.
  • First fix, superseded (isHiddenOrHasHiddenAncestor, step 637312fcef3be5b699603669): the from-inside test passed but the sibling test stayed red. That led to the ancestor walk.
  • Green, at head 831f7135 (side/focusloop-green @ e582a896, step 46d97b373ffe4f6f779938a7): CEFDeferredFocusTests ran 5 tests and ChildPageFocusKeyChangeTests ran 11, all passing.
  • Not run: a tagged GUI repro of the workspace switch. No symbolicated stack either: the nightly's dSYM is in Sentry, which wasn't authenticated on this host, so the stack was matched to source by structure.

Follow-up (not in this PR)

FocusCoordinator.send's drain has no bound, and keysReturned can re-send for the same page more than once in one drain. Another cause of "Chromium hides a page window we think is shown" would loop the same way.

Changelog

Fixed: cmux NEXT no longer hangs when you switch workspaces while a browser page has the keyboard.

🤖 Generated with Claude Code

austinywang and others added 3 commits October 29, 2026 19:46
…cus loop beachball)

Red on the base: the grant runs while an ancestor hides the page.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ing a focus loop that hung the app

A workspace switch hid an ancestor of the focused Chromium page's view. Chromium ordered
the page window out inside AppKit's hide walk, before the page's own viewDidHide concealed
it. The cmux window took the keys, ChildPageFocus.keysReturned re-sent the target, and
CEFTab.setFocused still saw the page as shown (visibleTab, not occluded), so it granted
focus, which showed the page window again. The cycle repeated inside one FocusCoordinator
drain forever (NIGHTLY 1.0.0-nightly.4093446036501, ~57 key changes a second, beachball).

CEFTab's shown check now also requires its view in a window under no hidden ancestor. The
ask stays pending and the existing contentDidAppear -> grantPendingFocusIfShown path
grants it when the view shows again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…le an ancestor hides

Review note: pins the real topology (Chromium's view under the page container).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@austinywang austinywang added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 30, 2026
@austinywang

Copy link
Copy Markdown
Contributor Author

Correctness review (independent subagent, read-only) — head 3c9a7b7

Verdict: no blocking findings.

  1. Loop breaks: during AppKit's hide walk the ancestor's hidden flag is already set, so isShown is false. The request becomes pendingFocus, no cmux_shim_set_focus runs, and the drain empties. The request is replayed by viewDidUnhide → contentDidAppear → present → grantPendingFocusIfShown.
  2. No regressions found: outside a hide walk, visibleTab === self already implies the new conditions (present runs only for a visible container in a window, and conceal runs on hide or leaving the window).
    • Paths checked: attach, createDeferred, setContentVisible, parked tabs, the New Tab spare.
    • Stale requests are cleared by setFocused(false).
    • Gap that predates this PR: movePopupsIn and attachPopupWindowIfNeeded never replay pendingFocus.
  3. Test fidelity: the sibling-view test is red on base and green on head. In the app, Chromium's view sits under the page's own view, so the reviewer suggested a test for that layout. Resolved: focusAskedFromInsideThePageWhileAnAncestorHidesIsGrantedWhenItShowsAgain added.
  4. Low, follow-up: FocusCoordinator.send's drain is unbounded, and keysReturned has no guard against re-sending for the same page within one drain. Recorded in cmux NEXT: focus loop between cmux and a hidden Chromium page hangs the main thread after a workspace switch #27869; not in this PR.

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 3c9a7b7: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr27873 -f ref=feat-cmux-next-focus-loop-hang -f platform=macos -f backend_mode=local

Not verified: macOS jobs did not run: compile and app tests were skipped on 831f713557; ci-status is green without them.

CI passes on 831f713557 (run 40995037227 attempt 1).
macOS jobs did not run: compile and app tests were skipped: Fast static checks skipped (the macos job needs both to succeed). Re-run CI once they pass, before merging.

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's; yours means the failing file is one this PR changes, also red on main that main's latest full suite fails the same way, seen on other PRs that it failed on another pull request's run lately.

…-walk flag

The fleet run showed isHiddenOrHasHiddenAncestor still false for the page's container
while AppKit's hide walk had not reached it (a view hidden earlier in the walk asked for
focus): the sibling test stayed red. Each ancestor's isHidden is set before the walk.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@austinywang
austinywang merged commit 65c882f into feat-cmux-next Oct 30, 2026
59 of 65 checks passed
@austinywang
austinywang deleted the feat-cmux-next-focus-loop-hang branch October 30, 2026 03:03
@austinywang

Copy link
Copy Markdown
Contributor Author

Correctness re-review (independent subagent, read-only) — head 831f713

Verdict: SHIP. No blocking findings.

  1. Manual ancestor isHidden walk vs isHiddenOrHasHiddenAncestor: outside AppKit's hide and unhide walks, both read the same bits on the same chain.
    • During the hide walk, the new check reads hidden sooner. That is the fix.
    • During the unhide walk, it reads shown sooner, so it never blocks there.
    • Parked tabs: parking sets container.isHidden, which both checks see. Unparking clears it before viewDidUnhide, which calls contentDidAppear, then present, then grantPendingFocusIfShown, so the request is replayed.
    • A blocked request keeps pendingFocus, so nothing is lost.
  2. Cost: a few dozen pointer hops, only on focus, present and attach, never per frame.
  3. Minor, not blocking: CEFHostView.viewDidMoveToWindow and viewDidUnhide still use isHiddenOrHasHiddenAncestor. They only gate contentDidAppear, and are safe because AppKit clears the flag before the unhide walk.

Tests at this head (side branch carrying #27862 compile fix): CEFDeferredFocusTests ran 5 tests and ChildPageFocusKeyChangeTests ran 11, all passing (cmux-ci step 46d97b373ffe4f6f779938a7). The red at the test commit is in the PR description.

@austinywang austinywang added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Oct 30, 2026
@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 831f7135 Deployed Oct 30, 2026 by austinywang via cmux-tui tree preflight #79965
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant