[JSC] JSGlobalObject::addSymbolTableEntry makes the global object a dictionary before it adds a name by robobun · Pull Request #738 · oven-sh/WebKit

robobun · 2026-09-27T06:18:56Z

Problem

A warm globalThis.x read keeps its old answer after the script var x = 33; declares x: stale miss: undefined. No user reported it.
The JSONP fast path (interpreter/Interpreter.cpp:1119) adds the name to the symbol table with no new Structure (runtime/JSGlobalObject.cpp:2556).
A bytecode script is correct: runtime/ProgramExecutable.cpp:219 makes the global object a dictionary first.

Fix

JSGlobalObject::addSymbolTableEntry converts a global object that is not a dictionary before it adds the name. That retires the old Structure: its ID changes, and its transition watchpoint fires.
addStaticGlobals gets an ASSERT. Its callers run while the global object is created.
Verified: JSTests/stress/jsonp-var-declaration-invalidates-global-object-property-caches.js fails on main and passes here in 10 of 10 JIT configurations. Also 15 existing tests.
Self-reviewed: 4 concerns raised, 4 addressed. The review gave no final list (Notes).

Background

Symbol table: the global var and function bindings of classic scripts, outside the Structure (hidden class).
Dictionary: a Structure of one object. Properties change under it, so no cache records an absent property there.
Considered the same lines in the JSONP branch: a second declaration of a name converts there too.

Downsides

A new JSONP variable leaves absent and inherited globalThis reads uncached: about 500 instructions each, not 3. Every bytecode script does this today.
Release jsc text: 32 bytes less. A new global binding costs 5 more instructions (321 to 326).
The JSONP branch skips the redeclaration checks of the bytecode path (Notes).

Notes

Repro (jsc)

function get() { return globalThis.missingVar; }
noInline(get);
for (let i = 0; i < 100000; ++i) get();
loadString("var missingVar = 33;");
if (get() !== 33) throw new Error("stale miss: " + get());

It throws with the JIT on (default, --useDFGJIT=0, --useFTLJIT=0, --useLLInt=0, --useConcurrentJIT=0) and passes with --useJIT=0. About 40 warm-up calls are enough when the concurrent JIT is off.

The interpreter has the same problem for a global object with no JSGlobalProxy in front of it ($vm.createGlobalObject() in the shell). After 20 calls of typeof object.toString and the script var toString = 33; the LLInt cache still answers function, also with --useJIT=0.

Repro (Bun 1.4.3, node prints 33 33, Bun prints undefined 33)

const vm = require("node:vm");
function get() { return globalThis.missingVar; }
for (let i = 0; i < 100000; ++i) get();
vm.runInThisContext("var missingVar = 33;");
console.log(get(), globalThis.missingVar);

What it takes to see the bug

All of these at once:

The script has the JSONP form: only statements like var name = <JSON>;, name = <JSON>; or a.b = <JSON>;. The same declaration with one more statement (var x = 33; void 0;) runs as bytecode and is correct. A function declaration is never JSONP.
The name is new to the global object.
A site that is already in JIT code reads or writes that name through globalThis or this (or as an identifier, for the DFG).
Between the warm-up and the read, the global object keeps its Structure. A bytecode script, a require() of a CommonJS file in Bun, or a new global property gives it another Structure and hides the bug.

What is stale on main

Every cache that looks through the JSGlobalProxy to the Structure of the global object (tryCacheGetBy, bytecode/Repatch.cpp:568, and tryCachePutBy, :1072), and the DFG and FTL code built from those caches:

a read of an absent name: globalThis.x, this.x, globalThis[key], super.x, Reflect.get, destructuring, optional chain
a data property, a getter or a method of the prototype chain that the new variable shadows (globalThis.toString after var toString = 33;)
a write that reached a setter of the prototype chain: the setter keeps running and the variable keeps its value
identifier sites on the global object itself: read and write in the DFG, delete name in Baseline
a site that gets warm between two statements of the same script, and a reader that is on the stack when the script runs
the LLInt cache of a load from the prototype chain, when the base is the global object itself

