[Merge chakra-core/ChakraCore@10d2f118ed] [1.6>1.7] [MERGE #3413 @atu…

…lkatti] Fixes #3065 Generator length property needs to be set of the script function it wraps. Merge pull request #3413 from atulkatti:Issue3065.GeneratorLengthGetterSetter.1 Fixes #3065
nodejs · Aug 10, 2017 · ab2781a · ab2781a
1 parent f389099
commit ab2781a
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 2 deletions.
diff --git a/deps/chakrashim/core/lib/Runtime/Library/JavascriptFunction.cpp b/deps/chakrashim/core/lib/Runtime/Library/JavascriptFunction.cpp
@@ -135,7 +135,7 @@ namespace Js
     /* static */
     bool JavascriptFunction::IsBuiltinProperty(Var objectWithProperty, PropertyIds propertyId)
     {
-        return ScriptFunction::Is(objectWithProperty)
+        return ScriptFunctionBase::Is(objectWithProperty)
             && (propertyId == PropertyIds::length || (JavascriptFunction::FromVar(objectWithProperty)->HasRestrictedProperties() && (propertyId == PropertyIds::arguments || propertyId == PropertyIds::caller)));
     }
 #endif

diff --git a/deps/chakrashim/core/lib/Runtime/Library/JavascriptGeneratorFunction.cpp b/deps/chakrashim/core/lib/Runtime/Library/JavascriptGeneratorFunction.cpp
@@ -378,8 +378,23 @@ namespace Js
         return false;
     }
 
+    BOOL JavascriptGeneratorFunction::SetAccessors(PropertyId propertyId, Var getter, Var setter, PropertyOperationFlags flags)
+    {
+        if (propertyId == PropertyIds::length)
+        {
+            return this->scriptFunction->SetAccessors(propertyId, getter, setter, flags);
+        }
+
+        return JavascriptFunction::SetAccessors(propertyId, getter, setter, flags);
+    }
+
     BOOL JavascriptGeneratorFunction::GetAccessors(PropertyId propertyId, Var *getter, Var *setter, ScriptContext * requestContext)
     {
+        if (propertyId == PropertyIds::length)
+        {
+            return this->scriptFunction->GetAccessors(propertyId, getter, setter, requestContext);
+        }
+
         if (propertyId == PropertyIds::caller || propertyId == PropertyIds::arguments)
         {
             // JavascriptFunction has special case for caller and arguments; call DynamicObject:: virtual directly to skip that.
@@ -397,6 +412,11 @@ namespace Js
             return DynamicObject::GetSetter(propertyId, setterValue, info, requestContext);
         }
 
+        if (propertyId == PropertyIds::length)
+        {
+            return this->scriptFunction->GetSetter(propertyId, setterValue, info, requestContext);
+        }
+
         return JavascriptFunction::GetSetter(propertyId, setterValue, info, requestContext);
     }
 
@@ -405,11 +425,19 @@ namespace Js
         PropertyRecord const* propertyRecord;
         this->GetScriptContext()->FindPropertyRecord(propertyNameString, &propertyRecord);
 
-        if (propertyRecord != nullptr && (propertyRecord->GetPropertyId() == PropertyIds::caller || propertyRecord->GetPropertyId() == PropertyIds::arguments))
+        if (propertyRecord != nullptr)
+        {
+            if (propertyRecord->GetPropertyId() == PropertyIds::length)
+            {
+                return this->scriptFunction->GetSetter(propertyNameString, setterValue, info, requestContext);
+            }
+
+            if ((propertyRecord->GetPropertyId() == PropertyIds::caller || propertyRecord->GetPropertyId() == PropertyIds::arguments))
         {
             // JavascriptFunction has special case for caller and arguments; call DynamicObject:: virtual directly to skip that.
             return DynamicObject::GetSetter(propertyNameString, setterValue, info, requestContext);
         }
+        }
 
         return JavascriptFunction::GetSetter(propertyNameString, setterValue, info, requestContext);
     }

