The interceptor should not copy a value onto the sandbox, if setting on

the global object will fail because we are in strict mode.

Function declarations should not throw, we expect that they are not set
when we define them, so copy them over!

Add test for setting variable in strict mode for issue nodejs#5344.

deps/v8/src/runtime/runtime-scopes.cc

@@ -44,12 +44,14 @@ static Object* DeclareGlobals(Isolate* isolate, Handle<JSGlobalObject> global,
   }
 
   // Do the lookup own properties only, see ES5 erratum.
-  LookupIterator::Configuration configuration =
-      LookupIterator::Configuration::HIDDEN_SKIP_INTERCEPTOR;
+  LookupIterator::Configuration configuration(
+      LookupIterator::Configuration::HIDDEN_SKIP_INTERCEPTOR);
+  Object::ShouldThrow should_throw(Object::ShouldThrow::THROW_ON_ERROR);
   if (is_function) {
     // for function declarations, use the interceptor on the declaration,
     // otherwise use it only on initialization
     configuration = LookupIterator::Configuration::DEFAULT;
+    should_throw = Object::ShouldThrow::DONT_THROW;
   }
   LookupIterator it(global, name, global, configuration);
   Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
@@ -91,8 +93,11 @@ static Object* DeclareGlobals(Isolate* isolate, Handle<JSGlobalObject> global,
   }
 
   // Define or redefine own property.
+  Maybe<bool> res = JSObject::DefineOwnPropertyIgnoreAttributes(
+      &it, value, attr, should_throw);
+
   RETURN_FAILURE_ON_EXCEPTION(
-      isolate, JSObject::DefineOwnPropertyIgnoreAttributes(&it, value, attr));
+      isolate, res.IsNothing() ? MaybeHandle<Object>() : value);
 
   return isolate->heap()->undefined_value();
 }

src/node_contextify.cc

@@ -303,7 +303,15 @@ class ContextifyContext {
     if (ctx->context_.IsEmpty())
       return;
 
-    ctx->sandbox()->Set(property, value);
+    Maybe<bool> has = ctx->global_proxy()->
+        HasRealNamedProperty(ctx->context(), property);
+
+    bool isContextualStore = ctx->global_proxy() != args.This();
+
+    if (!args.ShouldThrowOnError() || has.FromJust()
+        || !isContextualStore ) {
+      ctx->sandbox()->Set(property, value);
+    }
   }
 
 

test/parallel/test-vm-global-define-property.js

@@ -49,7 +49,8 @@ code =
     '    enumerable: false, ' +
     '    configurable: false, ' +
     '    value: 20 ' +
-    '  }); ' +
+    '  });' +
+    'global.x = 100' +
     '})()';
 
 var global = {};

test/parallel/test-vm-strict-mode.js

@@ -0,0 +1,22 @@
+// https://github.com/nodejs/node/issues/5344
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+const ctx = vm.createContext();
+
+vm.runInContext('w = 1;', ctx);
+assert.equal(1, ctx.w);
+
+assert.throws(function() {vm.runInContext('"use strict"; x = 1;', ctx);});
+assert.equal(undefined, ctx.x);
+
+vm.runInContext('"use strict"; var y = 1;', ctx);
+assert.equal(1, ctx.y);
+
+vm.runInContext('"use strict"; this.z = 1;', ctx);
+assert.equal(1, ctx.z);
+
+// w has been defined
+vm.runInContext('"use strict"; w = 2;', ctx);
+assert.equal(2, ctx.w);