The failing test is here:

/cc @TimothyGu @fhinkel @domenic

I think @AnnaMag already has a PR open for this. Her PR removes the copyProperties() and thus fixes most of the vm issues.

@TimothyGu I don't see how I can refactor the copy to a function. Do you have an idea, knowing that PropertyDescriptor has no copy constructor?

@targos... for that failing test, looking at it now. This is the bit of code that is failing: https://github.com/targos/node/blob/3eceb415fa3049764befd0e3f2149df00228d9f6/src/node_contextify.cc#L431-L432

There are two specific issues here:

1. The attributes do not appear to be getting set correctly and
2. When the value is read_only, it simply returns without throwing

I'm still looking at why the first one is happening, but for the second one, to match the expected behavior, the setter should likely throw instead of simply returning.

@targos ... a patch like this gets the test-vm-context.js test working with one small tweak..

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 10bd46b..e40039f 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -414,6 +414,7 @@ class ContextifyContext {
       const PropertyCallbackInfo<Value>& args) {
     ContextifyContext* ctx;
     ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+    Environment* env = Environment::GetCurrent(args.GetIsolate());

     // Still initializing
     if (ctx->context_.IsEmpty())
@@ -421,15 +422,16 @@ class ContextifyContext {

     auto attributes = PropertyAttribute::None;
     bool is_declared =
-        ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(),
-                                                            property)
+        ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(), property)
         .To(&attributes);
     bool read_only =
         static_cast<int>(attributes) &
         static_cast<int>(PropertyAttribute::ReadOnly);
-
-    if (is_declared && read_only)
+    if (is_declared && read_only) {
+      if (args.ShouldThrowOnError())
+        env->ThrowError("Cannot assign to read only property");
       return;
+    }

     // true for x = 5
     // false for this.x = 5
diff --git a/test/parallel/test-vm-context.js b/test/parallel/test-vm-context.js
index 7e54047..9e770f5 100644
--- a/test/parallel/test-vm-context.js
+++ b/test/parallel/test-vm-context.js
@@ -116,5 +116,5 @@ vm.runInContext('x = 0', ctx);                      // Does not throw but x...
 assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.

 assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
-              /Cannot assign to read only property 'x'/);
+              /Cannot assign to read only property/);
 assert.strictEqual(vm.runInContext('x', ctx), 42);

To make the error message match perfectly, we would need to grab the name of the property (just slightly trickier with Symbol props) and include that in the error message.

@jasnell thanks. I made that change. I also added a check to the PropertyDefinerCallback for when the property is non-configurable.

While I was working on this, I found a possibly unexpected behavior with calls like Reflect.ownKeys(this). I added a failing test here: https://github.com/targos/node/blob/8176f21cc70467899c83c2a072569550ddf08b25/test/known_issues/test-vm-ownkeys.js
Am I correct to think that it should work?

Edit: currently, the three tests return ['a']

@targos In case you want to go down the route of throwing the error manually, V8 uses the following algorithm:

"Cannot assign to read only property '%s' of %s '%s'",
    property->ToDetailString(), ctx->sandbox()->TypeOf(), ctx->sandbox()->ToDetailString()

Still not a fan though. Also note that certain other errors need to be checked as well, such as an accessor property w/o a setter.

I'll have to look into the Reflect.ownKeys(this) issue further but so far this change LGTM

So... I did what I could for the error messages with my limited C++ knowledge... Please make suggestions if you know how to improve that :)

@TimothyGu thank you, we're almost there! Compared to the error thrown by V8, only the last string is inconsistent: ctx->sandbox->ToDetailString() returns [object Object]. The V8 error has #<Object>.

CI: https://ci.nodejs.org/job/node-test-pull-request/9935/

@targos

Compared to the error thrown by V8, only the last string is inconsistent: ctx->sandbox->ToDetailString() returns [object Object]. The V8 error has #<Object>.

Hmm. ToDetailString uses the same algorithm as V8 when figuring out what to print... in particular it looks at the toString property to determine whether to use #<X> (for when toString === Object.prototype.toString) or [object X]. I'm frankly not quite sure what's going on here.

@TimothyGu I thought it could be because the wrong context was used so I tried with the main context and the contextified one but the result is the same :(

Confirmed this condition is not met:

@nodejs/v8 any idea?

cc @fhinkel @isheludko

Ping @nodejs/v8

@hashseed Any ideas?

I am not sure how we can properly progress further. It seems hard to get any further insight into the issue. @targos is the current status quo of the PR already better than the situation before?

My apologies for the delay in my reaction to this.
As @fhinkel mentioned, #13265 provides a fix for this and most other vm issues. The PR was stalled due to https://codereview.chromium.org/2807333003/.
IMHO it is worth waiting for #13265 to land. I'm working on moving things forward.

Closing in favor of #16293.