Skip to content

Return a deep cloned VSBuffer when it might be accessed again.#320190

Open
er91 wants to merge 6 commits into
microsoft:mainfrom
er91:patch-2
Open

Return a deep cloned VSBuffer when it might be accessed again.#320190
er91 wants to merge 6 commits into
microsoft:mainfrom
er91:patch-2

Conversation

@er91
Copy link
Copy Markdown

@er91 er91 commented Jun 5, 2026

Context

Fixes #316841

Summary

Returned VSBuffer object of ChunkStream._read() might be used in window.postMessage() and be transferred to an iframe, and become inaccessible in the current window. As a result, next time we try to access the same VSBuffer object, Safari will throw an exception synchronously.

In self-hosted code serve-web, this will cause extension webview to fail to load. Chrome/V8 is more relaxed and does not throw any exception in this case.

This PR makes sure to return a deep cloned VSBuffer object if it might be accessed again.

Copilot AI review requested due to automatic review settings June 5, 2026 22:26
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

Note

Copilot was unable to run its full agentic suite in this review.

This PR hardens ChunkStream._read() against a scenario where callers transfer returned VSBuffers (e.g., via window.postMessage()), which can detach underlying buffers and make the stream’s retained chunks inaccessible.

Changes:

  • Documented why _read() sometimes needs to return deep clones instead of references/views.
  • Updated _read() fast paths to return clones when advance === false and returning data that aliases an internal chunk.
  • Added cloning in additional branches to avoid returning buffers that could be transferred while still needed internally.
Comments suppressed due to low confidence (1)

src/vs/base/parts/ipc/common/ipc.net.ts:207

  • The new comment explains callers may transfer the returned VSBuffer. Returning a shared singleton from getEmptyBuffer() means a transfer could detach that shared underlying buffer and affect subsequent callers. Consider returning a fresh empty buffer (or a clone) here to avoid shared-state detachment.
		if (byteCount === 0) {
			return getEmptyBuffer();
		}

Comment thread src/vs/base/parts/ipc/common/ipc.net.ts
Comment thread src/vs/base/parts/ipc/common/ipc.net.ts Outdated
Comment thread src/vs/base/parts/ipc/common/ipc.net.ts Outdated
@er91
Copy link
Copy Markdown
Author

er91 commented Jun 5, 2026

@microsoft-github-policy-service agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants