vm: Function declarations should use [[Define]] instead of [[Set]] on the global object #31808

ExE-Boss · 2020-02-15T08:54:57Z

Version: v13.8.0
Platform: All; tested on Travis CI (Ubuntu 16.04.6 LTS)
Subsystem: vm

What steps will reproduce the bug?

const vm = require("vm");

let impl = { status: "" };
let ctx = vm.createContext();
Object.defineProperty(ctx, "status", {
	configurable: true,
	enumerable: true,
	get() {
		return impl.status;
	},
	set(v) {
		impl.status = `${v}`;
	}
});

// https://github.com/web-platform-tests/wpt/blob/master/xhr/status-async.htm
vm.runInContext(`
function statusRequest(...args) {
	console.log(args.map(JSON.stringify).join("\t"));
}
function status(code, text, content, type) {
	statusRequest("GET", code, text, content, type);
	statusRequest("HEAD", code, text, content, type);
	statusRequest("CHICKEN", code, text, content, type);
}
status(204, "UNICORNSWIN", "", "")
status(401, "OH HELLO", "Not today.", "")
status(402, "FIVE BUCKS", "<x>402<\/x>", "text/xml")
status(402, "FREE", "Nice!", "text/doesnotmatter")
status(402, "402 TEH AWESOME", "", "")
status(502, "YO", "", "")
status(502, "lowercase", "SWEET POTATO", "text/plain")
status(503, "HOUSTON WE HAVE A", "503", "text/plain")
status(699, "WAY OUTTA RANGE", "699", "text/plain")
`, ctx);

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

The functions get defined using [[DefineOwnProperty]] as specified in ECMA‑262 § 8.1.1.4.18 CreateGlobalFunctionBinding.

Expected log output

"GET" 	204 	"UNICORNSWIN" 	"" 	""
"HEAD" 	204 	"UNICORNSWIN" 	"" 	""
"CHICKEN" 	204 	"UNICORNSWIN" 	"" 	""
"GET" 	401 	"OH HELLO" 	"Not today." 	""
"HEAD" 	401 	"OH HELLO" 	"Not today." 	""
"CHICKEN" 	401 	"OH HELLO" 	"Not today." 	""
"GET" 	402 	"FIVE BUCKS" 	"402" 	"text/xml"
"HEAD" 	402 	"FIVE BUCKS" 	"402" 	"text/xml"
"CHICKEN" 	402 	"FIVE BUCKS" 	"402" 	"text/xml"
"GET" 	402 	"FREE" 	"Nice!" 	"text/doesnotmatter"
"HEAD" 	402 	"FREE" 	"Nice!" 	"text/doesnotmatter"
"CHICKEN" 	402 	"FREE" 	"Nice!" 	"text/doesnotmatter"
"GET" 	402 	"402 TEH AWESOME" 	"" 	""
"HEAD" 	402 	"402 TEH AWESOME" 	"" 	""
"CHICKEN" 	402 	"402 TEH AWESOME" 	"" 	""
"GET" 	502 	"YO" 	"" 	""
"HEAD" 	502 	"YO" 	"" 	""
"CHICKEN" 	502 	"YO" 	"" 	""
"GET" 	502 	"lowercase" 	"SWEET POTATO" 	"text/plain"
"HEAD" 	502 	"lowercase" 	"SWEET POTATO" 	"text/plain"
"CHICKEN" 	502 	"lowercase" 	"SWEET POTATO" 	"text/plain"
"GET" 	503 	"HOUSTON WE HAVE A" 	"503" 	"text/plain"
"HEAD" 	503 	"HOUSTON WE HAVE A" 	"503" 	"text/plain"
"CHICKEN" 	503 	"HOUSTON WE HAVE A" 	"503" 	"text/plain"
"GET" 	699 	"WAY OUTTA RANGE" 	"699" 	"text/plain"
"HEAD" 	699 	"WAY OUTTA RANGE" 	"699" 	"text/plain"
"CHICKEN" 	699 	"WAY OUTTA RANGE" 	"699" 	"text/plain"

What do you see instead?

evalmachine.<anonymous>:10
status(204, "UNICORNSWIN", "", "")
^