Not affected: in, hasOwnProperty, Object.hasOwn, Reflect.has, Object.keys, for-in, the megamorphic path, and an object whose prototype is the global object. They stop at the proxy or at OverridesGetOwnPropertySlot. A node:vm context is not affected, its global object prohibits caching.

Entry points in Bun

oven-sh/bun#44097 pins the preview build of this branch and adds the node:vm and napi tests.

vm.runInThisContext, new vm.Script().runInThisContext(), the same two in a Worker, and napi_run_script were run and are stale on 1.4.3. The inspector's Runtime.evaluate, the raw fallback of bun repl and the Bake loaders reach Interpreter::executeProgram too. They were not run for this PR.

Why the fix is in addSymbolTableEntry

It is the one function through which a script adds a name to that symbol table. Its callers are createGlobalVarBinding<Global> and createGlobalFunctionBinding<Global>, both after their check for an existing name.
The bytecode path already arrives there with a dictionary, so it does nothing new for that path.
A declaration of a name that exists creates no binding and converts nothing. The last block of the new test checks this with the StructureID.
The invalidation must happen at each insert. One conversion before the first statement is not enough: a setter that the script calls can warm a site and flatten the Structure before the next declaration.
Not chosen: GetOwnPropertySlotIsImpureForPropertyAbsence on JSGlobalObject (it covers misses only and turns the miss cache off for every program), a refusal in the caches (the same cost), a watchpoint for each name (every reader has to subscribe, and the write still happens under the live Structure).
Not chosen: no fast path for declarations, so that each such script runs as bytecode. It removes the other differences of the JSONP branch too (see below). But the fast path is there for data: var x = <1.3 MB of small objects>; takes 26 ms on the fast path and 1,170 ms as bytecode, and 738 KB of nested arrays take 24 ms and 92 ms (jsc, release build of this PR, best of 7).
The global object can leave the dictionary state again: eval flattens an uncacheable dictionary (interpreter/Interpreter.cpp:1548). The next new name then converts again, because the guard runs at each insert.

ASSERT in addStaticGlobals

JSGlobalObject::init() converts the global object first (runtime/JSGlobalObject.cpp:1102). Checked:

jsc with assertions and this diff: the main global object, $262.createRealm(), createGlobalObject(), $vm.createGlobalObject(), a $262.agent thread and a ShadowRealm, all made while caches of the main realm were warm. No assertion.
Bun 1.4.3 under gdb: 24 of 24 addStaticGlobals calls arrive with a dictionary (process start, ESM, CommonJS, a Worker, node:vm contexts, a ShadowRealm).

An embedder that calls addStaticGlobals after code ran needs the conversion there too.

Measurements, release jsc (bun-webkit-linux-amd64-lto), main f20ce77445 against this PR

Tools: gdb breakpoint counts, gdb single-step counts, nm -S, objdump -d, size. All with --useConcurrentJIT=0. The column for this PR is the bun-webkit-linux-amd64-lto build that CI made of 6f120a3bf2. Each step count was the same in 3 of 3 runs, but for the last row of the table and the reads after a conversion below.

Measurement	main	this PR
`jsc` text (`size`)	47,724,559 bytes	47,724,527 bytes
`addSymbolTableEntry` (`nm -S`, `objdump -d`)	402 bytes, 14 instructions before the lock	398 bytes, 24 instructions before the lock
`addStaticGlobals` (`nm -S`, `objdump -d`)	707 bytes, 182 instructions	707 bytes, the same 182 instructions
one new global binding, no conversion (step count of `addSymbolTableEntry`, call 100 of 20,000 bytecode declarations)	321 instructions	326
one JSONP program, new variable, no conversion (step count of `executeProgram`, call 100)	2,675	2,680
one JSONP program, existing variable	2,187	2,187
one JSONP program, assignment `x = -0`	2,169	2,169
one JSONP program, the first new variable on a flattened global object (the one conversion)	3,355	7,564 to 7,582
`convertToDictionary` calls: empty script / 20,000 bytecode declarations / 20,000 JSONP assignments / 20,000 JSONP declarations of an existing name / 2,000 new JSONP variables after a warm miss	2 / 2 / 2 / 2 / 2	2 / 2 / 2 / 2 / 3
40,000 reads after one JSONP script with a new name: reads on the slow path, CodeBlocks jettisoned	0, 0 (and a read of the new name is stale)	39,990, 2
the same with an existing name	0, 0	0, 0

