Does this issue occur when all extensions are disabled?: Yes
- VS Code Version: 1.120.0 (main branch, commit 89fc639)
- OS Version: All platforms
Bug
Multiple locations in the codebase use the pattern array.splice(array.indexOf(item), 1) without checking whether indexOf returned -1. When the element is not found, indexOf returns -1, and splice(-1, 1) silently removes the last element of the array instead of being a no-op.
This causes incorrect behavior: the wrong keyboard layout, activity badge, progress indicator, or settings entry is silently removed.
Affected locations
| File |
Method |
Impact |
keyboardLayoutService.ts:116-119 |
removeKeyboardLayout |
Removes wrong layout from MRU if called with stale ref |
activityService.ts:95, 158 |
showViewContainerActivity / showActivity dispose callback |
Removes wrong activity badge on double dispose |
progressService.ts:135 |
withWindowProgress finally block |
Removes wrong progress task if promise resolved before delay |
settingsTree.ts:2871 |
ApplySettingToAllProfilesAction.run |
Removes wrong setting from all-profiles list (TOCTOU between this.checked and fresh getValue()) |
Steps to Reproduce
activityService.ts (double dispose scenario):
- Register a view container activity via
showViewContainerActivity
- Store the returned
IDisposable
- Call
.dispose() on it twice
- On the second call,
indexOf returns -1 because the activity was already removed
splice(-1, 1) removes the last activity in the array (belonging to a different view container)
- The wrong activity badge disappears from the UI
settingsTree.ts (TOCTOU scenario):
- Open Settings UI
- Toggle "Apply Setting to All Profiles" for a setting
- If another VS Code window or process removes that setting from the
configurationService.allProfiles list between the this.checked guard (set via config listener) and the getValue() call, indexOf returns -1
splice(-1, 1) removes the last entry in the all-profiles list (a different setting)
Fix
A PR is available: #314635
The fix adds index !== -1 guards before each splice call. This is a standard defensive pattern with zero cost.
Does this issue occur when all extensions are disabled?: Yes
Bug
Multiple locations in the codebase use the pattern
array.splice(array.indexOf(item), 1)without checking whetherindexOfreturned-1. When the element is not found,indexOfreturns-1, andsplice(-1, 1)silently removes the last element of the array instead of being a no-op.This causes incorrect behavior: the wrong keyboard layout, activity badge, progress indicator, or settings entry is silently removed.
Affected locations
keyboardLayoutService.ts:116-119removeKeyboardLayoutactivityService.ts:95, 158showViewContainerActivity/showActivitydispose callbackprogressService.ts:135withWindowProgressfinally blocksettingsTree.ts:2871ApplySettingToAllProfilesAction.runthis.checkedand freshgetValue())Steps to Reproduce
activityService.ts(double dispose scenario):showViewContainerActivityIDisposable.dispose()on it twiceindexOfreturns-1because the activity was already removedsplice(-1, 1)removes the last activity in the array (belonging to a different view container)settingsTree.ts(TOCTOU scenario):configurationService.allProfileslist between thethis.checkedguard (set via config listener) and thegetValue()call,indexOfreturns-1splice(-1, 1)removes the last entry in the all-profiles list (a different setting)Fix
A PR is available: #314635
The fix adds
index !== -1guards before eachsplicecall. This is a standard defensive pattern with zero cost.