diff --git a/deps/chakrashim/core/lib/Runtime/Library/JavascriptGeneratorFunction.h b/deps/chakrashim/core/lib/Runtime/Library/JavascriptGeneratorFunction.h
@@ -59,6 +59,7 @@ namespace Js
         virtual BOOL SetProperty(PropertyId propertyId, Var value, PropertyOperationFlags flags, PropertyValueInfo* info) override;
         virtual BOOL SetProperty(JavascriptString* propertyNameString, Var value, PropertyOperationFlags flags, PropertyValueInfo* info) override;
 
+        virtual BOOL SetAccessors(PropertyId propertyId, Var getter, Var setter, PropertyOperationFlags flags = PropertyOperation_None) override;
         virtual BOOL GetAccessors(PropertyId propertyId, Var *getter, Var *setter, ScriptContext * requestContext) override;
         virtual DescriptorFlags GetSetter(PropertyId propertyId, Var *setterValue, PropertyValueInfo* info, ScriptContext* requestContext) override;
         virtual DescriptorFlags GetSetter(JavascriptString* propertyNameString, Var *setterValue, PropertyValueInfo* info, ScriptContext* requestContext) override;

diff --git a/deps/chakrashim/core/test/es6/generators-apis.js b/deps/chakrashim/core/test/es6/generators-apis.js
@@ -49,6 +49,54 @@ var tests = [
             assert.isFalse(gf.prototype.hasOwnProperty("constructor"), "Generator function prototype objects do not get a 'constructor' property");
         }
     },
+    {
+        // Regression Test GitHub Issue #3065: Generator length property is inconsistently configurable (+ assert failure in debug build) 
+        // https://github.com/Microsoft/ChakraCore/issues/3065
+        name: "Generator function length property is configurable correctly.",
+        body: function () {
+            function* gf(x, y) { }
+
+            checkAttributes("gf", gf, "length", { writable: false, enumerable: false, configurable: true });
+
+            assert.areEqual(2, gf.length, "Generator function object's 'length' property matches the number of parameters (2)");
+            gf.length = 3;
+            assert.areEqual(2, gf.length, "Generator function object's 'length' property should not be Writable");
+
+            var myLength = 4;
+            var getCalled = false;
+            var setCalled = false;
+            Object.defineProperty(gf, 'length', {
+                get: function() { getCalled = true; return myLength; },
+                set: function(value) { setCalled = true; myLength = value; }
+            });
+
+            assert.areEqual(4, gf.length, "Generator function object's 'length' property should return value from the accessor.");
+            assert.isTrue(getCalled, gf.length, "Generator function object's 'length' property should be returned from the accessor.");
+
+            getCalled = false;
+            gf.length = 5;
+            assert.areEqual(5, gf.length, "Generator function object's 'length' property should be set by the accessor.");
+            assert.isTrue(getCalled, gf.length, "Generator function object's 'length' property should be returned from the accessor.");
+
+            // Writable attribute should not be specified on this property now as it has accessors. 
+            checkAttributes("gf", gf, "length", { writable: undefined, enumerable: false, configurable: true });
+
+            getCalled = false;
+            setCalled = true;
+            var myName = "MyGeneratorFunction";
+            Object.defineProperty(gf, 'myName', {
+                get: function() { getCalled = true; return myName; },
+                set: function(value) { setCalled = true; myName = value; }
+            });
+            assert.areEqual("MyGeneratorFunction", gf.myName, "Generator function object's custom property should return proper value.");
+            assert.isTrue(getCalled, gf.myName, "Generator function object's custom property should be returned from the accessor.");
+
+            gf.myName = "MyGeneratorFunctionRenamed";
+            assert.areEqual("MyGeneratorFunctionRenamed", gf.myName, "Generator function object's custom property should be set by the accessor.");
+            assert.isTrue(setCalled, gf.myName, "Generator function object's custom property should be set using the accessor.");
+            checkAttributes("gf", gf, "myName", { writable: undefined, enumerable: false, configurable: false });
+        }
+    },
     {
         name: "arguments and caller properties are absent regardless of strictness",
         body: function () {