One read through globalThis, in instructions above an empty function call:

Read	Baseline only, cached	all tiers, cached	Baseline only, after the script	all tiers, after the script
absent name, main, after a bytecode script	+29 to +31	+3	+451 to +495	+454 to +497
absent name, this PR, after a JSONP script with a new name	+31	+3	+452 to +495	+497
inherited name, main, after a bytecode script	+32	+2 to +3	+525 to +527	+529
inherited name, this PR, after a JSONP script with a new name	+32	+3	+526	+529
own property, main, after a bytecode script	+32 to +34	+4	+32 to +33	+43
own property, this PR, after a JSONP script with a new name	+34	+4	+34	+43

On main a JSONP script leaves all three reads on their caches (+29, +32, +33), and that is the bug for the name it declared.

The state after the script is a dictionary that was flattened before. The miss and prototype caches give up on it. With this PR a JSONP script that declares a new name leaves the same state that every bytecode script leaves on main.

Tests

New test: 51 checks. 19 for the JSONP form and the same 19 for the bytecode form, each case in a realm of its own. Two for a global object with no proxy, one for a function declaration. 10 in the last block, for the StructureID and the values. On main with --useConcurrentJIT=0 23 of the 51 fail: 18 of the JSONP form, the 2 with no proxy, and 3 of the last block. With --useJIT=0 3 fail: the LLInt case and 2 of the last block. With compiler threads the number changes from run to run.
By hand, 10 configurations: default, --useConcurrentJIT=0, --useDFGJIT=0, --useFTLJIT=0, --useLLInt=0, --useJIT=0, --forceEagerCompilation=1, --useAccessInlining=0, low tier-up thresholds, --validateExceptionChecks=1. Main fails in all 10. The release build of CI and a local build with assertions pass all 10.
With run-jsc-stress-tests, 17 modes (default, bytecode-cache, mini-mode, lockdown, no-llint, and the eager, no-cjit and FTL modes). The release build and the build with assertions pass 17 of 17. The build of main fails 17 of 17.
The cases that use $vm.evaluateWithScopeExtension() give it only programs of the JSONP form. It has no bytecode cache, and the second run of the bytecode-cache mode asserts that each program comes from the cache.
The test pins the guard from both sides. A build that converts on each insert (the jne of the guard made a nop under gdb) fails the check "new name, the dictionary of init(): same Structure". A build that never converts fails the first cache check.
The case "cache created by an earlier statement" has a reader that nothing calls before the script. The first declaration finds a dictionary and converts nothing, the setter makes the site warm and the cache flattens the global object, and the second declaration has to convert. One conversion for each script does not pass it.
CI ran the JavaScriptCore test suites on the linux-amd64-lto build of this branch: passed.
The 15 existing tests: stress/can-declare-global-var-invoked-from-jsonp-fast-path.js, five stress/jsonp-*.js, stress/get-from-scope-global-variable-watchpoint-lookup.js, stress/create-global-function-binding-updates-descriptor-global.js, two stress/cached-custom-*-shadowed-on-flattened-dictionary.js, stress/get-by-id-constant-fold-dictionary-head-shadow-property.js, microbenchmarks/JSONP-negative-0.js, three microbenchmarks/global-var-*.js. Tier configurations: default, --useJIT=0, --useDFGJIT=0, --useFTLJIT=0, --useConcurrentJIT=0.
436 more stress tests that load scripts or touch dictionaries, in 3 modes on an assertion build: the same results as without the diff.

Self-review

The four concerns, all closed:

