[Merge chakra-core/ChakraCore@7829651f00] [1.6>1.7] [MERGE #3328 @suwc]…

… Fix Issue#3217: Reflect.construct permanently corrupts the invoked constructor Merge pull request #3328 from suwc:build/suwc/Issue3217 With the optional newTarget argument, Reflect.construct has a legitimate user case where cached constructor prototype mismatches prototype of newly created object. Therefore it is necessary to tighten requirement for sharing type by checking if constructor prototypes match. Confirmed no perf impact. Fixes #3328
nodejs · Jul 21, 2017 · 6494fd0 · 6494fd0
1 parent 7b06d0f
commit 6494fd0
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 5 deletions.
diff --git a/deps/chakrashim/core/lib/Runtime/Language/JavascriptOperators.cpp b/deps/chakrashim/core/lib/Runtime/Language/JavascriptOperators.cpp
@@ -6255,7 +6255,7 @@ namespace Js
                 if (typeHandler->IsSharable())
                 {
 #if DBG
-                    bool cachedProtoCanBeCached;
+                    bool cachedProtoCanBeCached = false;
                     Assert(type->GetPrototype() == JavascriptOperators::GetPrototypeObjectForConstructorCache(constructor, requestContext, cachedProtoCanBeCached));
                     Assert(cachedProtoCanBeCached);
                     Assert(type->GetScriptContext() == constructorCache->GetScriptContext());

diff --git a/deps/chakrashim/core/lib/Runtime/Library/JavascriptFunction.cpp b/deps/chakrashim/core/lib/Runtime/Library/JavascriptFunction.cpp
@@ -938,13 +938,15 @@ namespace Js
                 resultObject,
                 JavascriptFunction::Is(functionObj) && functionObj->GetScriptContext() == scriptContext ?
                 JavascriptFunction::FromVar(functionObj) :
-                nullptr);
+                nullptr,
+                overridingNewTarget != nullptr);
     }
 
     Var JavascriptFunction::FinishConstructor(
         const Var constructorReturnValue,
         Var newObject,
-        JavascriptFunction *const function)
+        JavascriptFunction *const function,
+        bool hasOverridingNewTarget)
     {
         Assert(constructorReturnValue);
 
@@ -954,7 +956,9 @@ namespace Js
             newObject = constructorReturnValue;
         }
 
-        if (function && function->GetConstructorCache()->NeedsUpdateAfterCtor())
+        // #3217: Cases with overriding newTarget are not what constructor cache is intended for;
+        //     Bypass constructor cache to avoid prototype mismatch/confusion.
+        if (function && function->GetConstructorCache()->NeedsUpdateAfterCtor() && !hasOverridingNewTarget)
         {
             JavascriptOperators::UpdateNewScObjectCache(function, newObject, function->GetScriptContext());
         }

diff --git a/deps/chakrashim/core/lib/Runtime/Library/JavascriptFunction.h b/deps/chakrashim/core/lib/Runtime/Library/JavascriptFunction.h
@@ -113,7 +113,7 @@ namespace Js
         static Var CallRootFunctionInScript(JavascriptFunction* func, Arguments args);
 
         static Var CallAsConstructor(Var v, Var overridingNewTarget, Arguments args, ScriptContext* scriptContext, const Js::AuxArray<uint32> *spreadIndices = nullptr);
-        static Var FinishConstructor(const Var constructorReturnValue, Var newObject, JavascriptFunction *const function);
+        static Var FinishConstructor(const Var constructorReturnValue, Var newObject, JavascriptFunction *const function, bool hasOverridingNewTarget = false);
 
 #if DBG
         static void CheckValidDebugThunk(ScriptContext* scriptContext, RecyclableObject *function);

diff --git a/deps/chakrashim/core/test/es6/classes_bugfixes.js b/deps/chakrashim/core/test/es6/classes_bugfixes.js
@@ -376,6 +376,18 @@ var tests = [
         assert.areEqual('abc', result, "result == 'abc'");
     }
   },
+  {
+    name: "Issue3217: Reflect.construct permanently corrupts the invoked constructor",
+    body: function () {
+        function Base() {}
+        function Derived() {}
+        Derived.prototype = Object.create(Base.prototype);
+
+        assert.isFalse(new Base() instanceof Derived);
+        Reflect.construct(Base, [], Derived);
+        assert.isFalse(new Base() instanceof Derived);
+    }
+  },
   {
     name: "OS12503560: assignment to super[prop] not accessing base class property",
     body: function () {