vm: mark global proxy as side-effect-free

Fixes: #27518

PR-URL: #27523
Reviewed-By: Aleksei Koziatinskii <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: addaleax

src/node_contextify.cc

@@ -61,6 +61,7 @@ using v8::PrimitiveArray;
 using v8::PropertyAttribute;
 using v8::PropertyCallbackInfo;
 using v8::PropertyDescriptor;
+using v8::PropertyHandlerFlags;
 using v8::Script;
 using v8::ScriptCompiler;
 using v8::ScriptOrigin;
@@ -149,13 +150,15 @@ MaybeLocal<Context> ContextifyContext::CreateV8Context(
   if (!CreateDataWrapper(env).ToLocal(&data_wrapper))
     return MaybeLocal<Context>();
 
-  NamedPropertyHandlerConfiguration config(PropertyGetterCallback,
-                                           PropertySetterCallback,
-                                           PropertyDescriptorCallback,
-                                           PropertyDeleterCallback,
-                                           PropertyEnumeratorCallback,
-                                           PropertyDefinerCallback,
-                                           data_wrapper);
+  NamedPropertyHandlerConfiguration config(
+      PropertyGetterCallback,
+      PropertySetterCallback,
+      PropertyDescriptorCallback,
+      PropertyDeleterCallback,
+      PropertyEnumeratorCallback,
+      PropertyDefinerCallback,
+      data_wrapper,
+      PropertyHandlerFlags::kHasNoSideEffect);
 
   IndexedPropertyHandlerConfiguration indexed_config(
       IndexedPropertyGetterCallback,
@@ -164,7 +167,8 @@ MaybeLocal<Context> ContextifyContext::CreateV8Context(
       IndexedPropertyDeleterCallback,
       PropertyEnumeratorCallback,
       IndexedPropertyDefinerCallback,
-      data_wrapper);
+      data_wrapper,
+      PropertyHandlerFlags::kHasNoSideEffect);
 
   object_template->SetHandler(config);
   object_template->SetHandler(indexed_config);

test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js

@@ -0,0 +1,33 @@
+'use strict';
+const common = require('../common');
+common.skipIfInspectorDisabled();
+
+// Test that if there is a side effect in a getter invoked through the vm
+// global proxy, Runtime.evaluate recognizes that.
+
+const assert = require('assert');
+const inspector = require('inspector');
+const vm = require('vm');
+
+const session = new inspector.Session();
+session.connect();
+
+const context = vm.createContext({
+  get a() {
+    global.foo = '1';
+    return 100;
+  }
+});
+
+session.post('Runtime.evaluate', {
+  expression: 'a',
+  throwOnSideEffect: true,
+  contextId: 2 // context's id
+}, (error, res) => {
+  assert.ifError(error);
+  const { exception } = res.exceptionDetails;
+  assert.strictEqual(exception.className, 'EvalError');
+  assert(/Possible side-effect/.test(exception.description));
+
+  assert(context);  // Keep 'context' alive and make linter happy.
+});

test/parallel/test-inspector-vm-global-accessors-sideeffects.js

@@ -0,0 +1,31 @@
+'use strict';
+const common = require('../common');
+common.skipIfInspectorDisabled();
+
+// Regression test for https://github.com/nodejs/node/issues/27518.
+
+const assert = require('assert');
+const inspector = require('inspector');
+const vm = require('vm');
+
+const session = new inspector.Session();
+session.connect();
+
+const context = vm.createContext({
+  a: 100
+});
+
+session.post('Runtime.evaluate', {
+  expression: 'a',
+  throwOnSideEffect: true,
+  contextId: 2 // context's id
+}, (error, res) => {
+  assert.ifError(error),
+  assert.deepStrictEqual(res, {
+    result: {
+      type: 'number',
+      value: context.a,
+      description: '100'
+    }
+  });
+});