Prevent Array.from and TypedArray from getting @@iterator twice

Remove the JavascriptOperators::IsIterable function, which calls @@iterator getter to check if it is undefined

Fixes gh-667, fixes gh-584

lib/Runtime/Language/JavascriptOperators.cpp

@@ -743,7 +743,7 @@ namespace Js
                 aLeft = JavascriptConversion::ToPrimitive(aLeft, JavascriptHint::HintNumber, scriptContext);
             }
             //BugFix: When @@ToPrimitive of an object is overridden with a function that returns null/undefined
-            //this helper will fall into a inescapable goto loop as the checks for null/undefined were outside of the path 
+            //this helper will fall into a inescapable goto loop as the checks for null/undefined were outside of the path
             return RelationalComparisonHelper(aLeft, aRight, scriptContext, leftFirst, undefinedAs);
         }
 
@@ -10175,55 +10175,47 @@ CommonNumber:
         return TypeIds_FirstNumberType <= typeId && typeId <= TypeIds_LastNumberType;
     }
 
-    BOOL JavascriptOperators::IsIterable(RecyclableObject* instance, ScriptContext* scriptContext)
-    {
-        if (JavascriptProxy::Is(instance))
-        {
-            Var func = JavascriptOperators::GetProperty(instance, PropertyIds::_symbolIterator, scriptContext);
-            if (JavascriptOperators::IsUndefinedObject(func))
-            {
-                return FALSE;
-            }
-            else
-            {
-                return TRUE;
-            }
-        }
-        else
-        {
-            return JavascriptOperators::HasProperty(instance, PropertyIds::_symbolIterator);
-        }
-    }
-
     // GetIterator as described in ES6.0 (draft 22) Section 7.4.1
-    RecyclableObject* JavascriptOperators::GetIterator(Var iterable, ScriptContext* scriptContext)
+    RecyclableObject* JavascriptOperators::GetIterator(Var iterable, ScriptContext* scriptContext, bool optional)
     {
         RecyclableObject* iterableObj = RecyclableObject::FromVar(JavascriptOperators::ToObject(iterable, scriptContext));
-        return JavascriptOperators::GetIterator(iterableObj, scriptContext);
+        return JavascriptOperators::GetIterator(iterableObj, scriptContext, optional);
     }
 
-    RecyclableObject* JavascriptOperators::GetIteratorFunction(Var iterable, ScriptContext* scriptContext)
+    RecyclableObject* JavascriptOperators::GetIteratorFunction(Var iterable, ScriptContext* scriptContext, bool optional)
     {
         RecyclableObject* iterableObj = RecyclableObject::FromVar(JavascriptOperators::ToObject(iterable, scriptContext));
-        return JavascriptOperators::GetIteratorFunction(iterableObj, scriptContext);
+        return JavascriptOperators::GetIteratorFunction(iterableObj, scriptContext, optional);
     }
 
-    RecyclableObject* JavascriptOperators::GetIteratorFunction(RecyclableObject* instance, ScriptContext * scriptContext)
+    RecyclableObject* JavascriptOperators::GetIteratorFunction(RecyclableObject* instance, ScriptContext * scriptContext, bool optional)
     {
         Var func = JavascriptOperators::GetProperty(instance, PropertyIds::_symbolIterator, scriptContext);
 
+        if (optional && JavascriptOperators::IsUndefinedOrNull(func))
+        {
+            return nullptr;
+        }
+
         if (!JavascriptConversion::IsCallable(func))
         {
-            JavascriptError::ThrowTypeError(scriptContext, JSERR_NeedFunction);
+            JavascriptError::ThrowTypeError(scriptContext, JSERR_Property_NeedFunction);
         }
 
         RecyclableObject* function = RecyclableObject::FromVar(func);
         return function;
     }
 
