fix: guard selectDropDownContainer.remove() against already-removed node (fixes #315703)#315709
Open
vs-code-engineering[bot] wants to merge 1 commit into
Open
Conversation
…ode (fixes #315703) The dispose callback in selectBoxCustom calls .remove() on the dropdown container, but a browser blur event (triggered by scroll in the settings editor) can remove the node first. Add a parentNode guard to prevent the NotFoundError. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
🔧 Error Fix
Summary
Error:
NotFoundError: Failed to execute 'remove' on 'Element': The node to be removed is no longer a child of this node.Bucket:
5dab52a4-d055-fd3c-d16c-a042f3d75688The settings editor triggers
cancelSuggesters()on scroll viaupdateTreeScrollSync(). This hides the context view, which disposes the select box dropdown. The dispose callback callsselectDropDownContainer.remove(). However, a browser blur event (triggered by the same scroll action) can remove the DOM node before the dispose callback runs, causing the NotFoundError.Fixes #315703
Recommended reviewer:
@mrleemurrayCulprit Commit
No specific culprit commit identified. This is a long-standing race condition between browser blur events and the dispose lifecycle of the select box dropdown.
Code Flow
sequenceDiagram participant SE as settingsEditor2 participant ST as settingsTree participant CV as contextViewService participant SB as selectBoxCustom participant DOM as Browser DOM SE->>ST: cancelSuggesters() ST->>CV: hideContextView() CV->>SB: dispose() Note over DOM: blur event already removed the node SB->>DOM: selectDropDownContainer.remove() DOM-->>SB: NotFoundError!Affected Files
src/vs/base/browser/ui/selectBox/selectBoxCustom.tssrc/vs/workbench/contrib/preferences/browser/settingsEditor2.tssrc/vs/workbench/contrib/preferences/browser/settingsTree.tsHow the Fix Works
Chosen approach (
src/vs/base/browser/ui/selectBox/selectBoxCustom.ts:531): Added aparentNodeguard before calling.remove(). The producer of the invalid state is the browser blur event handler which removes the DOM node before our dispose runs. We cannot prevent the browser behavior, so a guard clause is the correct fix - it prevents the error without hiding any bug.Recommended Owner
@mrleemurray- recent owner of selectBoxCustom.tserrors-fix-driver — cycle 25
Trigger: cron_review_comments · Head:
4a85d016b2b0978c04890e5b677dd86f2d74135f(4a85d01)blocked(awaiting approval)Push: no — no code changes needed · Copilot rerequested: no (no push)
Ready gate: CI green. No unresolved threads. PR is not draft. Awaiting human approval from
@mrleemurray.errors-fix-driver — cycle 26
Trigger: cron_review_comments · Head:
4a85d016b2b0978c04890e5b677dd86f2d74135f(4a85d01)blocked(awaiting approval)Push: no — no code changes needed · Copilot rerequested: no (no push)
Ready gate: CI green. No unresolved threads. PR is not draft. Awaiting human approval from
@mrleemurray.errors-fix-driver — cycle 27
Trigger: cron_review_comments · Head:
4a85d016b2b0978c04890e5b677dd86f2d74135f(4a85d01)blocked(awaiting approval)Push: no — no code changes needed · Copilot rerequested: no (no push)
Ready gate: CI green. No unresolved threads. PR is not draft. Awaiting human approval from
@mrleemurray.