[JSC] ShadowRealm: importValue resolves an export whose value is undefined - #612
[JSC] ShadowRealm: importValue resolves an export whose value is undefined#612robobun wants to merge 1 commit into
Conversation
…fined The ExportGetter of ShadowRealm.prototype.importValue checks HasOwnProperty(exports, name) and then reads the value (https://tc39.es/proposal-shadowrealm/#sec-export-getter-functions). The builtin read the value and treated undefined as a missing export, so `export const v = undefined`, or an `export let v` that is assigned later, rejected with "requires |exportName| to exist". Check the name with @object.@hasOwn on the namespace object instead, then read it. For a module namespace object that is a lookup in the export list and does not read the binding. * JSTests/stress/shadow-realm-import-value.js: * JSTests/stress/resources/shadow-realm-example-module.js:
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs — the fix looks correct and matches the spec's ExportGetter semantics — but since it changes observable behavior of a JSC builtin and both touched paths fall under jsc-reviewers in CODEOWNERS, a human sign-off is still worthwhile.
What was reviewed:
lookupBindingnow uses@ Object.@ hasOwn(an existing intrinsic pattern also used inInjectedScriptSource.js), so it is not user-tamperable and stays an own-property check.- Confirmed inherited names (
toString,__proto__,__esModule) still reject andexportNameis already constrained to strings, so the namespace's ownSymbol.toStringTagis not reachable. - New stress-test assertions throw on failure and add no logging, per
JSTestsconventions.
Extended reasoning...
Overview
The PR changes three files: the self-hosted ShadowRealmPrototype.js builtin (4 effective lines), the shadow-realm-import-value.js stress test, and its resource module. The functional change replaces module[exportName] === @ undefined with !@ Object.@ hasOwn(module, exportName) as the missing-export check in importValue's lookupBinding closure, and only reads module[exportName] after the presence check passes. This aligns with the ShadowRealm proposal's ExportGetter algorithm (HasOwnProperty → Get) and lets exports that exist with value undefined — including uninitialized-then-assigned let live bindings — resolve rather than reject.
Security risks
None identified. @ Object.@ hasOwn is a private-name intrinsic reference (the same pattern already appears in Source/JavaScriptCore/inspector/InjectedScriptSource.js), so user code cannot override either Object or hasOwn to influence the check. The check remains own-property only, so names a namespace object might inherit (relevant in Bun where the namespace prototype carries __esModule) continue to be rejected — the new tests cover toString, __proto__, and __esModule. The exportName is validated as a string before this closure is built, so the namespace's own Symbol.toStringTag is unreachable through this path.
Level of scrutiny
Moderate. The diff is tiny and the logic is straightforward, but it alters observable semantics of a spec-tracked JS engine API (ShadowRealm.prototype.importValue), and the touched directories (Source/JavaScriptCore, JSTests) are listed under @ WebKit/jsc-reviewers in .github/CODEOWNERS. Per the review guidelines, CODEOWNER-covered paths should get a human look even when the automated review is clean, so I'm deferring rather than approving.
Other factors
Test coverage is good: the stress test adds both positive cases (undefinedValue, notYet before/after setNotYet) and negative cases (inherited names), and the assertions (shouldBe, shouldThrowAsync) throw on failure with no printing, consistent with JSTests/CLAUDE.md requirements. The PR description reports verification with a Debug+ASAN jsc under both JIT and --useJIT=false, which is the expected workflow from the project docs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesShadowRealm ShadowRealm importValue behavior
Priority: ⬇️ Low Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to importValue now accepts own exports whose value is undefined while continuing to reject missing or inherited names. The supplied test coverage and readiness assessment indicate no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, cause, fix, behavior, and validation. It does not include the required Bugzilla bug title and URL, reviewer line, or template-formatted changed-file and function list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
1 participant
Problem
ShadowRealm.prototype.importValuerejects an export that exists with the valueundefined:export let ready;that the module assigns later rejects the same way until it is assigned. Node.js (--experimental-shadow-realm) resolvesundefined.lookupBindingstep of theimportValuebuiltin (Source/JavaScriptCore/builtins/ShadowRealmPrototype.js): it readmodule[exportName]and treatedundefinedas missing. The proposal's ExportGetter testsHasOwnProperty(exports, name)and then reads the value. Upstream WebKitmainhas the same code.Fix
@Object.@hasOwn(module, exportName), then read it and wrap it as before. The error message for a missing name does not change.hasOwnis answered from the export list without reading the binding (JSModuleNamespaceObject::getOwnPropertySlotCommon,HasPropertycase). It is an own-property check, so a name the namespace would only inherit still rejects. That matters in Bun, where namespace objects have a prototype that carries an__esModuleaccessor:inwould have letimportValue(m, "__esModule")through.jsc: the extendedJSTests/stress/shadow-realm-import-value.js(anundefined-valued export, a liveletbinding before and after assignment, andtoString/__proto__/__esModulestill rejecting) passes with and without--useJIT=false. A Bun debug build on this branch passes the newimportValuecase in Bun'stest/js/bun/jsc/shadow.test.js, which fails on the current pin.Background
importValue(specifier, name)imports the module inside the shadow realm and resolves with one export, wrapped for the caller: a primitive as is, a callable as a wrapped function, anything else rejects with a TypeError. The namespace object itself never crosses the boundary, so this getter is the only place the export is looked up.