No test pinned the guard. A build that converts on each insert passed the first version of the test. The last block of the test now fails on it.
The case "cache created by an earlier statement" made the reader warm before the script ran, so it also passed with one conversion for each script. The reader is now cold until the setter runs.
The LLInt cache of a global object with no proxy was not under test. Two cases cover it now.
These notes gave 18 failed checks of 42 on main for the first version of the test. The number was 19 with --useConcurrentJIT=0, and it changes with compiler threads.

The self-review did not return its final list. These four come from its working files, and I checked each one before I acted on it. Other concerns of that review can be open.

One review comment on this PR asks for a plain transition in place of the dictionary, to keep the caches. It is not adopted, and the thread has the reasons.

Related

#735 and oven-sh/bun#44024 each have one test block that runs only while the reader is in the interpreter. The reason is this bug.

Not in this PR

The JSONP declaration branch (interpreter/Interpreter.cpp:1116-1126) differs from the bytecode path in more ways. They need no warm cache and this PR does not change them:

let a = 1; and then the script var a = 2;: no SyntaxError, and globalThis.a is 2
bindings are created statement by statement, not before the first statement
x = 1; var x = 2; leaves x configurable
the statements before a failed declaration run twice on a global object that is not extensible
no check for a Proxy in the prototype chain of the global object

Also left as it is: ProgramExecutable.cpp:219 converts for every bytecode script, also for one that declares nothing.

Upstream WebKit has the same code in all of these places (checked at be499e24a6).

perf, valgrind, strace and bloaty are not installed on the machine that made these numbers.

…ictionary before it adds a name A program of the form `var x = <JSON>;` runs on the JSONP fast path of Interpreter::executeProgram. That path calls createGlobalVarBinding, which adds the name to the symbol table of the global object. The Structure of the global object stays the same, so inline caches and optimized code that hold "x is not an own property" against that Structure keep their answer: function get() { return globalThis.x; } noInline(get); for (let i = 0; i < 100000; ++i) get(); loadString("var x = 33;"); get(); // undefined, expected 33 A program that runs as bytecode does not have the bug, because ProgramExecutable::initializeGlobalProperties makes the global object a dictionary before it creates a binding. addSymbolTableEntry now converts a global object whose Structure is not a dictionary before it adds the name. Caches hold the absence of a property only against a Structure that is not a dictionary, and the conversion gives the object another Structure and fires the transition watchpoint of the old one. A declaration of a name that exists creates no binding and converts nothing. addStaticGlobals, the other function that adds names to that symbol table, asserts the same condition. Its callers run while the global object is created, after init() has made it a dictionary. * JSTests/stress/jsonp-var-declaration-invalidates-global-object-property-caches.js: Added. * Source/JavaScriptCore/runtime/JSGlobalObject.cpp: (JSC::JSGlobalObject::addSymbolTableEntry): (JSC::JSGlobalObject::addStaticGlobals):

robobun · 2026-09-27T06:19:13Z

Status

