Skip to content

fix: clear parent change listener before disposeContext in ScopedContextKeyService#307593

Merged
Yoyokrazy merged 2 commits intomicrosoft:mainfrom
maruthang:fix/issue-307067-context-key-listener-leak
Apr 22, 2026
Merged

fix: clear parent change listener before disposeContext in ScopedContextKeyService#307593
Yoyokrazy merged 2 commits intomicrosoft:mainfrom
maruthang:fix/issue-307067-context-key-listener-leak

Conversation

@maruthang
Copy link
Copy Markdown
Contributor

Fixes #307067

Summary

  • ScopedContextKeyService.dispose() now calls this._parentChangeListener.clear() before this._parent.disposeContext(this._myContextId)
  • Previously, the parent emitter listener could still fire during disposal if a context change occurred between the start of dispose() and super.dispose() being called, contributing to the listener leak

Test plan

  • Create many scoped context key service instances and dispose them — the "potential listener LEAK detected" warning should no longer appear in the console

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a potential listener leak in the platform ContextKeyService by ensuring ScopedContextKeyService stops forwarding parent context-change events before tearing down its scoped context.

Changes:

  • Clear the scoped service’s parent onDidChangeContext listener at the start of ScopedContextKeyService.dispose().
  • Dispose the scoped context (disposeContext) only after detaching from parent events, preventing late event forwarding during teardown.
Show a summary per file
File Description
src/vs/platform/contextkey/browser/contextKeyService.ts Adjusts ScopedContextKeyService.dispose() ordering to detach parent listener before disposing scoped context, addressing listener leak warnings.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines 515 to 517
this._parent.disposeContext(this._myContextId);
this._domNode.removeAttribute(KEYBINDING_CONTEXT_ATTR);
super.dispose();
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

This disposal-order fix would be safer with a regression test. There are existing unit tests for ContextKeyService, but none appear to cover that a disposed scoped service stops forwarding parent onDidChangeContext events (and doesn’t contribute to listener leak warnings). Consider adding a test that creates many scoped services, disposes them, then mutates the parent context and asserts no child change events fire / no listeners remain attached.

Suggested change
this._parent.disposeContext(this._myContextId);
this._domNode.removeAttribute(KEYBINDING_CONTEXT_ATTR);
super.dispose();
try {
this._parent.disposeContext(this._myContextId);
this._domNode.removeAttribute(KEYBINDING_CONTEXT_ATTR);
} finally {
super.dispose();
}

Copilot uses AI. Check for mistakes.
@Yoyokrazy Yoyokrazy enabled auto-merge (squash) April 16, 2026 22:52
@Yoyokrazy Yoyokrazy disabled auto-merge April 16, 2026 22:52
@Yoyokrazy Yoyokrazy enabled auto-merge (squash) April 22, 2026 20:34
@Yoyokrazy Yoyokrazy merged commit 16f1ebc into microsoft:main Apr 22, 2026
26 checks passed
@vs-code-engineering vs-code-engineering Bot added this to the 1.118.0 milestone Apr 22, 2026
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.

[Unhandled Error] [78d] potential listener LEAK detected (282 listeners) — chatInlineAnchorWidget / contextKeyService

5 participants