Uncaught TypeError: status is not a function
    at evalmachine.<anonymous>:10:1
    at Script.runInContext (vm.js:131:20)
    at Object.runInContext (vm.js:295:6)
    at repl:1:4
    at Script.runInThisContext (vm.js:120:20)
    at REPLServer.defaultEval (repl.js:432:29)
    at bound (domain.js:429:14)
    at REPLServer.runBound [as eval] (domain.js:442:12)
    at REPLServer.onLine (repl.js:759:10)
    at REPLServer.emit (events.js:333:22)

Additional information

Discovered in: jsdom/jsdom#2835 (comment)

    
           'use strict'; 
        
           // https://github.com/nodejs/node/issues/31808 
        
           // function declarations currently call [[Set]] instead of [[DefineOwnProperty]] 
        
           // in VM contexts, which violates the ECMA-262 specification: 
        
           // https://tc39.es/ecma262/#sec-createglobalfunctionbinding 
        
           const common = require('../common'); 
        
           const vm = require('vm'); 
        
           const assert = require('assert'); 
        
           const ctx = vm.createContext(); 
        
           Object.defineProperty(ctx, 'x', { 
        
             enumerable: true, 
        
             configurable: true, 
        
             get: common.mustNotCall('ctx.x getter must not be called'), 
        
             set: common.mustNotCall('ctx.x setter must not be called'), 
        
           }); 
        
           vm.runInContext('function x() {}', ctx); 
        
           assert.strictEqual(typeof ctx.x, 'function');

If not, then this issue should remain open.

domenic · 2024-08-25T08:58:13Z

Surprisingly, this is still an issue. Tested v22.7.0 and v20.17.0. I hope it is OK to cc @targos and @legendecas as people working in the area recently.

Because of this issue jsdom has lots of global properties which are currently data properties, and it is too scary for us to upgrade them to getters + setters (matching specs) because suddenly a bunch of scripts running in the jsdom context might start breaking.

targos · 2024-08-25T10:45:29Z

I don't know if it really is a Node.js bug. V8 calls the PropertySetterCallback, not the PropertyDefinerCallback (It also happens when ctx is empty).

domenic · 2024-08-25T10:59:59Z

Hmm, OK. Do you know if there's an upstream tracking bug on V8 then? I can try filing one but I think you know more about the relevant code so would be able to file a better one...

targos · 2024-08-25T11:38:03Z

