fix(runtime): lazy-loaded globals should shadow on inherited [[Set]]#34405
Merged
nathanwhit merged 3 commits intoMay 27, 2026
Merged
Conversation
fibibot
requested changes
May 26, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
ObjectDefinePropertiesis still destructured inruntime/js/90_deno_ns.js, but this PR replaces its only remaining uses withcore.defineGlobalProperties.deno lint runtime/js/90_deno_ns.jsreportsno-unused-vars, matching the redlint debug linux-x86_64CI shard. Drop it from the primordial destructuring.core.defineGlobalPropertiesis exported fromlibs/core/01_core.jsbut is not declared inlibs/core/core.d.tsnext topropWritableLazyLoaded. Please add the declaration so the internalcoretype stays in sync with the exported API.
Lazy-loaded global accessors (`Response`, `Request`, `ReadableStream`, `WebSocket`, ...) installed via `propNonEnumerableLazyLoaded` / `propWritableLazyLoaded` used to store assigned values in a shared closure and ignore the receiver. Assigning through an object whose prototype chain includes `globalThis` (the `whatwg-fetch` / `cross-fetch` polyfill idiom) clobbered the global instead of creating a shadowing own property on the receiver, which broke `x instanceof Response` for real fetch responses. Restore data-property [[Set]] semantics: the setter now uses `Object.defineProperty(this, name, ...)` to define an own data property on the receiver. A new `core.defineGlobalProperties` helper stamps the property name onto each lazy descriptor at install time so the setter knows what to define. Fixes denoland#34403.
nathanwhit
force-pushed
the
nathanwhit/fix-34403-lazy-global-shadow
branch
from
May 26, 2026 23:47
b9d2dd0 to
c2d2acb
Compare
Drop unused `ObjectDefineProperties` primordial destructuring in `runtime/js/90_deno_ns.js` and add the missing `defineGlobalProperties` type declaration in `libs/core/core.d.ts`.
nathanwhit
force-pushed
the
nathanwhit/fix-34403-lazy-global-shadow
branch
from
May 26, 2026 23:48
c2d2acb to
ad84316
Compare
fibibot
previously requested changes
May 26, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
propNonEnumerableLazyLoadednow shadows inherited assignments withenumerable: false. That does not match ordinary [[Set]] on an inherited writable data property:g.Response = ...should create an enumerable own data property ong, while direct assignment to the own global should keep the global descriptor non-enumerable. Please branch on whetherthisalready ownsdesc[lazyNameSym](or equivalent) and add that descriptor assertion tolazy_global_polyfill_shadowing.
The previous blockers are fixed: ObjectDefineProperties is no longer unused in runtime/js/90_deno_ns.js, and core.defineGlobalProperties is declared in core.d.ts.
Inherited [[Set]] of a writable data property creates a new enumerable own data property on the receiver. Direct [[Set]] on the holder updates the existing descriptor in place, preserving its enumerable attribute. The lazy global setter now branches on whether `this` already owns the property name: if it does, the replacement descriptor keeps the original `enumerable: false`; if not, the new own property on the receiver is created with `enumerable: true`. Extend the spec test to assert both descriptor shapes.
fibibot
reviewed
May 27, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
The current head fixes my prior blocker: propNonEnumerableLazyLoaded now branches on ObjectHasOwn(this, name), so inherited g.Response = ... creates an enumerable own data property while direct globalThis.WebSocket = ... preserves the global descriptor's non-enumerability. core.defineGlobalProperties is declared in core.d.ts, and the stale ObjectDefineProperties destructuring is gone. Holding approval until CI is green.
fibibot
approved these changes
May 27, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
CI is green now, promoting prior review to APPROVE.
Contributor
|
@bartlomieju this is ready to merge |
littledivy
pushed a commit
to crowlKats/deno
that referenced
this pull request
Jun 10, 2026
…enoland#34405) ## Summary Fixes denoland#34403. Lazy-loaded global accessors (`Response`, `Request`, `ReadableStream`, `WebSocket`, `CompressionStream`, ...) installed via `propNonEnumerableLazyLoaded` / `propWritableLazyLoaded` stored assigned values in a shared closure and ignored the receiver. Assigning through an object whose prototype chain includes `globalThis` (the `whatwg-fetch` / `cross-fetch` polyfill idiom) clobbered the global instead of creating a shadowing own property on the receiver, breaking `x instanceof Response` for real fetch responses and tripping up wasm-bindgen loaders such as `@swc/wasm-web`. The setter now uses `Object.defineProperty(this, name, ...)` to define an own data property on the receiver, matching the pre-2.8 data-property `[[Set]]` semantics. A new `core.defineGlobalProperties` helper stamps the property name onto each lazy descriptor at install time so the setter knows which name to define. After the first set, the accessor is replaced by a regular data property on whichever object received the set, so subsequent reads on that object skip the lazy loader entirely: - Direct assignment to `globalThis`: descriptor becomes a data descriptor with the assigned value (same as 2.7.x). - Assignment via an inheriting object: creates a shadowing own property on the inheritor; `globalThis` is untouched. ## Test plan - [x] New spec test `tests/specs/run/lazy_global_polyfill_shadowing` covers the polyfill pattern, the `instanceof` invariant, and direct `globalThis` assignment. - [x] `cargo test --test specs -- lazy_global` passes. - [x] Related spec tests (`fetch`, `window`) still pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #34403.
Lazy-loaded global accessors (
Response,Request,ReadableStream,WebSocket,CompressionStream, ...) installed viapropNonEnumerableLazyLoaded/propWritableLazyLoadedstored assigned values in a shared closure and ignored the receiver. Assigning through an object whose prototype chain includesglobalThis(thewhatwg-fetch/cross-fetchpolyfill idiom) clobbered the global instead of creating a shadowing own property on the receiver, breakingx instanceof Responsefor real fetch responses and tripping up wasm-bindgen loaders such as@swc/wasm-web.The setter now uses
Object.defineProperty(this, name, ...)to define an own data property on the receiver, matching the pre-2.8 data-property[[Set]]semantics. A newcore.defineGlobalPropertieshelper stamps the property name onto each lazy descriptor at install time so the setter knows which name to define.After the first set, the accessor is replaced by a regular data property on whichever object received the set, so subsequent reads on that object skip the lazy loader entirely:
globalThis: descriptor becomes a data descriptor with the assigned value (same as 2.7.x).globalThisis untouched.Test plan
tests/specs/run/lazy_global_polyfill_shadowingcovers the polyfill pattern, theinstanceofinvariant, and directglobalThisassignment.cargo test --test specs -- lazy_globalpasses.fetch,window) still pass.