Reproduced with the jsc shell of main f20ce77445 (release build): the first script in the Notes of this PR throws stale miss: undefined. It passes with --useJIT=0.
Reproduced in Bun 1.4.3: after vm.runInThisContext("var missingVar = 33;") a warm globalThis.missingVar read gives undefined. Node gives 33.
This PR: [JSC] JSGlobalObject::addSymbolTableEntry makes the global object a dictionary before it adds a name #738 (ready for review, CI green on 67fe85979c).
The Bun PR that pins the build of this branch and adds the node:vm and napi tests: Bump WebKit (oven-sh/WebKit#738 preview): a warm globalThis read sees a var that vm.runInThisContext declares bun#44097

github-actions · 2026-09-27T06:33:23Z

Preview build of 67fe859: autobuild-preview-pr-738-67fe8597

Jarred-Sumner · 2026-09-28T20:41:58Z

@robobun wake up!!

…e for the LLInt - The reader of the case "cache created by an earlier statement" is cold before the program runs. The second declaration of the program has to convert the global object. - Two cases use a global object with no JSGlobalProxy. The LLInt cache of a load from the prototype chain is stale there without the fix, also with --useJIT=0. - The last block checks that a new name on a dictionary keeps the Structure, and that a new name converts again after the global object was flattened again. - A function declaration as a program.

…mode $vm.evaluateWithScopeExtension() has no bytecode cache, and the second run of the bytecode-cache mode asserts that each program comes from the cache. So the cases that use it give it only programs of the JSONP form, which have no bytecode.

robobun · 2026-09-28T22:05:27Z

I am here. The state of this PR:

The change under Source/ is the same as on Sep 27: 9 added lines in runtime/JSGlobalObject.cpp. CI was green on it.
Today I made the stress test stronger. The first push of it (daea05ba3f) failed one run in CI: the bytecode-cache mode. The new cases gave a program that runs as bytecode to $vm.evaluateWithScopeExtension(), which has no bytecode cache. 67fe85979c corrects the test.
With 67fe85979c, run-jsc-stress-tests passes the test in 17 of 17 modes on the release build and on a build with assertions. The build of main fails it in 17 of 17.

I mark this PR ready for review when CI is green on 67fe85979c. The Bun side is oven-sh/bun#44097.

coderabbitai · 2026-09-29T00:26:39Z

Warning

Review limit reached

Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 20 minutes for your next included review.

Check out review usage here.

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: Essentials

Run ID: 24af71d2-817f-4049-af97-85b0dcbd6847

📥 Commits

Reviewing files that changed from the base of the PR and between f20ce77 and 67fe859.

📒 Files selected for processing (2)

JSTests/stress/jsonp-var-declaration-invalidates-global-object-property-caches.js
Source/JavaScriptCore/runtime/JSGlobalObject.cpp

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

claude

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

robobun added a commit to oven-sh/bun that referenced this pull request Sep 27, 2026

Bump WebKit to the preview build of oven-sh/WebKit#738

52e7d8b

robobun mentioned this pull request Sep 27, 2026

Bump WebKit (oven-sh/WebKit#738 preview): a warm globalThis read sees a var that vm.runInThisContext declares oven-sh/bun#44097

Draft

robobun added 2 commits September 28, 2026 20:58

robobun added a commit to oven-sh/bun that referenced this pull request Sep 29, 2026

Bump WebKit to the preview build of oven-sh/WebKit#738 at 67fe8597

c54f9f0

robobun marked this pull request as ready for review September 29, 2026 00:26

claude Bot reviewed Sep 29, 2026

View reviewed changes

Comment thread Source/JavaScriptCore/runtime/JSGlobalObject.cpp Resolved

robobun mentioned this pull request Sep 29, 2026

[JSC] LLInt get_by_id: misses count toward the Baseline JIT, and the cache holds more #735

Open

robobun requested a review from Jarred-Sumner September 29, 2026 15:12

Navigation Menu

Repository navigation

[JSC] JSGlobalObject::addSymbolTableEntry makes the global object a dictionary before it adds a name - #738

[JSC] JSGlobalObject::addSymbolTableEntry makes the global object a dictionary before it adds a name#738
robobun wants to merge 3 commits into
mainfrom
robobun/f4bc73ed/global-var-stale-miss-ic

robobun commented Sep 27, 2026 •

edited

Loading

Uh oh!

robobun commented Sep 27, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 27, 2026 •

edited

Loading

Uh oh!

Jarred-Sumner commented Sep 28, 2026

Uh oh!

robobun commented Sep 28, 2026

Uh oh!

coderabbitai Bot commented Sep 29, 2026

Review limit reached

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

Downsides

Uh oh!

robobun commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jarred-Sumner commented Sep 28, 2026

Uh oh!

robobun commented Sep 28, 2026

Uh oh!

coderabbitai Bot commented Sep 29, 2026

Review limit reached

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Sep 27, 2026 •

edited

Loading

robobun commented Sep 27, 2026 •

edited

Loading

github-actions Bot commented Sep 27, 2026 •

edited

Loading