Here's a stack trace:

  * frame #0: 0x00000001002804f0 node`node::contextify::ContextifyContext::PropertySetterCallback(property=Local<v8::Name> @ 0x000000016fdfbe90, value=Local<v8::Value> @ 0x000000016fdfbe88, args=0x000000016fdfbf88) at node_contextify.cc:549:51
    frame #1: 0x0000000100d60c9c node`v8::internal::PropertyCallbackArguments::CallNamedSetter(this=<unavailable>, interceptor=<unavailable>, name=Handle<v8::internal::Name> @ x20, value=Handle<v8::internal::Object> @ x19) at api-arguments-inl.h:227:7 [opt]
    frame #2: 0x000000010105a580 node`v8::internal::(anonymous namespace)::SetPropertyWithInterceptorInternal(it=0x000000016fdfc148, interceptor=v8::internal::DirectHandle<InterceptorInfo> @ x20, should_throw=(has_value_ = true, value_ = kThrowOnError), value=Handle<v8::internal::Object> @ x19) at js-objects.cc:1306:18 [opt]
    frame #3: 0x000000010105296c node`v8::internal::JSObject::DefineOwnPropertyIgnoreAttributes(it=0x000000016fdfc148, value=Handle<v8::internal::Object> @ x19, attributes=DONT_DELETE, should_throw=(has_value_ = true, value_ = kThrowOnError), handling=DONT_FORCE_FIELD, semantics=kSet, store_origin=kNamed) at js-objects.cc:3698:18 [opt]
    frame #4: 0x0000000101051f7c node`v8::internal::JSObject::DefineOwnPropertyIgnoreAttributes(it=<unavailable>, value=Handle<v8::internal::Object> @ x19, attributes=<unavailable>, handling=<unavailable>, semantics=<unavailable>) at js-objects.cc:3643:3 [opt]
    frame #5: 0x00000001013f5040 node`v8::internal::(anonymous namespace)::DeclareGlobal(isolate=0x0000000128008000, global=<unavailable>, name=Handle<v8::internal::String> @ x21, value=Handle<v8::internal::Object> @ x20, attr=<unavailable>, is_var=<unavailable>, redeclaration_type=kSyntaxError) at runtime-scopes.cc:126:3 [opt]
    frame #6: 0x00000001013f05d0 node`v8::internal::Runtime_DeclareGlobals(int, unsigned long*, v8::internal::Isolate*) at runtime-scopes.cc:204:3 [opt]
    frame #7: 0x00000001013f0070 node`v8::internal::Runtime_DeclareGlobals(args_length=<unavailable>, args_object=<unavailable>, isolate=0x0000000128008000) at runtime-scopes.cc:182:1 [opt]
    frame #8: 0x0000000101f9bcc8 node`Builtins_CEntry_Return1_ArgvInRegister_NoBuiltinExit + 72
    frame #9: 0x00000001020c8694 node`Builtins_CallRuntimeHandler + 84
    frame #10: 0x0000000101ef37d8 node`Builtins_InterpreterEntryTrampoline + 280
    frame #11: 0x0000000101ef0cac node`Builtins_JSEntryTrampoline + 172
    frame #12: 0x0000000101ef0950 node`Builtins_JSEntry + 176
    frame #13: 0x0000000100a529dc node`v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [inlined] v8::internal::GeneratedCode<unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, long, unsigned long**>::Call(this=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>) at simulator.h:187:12 [opt]
    frame #14: 0x0000000100a529d8 node`v8::internal::(anonymous namespace)::Invoke(isolate=0x0000000128008000, params=0x000000016fdfc608) at execution.cc:420:22 [opt]
    frame #15: 0x0000000100a53918 node`v8::internal::Execution::CallScript(isolate=0x0000000128008000, script_function=Handle<v8::internal::JSFunction> @ x21, receiver=Handle<v8::internal::Object> @ x20, host_defined_options=Handle<v8::internal::Object> @ 0x000000016fdfc658) at execution.cc:517:10 [opt]
    frame #16: 0x00000001006f2b24 node`v8::Script::Run(this=0x0000000120829eb0, context=<unavailable>, host_defined_options=<unavailable>) at api.cc:2128:7 [opt]
    frame #17: 0x0000000100285f14 node`node::contextify::ContextifyScript::EvalMachine(v8::Local<v8::Context>, node::Environment*, long long, bool, bool, bool, v8::MicrotaskQueue*, v8::FunctionCallbackInfo<v8::Value> const&)::$_1::operator()(this=0x000000016fdfc898) const at node_contextify.cc:1252:40

We see that DefineOwnPropertyIgnoreAttributes is called with semantics=kSet.

This seems to be a known limitation:

node/deps/v8/src/objects/objects.h

Lines 108 to 116 in 7fea010

    
           // Currently DefineOwnPropertyIgnoreAttributes invokes the setter 
        
           // interceptor and user-defined setters during define operations, 
        
           // even in places where it makes more sense to invoke the definer 
        
           // interceptor and not invoke the setter: e.g. both the definer and 
        
           // the setter interceptors are called in Object.defineProperty(). 
        
           // kDefine allows us to implement the define semantics correctly 
        
           // in selected locations. 
        
           // TODO(joyee): see if we can deprecate the old behavior. 
        
           enum class EnforceDefineSemantics { kSet, kDefine };

Comment added in v8/v8@4ee68d8

I don't know if there's an open V8 bug about this.

/cc @joyeecheung

joyeecheung · 2024-08-25T11:49:37Z

Yes I noticed that issue when implementing class fields and avoided that weird behavior with a separate path for class fields. But ordinary Object.defineProperty() still has that old behavior AFAICT. I never got around to investigate whether the old behavior could be broken.

joyeecheung · 2024-08-25T11:55:30Z

Here is the bug I opened a while back after I discovered that behavior: https://issues.chromium.org/u/2/issues/42202495,
and this is the thread where we discussed how that behavior came to be… https://chromium-review.googlesource.com/c/v8/v8/+/3300092/comments/e98c91dd_75cb8cb2

joyeecheung · 2024-08-25T12:10:50Z

BTW for interceptor-less contexts, there is also #54394 though I am not sure if that meets all the requirements of jsdom (if you care about the constructor of the global object, maybe it doesn't?)

domenic · 2024-09-22T04:50:06Z

Based on some initial testing I think DONT_CONTEXTIFY can work for jsdom. (We can fix up the constructor by changing the .constructor property.)