-    RecyclableObject* JavascriptOperators::GetIterator(RecyclableObject* instance, ScriptContext * scriptContext)
+    RecyclableObject* JavascriptOperators::GetIterator(RecyclableObject* instance, ScriptContext * scriptContext, bool optional)
     {
-        RecyclableObject* function = GetIteratorFunction(instance, scriptContext);
+        RecyclableObject* function = GetIteratorFunction(instance, scriptContext, optional);
+
+        if (function == nullptr)
+        {
+            Assert(optional);
+            return nullptr;
+        }
+
         Var iterator = function->GetEntryPoint()(function, CallInfo(Js::CallFlags_Value, 1), instance);
 
         if (!JavascriptOperators::IsObject(iterator))

lib/Runtime/Language/JavascriptOperators.h

@@ -213,7 +213,6 @@ namespace Js
         static BOOL IsUndefinedObject(Var instance, RecyclableObject *libraryUndefined);
         static BOOL IsUndefinedObject(Var instance, JavascriptLibrary* library);
         static BOOL IsAnyNumberValue(Var instance);
-        static BOOL IsIterable(RecyclableObject* instance, ScriptContext* scriptContext);
         static BOOL IsClassConstructor(Var instance);
 
         static BOOL HasOwnItem(RecyclableObject* instance, uint32 index);
@@ -399,10 +398,10 @@ namespace Js
         static void AddFloatsToArraySegment(SparseArraySegment<double> * segment, const Js::AuxArray<double> *doubles);
         static void UpdateNewScObjectCache(Var function, Var instance, ScriptContext* requestContext);
 
-        static RecyclableObject* GetIteratorFunction(Var iterable, ScriptContext* scriptContext);
-        static RecyclableObject* GetIteratorFunction(RecyclableObject* instance, ScriptContext * scriptContext);
-        static RecyclableObject* GetIterator(Var instance, ScriptContext* scriptContext);
-        static RecyclableObject* GetIterator(RecyclableObject* instance, ScriptContext* scriptContext);
+        static RecyclableObject* GetIteratorFunction(Var iterable, ScriptContext* scriptContext, bool optional = false);
+        static RecyclableObject* GetIteratorFunction(RecyclableObject* instance, ScriptContext * scriptContext, bool optional = false);
+        static RecyclableObject* GetIterator(Var instance, ScriptContext* scriptContext, bool optional = false);
+        static RecyclableObject* GetIterator(RecyclableObject* instance, ScriptContext* scriptContext, bool optional = false);
         static RecyclableObject* IteratorNext(RecyclableObject* iterator, ScriptContext* scriptContext, Var value = nullptr);
         static bool IteratorComplete(RecyclableObject* iterResult, ScriptContext* scriptContext);
         static Var IteratorValue(RecyclableObject* iterResult, ScriptContext* scriptContext);

lib/Runtime/Library/JavascriptArray.cpp

@@ -9659,7 +9659,9 @@ Case0:
         RecyclableObject* newObj = nullptr;
         JavascriptArray* newArr = nullptr;
 
-        if (JavascriptOperators::IsIterable(items, scriptContext))
+        RecyclableObject* iterator = JavascriptOperators::GetIterator(items, scriptContext, true /* optional */);
+
+        if (iterator != nullptr)
         {
             if (constructor)
             {
@@ -9679,7 +9681,6 @@ Case0:
                 newObj = newArr;
             }
 
-            RecyclableObject* iterator = JavascriptOperators::GetIterator(items, scriptContext);
             Var nextValue;
             uint32 k = 0;
 

lib/Runtime/Library/TypedArray.cpp

@@ -46,12 +46,11 @@ namespace Js
     {
     }
 
-    inline JsUtil::List<Var, ArenaAllocator>* IterableToList(RecyclableObject *object, ScriptContext *scriptContext, ArenaAllocator *alloc)
+    inline JsUtil::List<Var, ArenaAllocator>* IteratorToList(RecyclableObject *iterator, ScriptContext *scriptContext, ArenaAllocator *alloc)
     {
-        Assert(JavascriptOperators::IsIterable(object, scriptContext));
+        Assert(iterator != nullptr);
 
         Var nextValue;
-        RecyclableObject* iterator = JavascriptOperators::GetIterator(object, scriptContext);
         JsUtil::List<Var, ArenaAllocator>* retList = JsUtil::List<Var, ArenaAllocator>::New(alloc);
 
         while (JavascriptOperators::IteratorStepAndValue(iterator, scriptContext, &nextValue))
@@ -62,7 +61,7 @@ namespace Js
         return retList;
     }
 
-    Var TypedArrayBase::CreateNewInstanceFromIterableObj(RecyclableObject *object, ScriptContext *scriptContext, uint32 elementSize, PFNCreateTypedArray pfnCreateTypedArray)
+    Var TypedArrayBase::CreateNewInstanceFromIterator(RecyclableObject *iterator, ScriptContext *scriptContext, uint32 elementSize, PFNCreateTypedArray pfnCreateTypedArray)
     {
         TypedArrayBase *newArr = nullptr;
 
@@ -70,7 +69,7 @@ namespace Js
 
         ACQUIRE_TEMP_GUEST_ALLOCATOR(tempAlloc, scriptContext, _u("Runtime"));
         {
-            JsUtil::List<Var, ArenaAllocator>* tempList = IterableToList(object, scriptContext, tempAlloc);
+            JsUtil::List<Var, ArenaAllocator>* tempList = IteratorToList(iterator, scriptContext, tempAlloc);
 
             uint32 len = tempList->Count();
             uint32 byteLen;
@@ -152,13 +151,19 @@ namespace Js
             {
                 if (JavascriptOperators::IsObject(firstArgument))
                 {
-                    Var iterator = JavascriptOperators::GetProperty(RecyclableObject::FromVar(firstArgument), PropertyIds::_symbolIterator, scriptContext);
-
-                    if (!JavascriptOperators::IsUndefinedObject(iterator) &&
-                        (iterator != scriptContext->GetLibrary()->GetArrayPrototypeValuesFunction() ||
+                    // Use GetIteratorFunction instead of GetIterator to check if it is the built-in array iterator
+                    RecyclableObject* iteratorFn = JavascriptOperators::GetIteratorFunction(firstArgument, scriptContext, true /* optional */);
+                    if (iteratorFn != nullptr &&
+                        (iteratorFn != scriptContext->GetLibrary()->GetArrayPrototypeValuesFunction() ||
                             !JavascriptArray::Is(firstArgument) || ArrayIteratorPrototypeHasUserDefinedNext(scriptContext) ))
                     {
-                        return CreateNewInstanceFromIterableObj(RecyclableObject::FromVar(firstArgument), scriptContext, elementSize, pfnCreateTypedArray);
+                        Var iterator = iteratorFn->GetEntryPoint()(iteratorFn, CallInfo(Js::CallFlags_Value, 1), RecyclableObject::FromVar(firstArgument));
+
+                        if (!JavascriptOperators::IsObject(iterator))
+                        {
+                            JavascriptError::ThrowTypeError(scriptContext, JSERR_NeedObject);
+                        }
+                        return CreateNewInstanceFromIterator(RecyclableObject::FromVar(iterator), scriptContext, elementSize, pfnCreateTypedArray);
                     }
                     else if (JavascriptConversion::ToObject(firstArgument, scriptContext, &jsArraySource))
                     {
@@ -1429,8 +1434,9 @@ namespace Js
         }
 
         Var newObj;
+        RecyclableObject* iterator = JavascriptOperators::GetIterator(items, scriptContext, true /* optional */);
 
-        if (JavascriptOperators::IsIterable(items, scriptContext))
+        if (iterator != nullptr)
         {
             DECLARE_TEMP_GUEST_ALLOCATOR(tempAlloc);
 
@@ -1445,7 +1451,7 @@ namespace Js
                 //       for types we know such as TypedArray. We know the length of a TypedArray but we still
                 //       have to be careful in case there is a proxy which could return anything from [[Get]]
                 //       or the built-in @@iterator has been replaced.
-                JsUtil::List<Var, ArenaAllocator>* tempList = IterableToList(items, scriptContext, tempAlloc);
+                JsUtil::List<Var, ArenaAllocator>* tempList = IteratorToList(iterator, scriptContext, tempAlloc);
 
                 uint32 len = tempList->Count();
 

lib/Runtime/Library/TypedArray.h

@@ -167,7 +167,7 @@ namespace Js
 
     protected:
         inline BOOL IsBuiltinProperty(PropertyId);
-        static Var CreateNewInstanceFromIterableObj(RecyclableObject *object, ScriptContext *scriptContext, uint32 elementSize, PFNCreateTypedArray pfnCreateTypedArray);
+        static Var CreateNewInstanceFromIterator(RecyclableObject *iterator, ScriptContext *scriptContext, uint32 elementSize, PFNCreateTypedArray pfnCreateTypedArray);
         static Var CreateNewInstance(Arguments& args, ScriptContext* scriptContext, uint32 elementSize, PFNCreateTypedArray pfnCreateTypedArray );
         static int32 ToLengthChecked(Var lengthVar, uint32 elementSize, ScriptContext* scriptContext);
         static bool ArrayIteratorPrototypeHasUserDefinedNext(ScriptContext *scriptContext);

test/es6/ES6ArrayAPI.js

@@ -278,6 +278,12 @@ var tests = [
             assert.throws(function() { Array.from(objectWithIteratorWhichDoesNotReturnObjects); }, TypeError, "obj[@@iterator] must return an object", "Object expected");
             assert.throws(function() { Array.from(objectWithIteratorNextIsNotAFunction); }, TypeError, "obj[@@iterator].next must be a function", "Function expected");
             assert.throws(function() { Array.from(objectWithIteratorNextDoesNotReturnObjects); }, TypeError, "obj[@@iterator].next must return an object", "Object expected");
+
+            var objectWithUndefinedIterator = { 0: "a", 1: "b", length: 2, [Symbol.iterator]: undefined };
+            var objectWithNullIterator = { 0: "a", 1: "b", length: 2, [Symbol.iterator]: null };
+
+            assert.areEqual([ "a", "b" ], Array.from(objectWithUndefinedIterator), "'@@iterator: undefined' is ignored");
+            assert.areEqual([ "a", "b" ], Array.from(objectWithNullIterator), "'@@iterator: null' is ignored");
         }
     },
     {
@@ -333,6 +339,31 @@ var tests = [
             assert.throws(function() { Array.from(o); }, RangeError, "Array.from uses abstract operation ArrayCreate which throws RangeError when requested length > 2^32-1", "Array length must be a finite positive integer");
         }
     },
+    {
+        name: "Array.from doesn't get @@iterator twice",
+        body: function () {
+            let count = 0;
+            Array.from({
+                get [Symbol.iterator]() {
+                    count++;
+                    return [][Symbol.iterator];
+                }
+            });
+            assert.areEqual(count, 1, "Array.from calls @@iterator getter once");
+
+            count = 0;
+            Array.from(new Proxy({}, {
+                get(target, property) {
+                    if (property === Symbol.iterator) {
+                        count++;
+                        return [][Symbol.iterator];
+                    }
+                    return Reflect.get(target, property);
+                }
+            }));
+            assert.areEqual(count, 1, "Array.from calls proxy's getter with @@iterator as parameter only once");
+        }
+    },
     {
         name: "Array#fill called with an object that has length > 2^32-1",
         body: function() {

test/typedarray/TypedArrayBuiltins.js

@@ -3,7 +3,7 @@
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 
-// Verifies TypedArray builtin properties 
+// Verifies TypedArray builtin properties
 
 if (this.WScript && this.WScript.LoadScriptFile) { // Check for running in ch
     this.WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
@@ -23,11 +23,11 @@ var tests = [
         body: function () {
             var arr = new ArrayBuffer(100);
             var u8 = new Uint8Array(arr, 90);
-            
+
             for (var i = 0; i < u8.length; i++) {
                 u8[i] = i;
             }
-            
+
             mangle(u8);
 
             assert.areEqual(10, u8.length, "Writing to length has no effect");
@@ -166,6 +166,52 @@ var tests = [
                 Object.defineProperty(arrayIteratorProto, "next", builtinArrayPrototypeIteratorNextDesc);
             })();
         }
+    },
+    {
+        name: "TypedArray constructor and TypedArray.from don't get @@iterator twice",
+        body: function () {
+            let count = 0;
+            new Uint8Array({
+                get [Symbol.iterator]() {
+                    count++;
+                    return [][Symbol.iterator];
+                }
+            });
+            assert.areEqual(count, 1, "TypedArray constructor calls @@iterator getter once");
+
+            count = 0;
+            new Uint8Array(new Proxy({}, {
+                get(target, property) {
+                    if (property === Symbol.iterator) {
+                        count++;
+                        return [][Symbol.iterator];
+                    }
+                    return Reflect.get(target, property);
+                }
+            }));
+            assert.areEqual(count, 1, "TypedArray constructor calls proxy's getter with @@iterator as parameter only once");
+
+            count = 0;
+            Uint8Array.from({
+                get [Symbol.iterator]() {
+                    count++;
+                    return [][Symbol.iterator];
+                }
+            });
+            assert.areEqual(count, 1, "TypedArray.from calls @@iterator getter once");
+
+            count = 0;
+            Uint8Array.from(new Proxy({}, {
+                get(target, property) {
+                    if (property === Symbol.iterator) {
+                        count++;
+                        return [][Symbol.iterator];
+                    }
+                    return Reflect.get(target, property);
+                }
+            }));
+            assert.areEqual(count, 1, "TypedArray.from calls proxy's getter with @@iterator as parameter only once");
+        }
     }
 ];