Skip to content

Commit 9117c5c

Browse files
author
Tajudeen
committed
Fix menubar state machine bug: prevent OPEN state without focusedMenu
- Add validation in focusState setter to ensure focusedMenu is set before allowing OPEN state - Prevents crash when accessing this.focusedMenu.index in showCustomMenu - Add defensive checks as safety net (defense in depth) - Fixes Windows-only bug where clicking File/Edit/etc menus would crash - Maintains Windows-specific stacking context logic (document.body appendChild) Root cause: focusState setter allowed OPEN state transition without validating that focusedMenu was set, creating inconsistent state machine state.
1 parent eec119b commit 9117c5c

File tree

1 file changed

+26
-5
lines changed

1 file changed

+26
-5
lines changed

src/vs/base/browser/ui/menu/menubar.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -677,12 +677,23 @@ export class MenuBar extends Disposable {
677677
return;
678678
}
679679

680+
// CRITICAL FIX: Validate state machine integrity before allowing OPEN state
681+
// If transitioning to OPEN, focusedMenu MUST be set. This prevents the bug where
682+
// focusState is OPEN but focusedMenu is undefined, causing crashes when accessing
683+
// this.focusedMenu.index in showCustomMenu.
684+
if (value === MenubarState.OPEN && !this.focusedMenu) {
685+
// State machine integrity violation: cannot open menu without focusedMenu
686+
// This should never happen if all code paths set focusedMenu before setting focusState to OPEN
687+
// However, as a defensive measure, we'll log and prevent the invalid state transition
688+
console.warn('MenuBar: Attempted to set focusState to OPEN without focusedMenu. This indicates a state machine bug.');
689+
// Don't allow the invalid state transition - return early
690+
return;
691+
}
692+
680693
const isVisible = this.isVisible;
681694
const isOpen = this.isOpen;
682695
const isFocused = this.isFocused;
683696

684-
this._focusState = value;
685-
686697
switch (value) {
687698
case MenubarState.HIDDEN:
688699
if (isVisible) {
@@ -759,13 +770,23 @@ export class MenuBar extends Disposable {
759770
this.showMenubar();
760771
}
761772

762-
if (this.focusedMenu) {
763-
this.cleanupCustomMenu();
764-
this.showCustomMenu(this.focusedMenu.index, this.openedViaKeyboard);
773+
// CRITICAL FIX: At this point, focusedMenu is guaranteed to be set due to validation above
774+
// However, add an additional defensive check as a safety net (defense in depth)
775+
if (!this.focusedMenu) {
776+
console.error('MenuBar: focusState set to OPEN but focusedMenu is undefined. This should be impossible after validation.');
777+
// Reset to FOCUSED state to maintain state machine integrity
778+
// Don't set _focusState to OPEN, keep it at current state
779+
this._focusState = this._focusState; // Keep current state
780+
return;
765781
}
782+
783+
this.cleanupCustomMenu();
784+
this.showCustomMenu(this.focusedMenu.index, this.openedViaKeyboard);
766785
break;
767786
}
768787

788+
// Set the state AFTER all switch cases have been processed
789+
// This ensures state is only updated if all validations pass
769790
this._focusState = value;
770791
this._onFocusStateChange.fire(this.focusState >= MenubarState.FOCUSED);
771792
}

0 commit comments

Comments
 (0)