However, since it won't be supported in all active LTS versions until ~May 2026, it isn't very helpful for us now :(

targos · 2024-09-22T07:08:17Z

FWIW the next v20.x version will include DONT_CONTEXTIFY

ExE-Boss mentioned this issue Feb 15, 2020

Refactor Window to use WebIDL2JS jsdom/jsdom#2835

Draft

7 tasks

devsnek added v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem. labels Feb 15, 2020

devsnek mentioned this issue Feb 15, 2020

vm: introduce vm.Context #30709

Closed

4 tasks

devsnek added a commit to devsnek/node that referenced this issue Feb 17, 2020

vm: introduce vm.Context

4725ac6

Fixes nodejs#855 Fixes nodejs#31658 Fixes nodejs#31808

ExE-Boss added a commit to ExE-Boss/node that referenced this issue Jun 23, 2020

vm: add tests for function declarations using [[DefineOwnProperty]]

1340c99

Refs: nodejs#31808

ExE-Boss mentioned this issue Jun 23, 2020

vm: add tests for function declarations using [[DefineOwnProperty]] #34032

Closed

3 tasks

addaleax pushed a commit that referenced this issue Jul 15, 2020

vm: add tests for function declarations using [[DefineOwnProperty]]

9ae8491

Refs: #31808 PR-URL: #34032 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host>

cjihrig pushed a commit that referenced this issue Jul 23, 2020

vm: add tests for function declarations using [[DefineOwnProperty]]

f1578e4

Refs: #31808 PR-URL: #34032 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host>

MylesBorins pushed a commit that referenced this issue Jul 27, 2020

vm: add tests for function declarations using [[DefineOwnProperty]]

c9b652f

Refs: #31808 PR-URL: #34032 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host>

addaleax pushed a commit that referenced this issue Sep 22, 2020

vm: add tests for function declarations using [[DefineOwnProperty]]

981d512

Refs: #31808 PR-URL: #34032 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host>

addaleax pushed a commit that referenced this issue Sep 22, 2020

vm: add tests for function declarations using [[DefineOwnProperty]]

7f51e79

Refs: #31808 PR-URL: #34032 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host>

Mesteery closed this as completed Feb 17, 2022

Mesteery reopened this Feb 18, 2022

ExE-Boss mentioned this issue Sep 5, 2022

vm: in strict mode, assigning functions to certain globals throws, but succeeds #38918

Open

domenic mentioned this issue Aug 25, 2024

Change various Window data properties to accessors jsdom/jsdom#3770

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: Function declarations should use [[Define]] instead of [[Set]] on the global object #31808

vm: Function declarations should use [[Define]] instead of [[Set]] on the global object #31808

ExE-Boss commented Feb 15, 2020 •

edited by targos

Loading

ExE-Boss commented Feb 15, 2020

devsnek commented Feb 15, 2020

ExE-Boss commented Feb 15, 2020

ExE-Boss commented Feb 18, 2022

domenic commented Aug 25, 2024

targos commented Aug 25, 2024 •

edited

Loading

domenic commented Aug 25, 2024

targos commented Aug 25, 2024

joyeecheung commented Aug 25, 2024

joyeecheung commented Aug 25, 2024 •

edited

Loading

joyeecheung commented Aug 25, 2024

domenic commented Sep 22, 2024

targos commented Sep 22, 2024

vm: Function declarations should use [[Define]] instead of [[Set]] on the global object #31808

vm: Function declarations should use [[Define]] instead of [[Set]] on the global object #31808

Comments

ExE-Boss commented Feb 15, 2020 • edited by targos Loading

What steps will reproduce the bug?

How often does it reproduce? Is there a required condition?

What is the expected behavior?

What do you see instead?

Additional information

See also

ExE-Boss commented Feb 15, 2020

devsnek commented Feb 15, 2020

ExE-Boss commented Feb 15, 2020

ExE-Boss commented Feb 18, 2022

domenic commented Aug 25, 2024

targos commented Aug 25, 2024 • edited Loading

domenic commented Aug 25, 2024

targos commented Aug 25, 2024

joyeecheung commented Aug 25, 2024

joyeecheung commented Aug 25, 2024 • edited Loading

joyeecheung commented Aug 25, 2024

domenic commented Sep 22, 2024

targos commented Sep 22, 2024

ExE-Boss commented Feb 15, 2020 •

edited by targos

Loading

targos commented Aug 25, 2024 •

edited

Loading

joyeecheung commented Aug 25, 2024 •

edited

Loading