Skip to content

ErrorInstance: pass the instance to the GC-time onComputeErrorInfo callback - #509

Closed
robobun wants to merge 1 commit into
mainfrom
farm/acf75065/error-info-pass-instance
Closed

ErrorInstance: pass the instance to the GC-time onComputeErrorInfo callback#509
robobun wants to merge 1 commit into
mainfrom
farm/acf75065/error-info-pass-instance

Conversation

@robobun

@robobun robobun commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • ErrorInstance::reconcileWeakReferencesAtGCEnd materializes the stack string as soon as one frame's callee or CodeBlock is unmarked. It calls vm.onComputeErrorInfo() with the frames, but not with the ErrorInstance.
  • Bun formats error.stack V8-style, with a name: message first line. Without the instance it cannot read name or message, so every error that goes through this path gets a bare Error header. An async function body runs in a per-call closure, so this hits every error thrown from an async function once a GC runs before the first .stack read (Async-thrown Error loses its message from error.stack when GC runs before first .stack access bun#34398).

Fix

  • Add JSC::JSObject* errorInstance to ErrorInfoFunction, in the same position as in ErrorInfoFunctionJSValue, and pass this from ErrorInstance::computeErrorInfo.
  • The instance is marked (the reconcile loop only visits marked cells), so its own properties are safe to read. The callee still must not allocate on the GC heap. The comment on the typedef says so.
  • The only caller is Bun. The matching Bun change reads the name and message without GC and uses this build.

…llback

reconcileWeakReferencesAtGCEnd materializes the stack string as soon as one
frame's callee or CodeBlock is unmarked. The embedder callback that formats
the string received the frames but not the ErrorInstance, so it could not
read the error's name and message. Bun formats the stack V8-style with a
"name: message" header, and that header came out as a bare "Error".

Add the ErrorInstance as a JSObject* parameter, in the same position as in
ErrorInfoFunctionJSValue.
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file.

Or wait 28 minutes for your next included review.

View limit details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a37cd590-b351-4e01-bc56-31492c565b79

📥 Commits

Reviewing files that changed from the base of the PR and between c148a12 and 682e98a.

📒 Files selected for processing (2)
  • Source/JavaScriptCore/runtime/ErrorInstance.cpp
  • Source/JavaScriptCore/runtime/VM.h

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — adds the ErrorInstance* parameter to ErrorInfoFunction and threads this through the single call site.

What was reviewed:

  • Confirmed ErrorInfoFunction has exactly one caller in this repo (ErrorInstance::computeErrorInfo), and it was updated to match the new signature.
  • New parameter position mirrors the existing ErrorInfoFunctionJSValue typedef; the added comment correctly documents the GC-end-phase no-allocation constraint.
  • this is safe to pass here — the reconcile loop only runs on marked cells, and computeErrorInfo's other caller (materializeErrorInfoIfNeeded) is a normal member call on a live instance.
Extended reasoning...

Overview

This PR adds a JSC::JSObject* errorInstance parameter to the ErrorInfoFunction typedef in VM.h (a Bun-specific hook under USE_BUN_JSC_ADDITIONS) and updates its single call site in ErrorInstance::computeErrorInfo to pass this. It also adds a two-line comment above the typedef documenting that the callback runs during the GC end phase and must not allocate on the GC heap or run JS.

Security risks

None. This is a purely additive signature change to an embedder callback typedef. No user-facing input handling, auth, or memory-safety-relevant logic is touched. The pointer being passed (this) is the marked ErrorInstance whose method is executing — it cannot be null or dangling.

Level of scrutiny

Low. The change is mechanical: one parameter added to a WTF::Function typedef, one argument added at the sole call site, plus a documentation comment. I grepped the tree for ErrorInfoFunction and onComputeErrorInfo — the only in-repo usages are the typedef, the VM accessors/member, and the single call in ErrorInstance.cpp, all of which are consistent after this change. The actual callback implementation lives in the Bun repo, and the PR description notes a matching Bun-side change; any signature mismatch there would be a compile error, not a runtime bug.

Other factors

  • The new parameter is placed in the same position as in the sibling typedef ErrorInfoFunctionJSValue, keeping the two hooks symmetric.
  • computeErrorInfo has two callers: reconcileWeakReferencesAtGCEnd (only iterates marked cells, so this is live) and materializeErrorInfoIfNeeded (ordinary method call on a live object). Passing this is safe in both.
  • The added comment on the typedef is accurate and useful — it makes the no-GC-allocation constraint explicit for the embedder.
  • No prior reviews or outstanding comments on the PR.

@github-actions

Copy link
Copy Markdown

Preview Builds

Commit Release Date
682e98ab autobuild-preview-pr-509-682e98ab 2026-08-24 13:42:11 UTC

@robobun

robobun commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #302, which fixes the same bug for oven-sh/bun#34398 and already went through review. This PR took the approach that review rejected (reading the instance inside the GC end phase).

@robobun robobun closed this Aug 24, 2026
@robobun
robobun deleted the farm/acf75065/error-info-pass-instance branch August 24, 2026 13:58
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.

1 participant