Close iterator after error in Array.from

Issue #17

2 files changed, 215 insertions, 14 deletions

diff --git a/js/src/builtin/Array.js b/js/src/builtin/Array.js
index 54b47b72f5..5ab0b71be5 100644
--- a/js/src/builtin/Array.js
+++ b/js/src/builtin/Array.js
@@ -784,7 +784,7 @@ function ArrayKeys() {
     return CreateArrayIterator(this, ITEM_KIND_KEY);
 }
 
-// ES6 draft rev31 (2015/01/15) 22.1.2.1 Array.from(source[, mapfn[, thisArg]]).
+// ES 2017 draft 0f10dba4ad18de92d47d421f378233a2eae8f077 22.1.2.1
 function ArrayFrom(items, mapfn=undefined, thisArg=undefined) {
     // Step 1.
     var C = this;
@@ -795,43 +795,61 @@ function ArrayFrom(items, mapfn=undefined, thisArg=undefined) {
         ThrowTypeError(JSMSG_NOT_FUNCTION, DecompileArg(1, mapfn));
     var T = thisArg;
 
-    // Steps 4-5.
+    // Step 4.
     var usingIterator = GetMethod(items, std_iterator);
 
-    // Step 6.
+    // Step 5.
     if (usingIterator !== undefined) {
-        // Steps 6.a-c.
+        // Steps 5.a-c.
         var A = IsConstructor(C) ? new C() : [];
 
-        // Steps 6.d-e.
+        // Step 5.c.
         var iterator = GetIterator(items, usingIterator);
 
-        // Step 6.f.
+        // Step 5.d.
         var k = 0;
 
-        // Step 6.g.
+        // Step 5.e.
         // These steps cannot be implemented using a for-of loop.
         // See <https://bugs.ecmascript.org/show_bug.cgi?id=2883>.
         while (true) {
-            // Steps 6.g.i-iii.
+            // Step 5.e.i.
+            // Disabled for performance reason.  We won't hit this case on
+            // normal array, since _DefineDataProperty will throw before it.
+            // We could hit this when |A| is a proxy and it ignores
+            // |_DefineDataProperty|, but it happens only after too long loop.
+            /*
+            if (k >= 0x1fffffffffffff) {
+                IteratorCloseThrow(iterator);
+                ThrowTypeError(JSMSG_TOO_LONG_ARRAY);
+            }
+            */
+
+            // Step 5.e.iii.
             var next = callContentFunction(iterator.next, iterator);
             if (!IsObject(next))
                 ThrowTypeError(JSMSG_NEXT_RETURNED_PRIMITIVE);
 
-            // Step 6.g.iv.
+            // Step 5.e.iv.
             if (next.done) {
                 A.length = k;
                 return A;
             }
 
-            // Steps 6.g.v-vi.
+            // Steps 5.e.v.
             var nextValue = next.value;
 
-            // Steps 6.g.vii-viii.
-            var mappedValue = mapping ? callContentFunction(mapfn, thisArg, nextValue, k) : nextValue;
+            // Steps 5.e.vi-vii.
+            try {
+                var mappedValue = mapping ? callContentFunction(mapfn, thisArg, nextValue, k) : nextValue;
 
-            // Steps 6.g.ix-xi.
-            _DefineDataProperty(A, k++, mappedValue);
+                // Steps 5.e.ii (reordered), 5.e.viii.
+                _DefineDataProperty(A, k++, mappedValue);
+            } catch (e) {
+                // Steps 5.e.vi.2, 5.e.ix.
+                IteratorCloseThrow(iterator);
+                throw e;
+            }
         }
     }
 
diff --git a/js/src/tests/ecma_6/Array/from-iterator-close.js b/js/src/tests/ecma_6/Array/from-iterator-close.js
new file mode 100644
index 0000000000..fa97ea282a
--- /dev/null
+++ b/js/src/tests/ecma_6/Array/from-iterator-close.js
@@ -0,0 +1,183 @@
+var BUGNUMBER = 1180306;
+var summary = 'Array.from should close iterator on error';
+
+print(BUGNUMBER + ": " + summary);
+
+function test(ctor, { mapVal=undefined,
+                      nextVal=undefined,
+                      nextThrowVal=undefined,
+                      modifier=undefined,
+                      exceptionVal=undefined,
+                      exceptionType=undefined,
+                      closed=true }) {
+    let iterable = {
+        closed: false,
+        [Symbol.iterator]() {
+            let iterator = {
+                first: true,
+                next() {
+                    if (this.first) {
+                        this.first = false;
+                        if (nextThrowVal)
+                            throw nextThrowVal;
+                        return nextVal;
+                    }
+                    return { value: undefined, done: true };
+                },
+                return() {
+                    iterable.closed = true;
+                    return {};
+                }
+            };
+            if (modifier)
+                modifier(iterator, iterable);
+
+            return iterator;
+        }
+    };
+    if (exceptionVal) {
+        let caught = false;
+        try {
+            ctor.from(iterable, mapVal);
+        } catch (e) {
+            assertEq(e, exceptionVal);
+            caught = true;
+        }
+        assertEq(caught, true);
+    } else if (exceptionType) {
+        assertThrowsInstanceOf(() => ctor.from(iterable, mapVal), exceptionType);
+    } else {
+        ctor.from(iterable, mapVal);
+    }
+    assertEq(iterable.closed, closed);
+}
+
+// == Error cases with close ==
+
+// ES 2017 draft 22.1.2.1 step 5.e.i.1.
+// Cannot test.
+
+// ES 2017 draft 22.1.2.1 step 5.e.vi.2.
+test(Array, {
+    mapVal: () => { throw "map throws"; },
+    nextVal: { value: 1, done: false },
+    exceptionVal: "map throws",
+    closed: true,
+});
+
+// ES 2017 draft 22.1.2.1 step 5.e.ix.
+class MyArray extends Array {
+    constructor() {
+        return new Proxy({}, {
+            defineProperty() {
+                throw "defineProperty throws";
+            }
+        });
+    }
+}
+test(MyArray, {
+    nextVal: { value: 1, done: false },
+    exceptionVal: "defineProperty throws",
+    closed: true,
+});
+
+// ES 2017 draft 7.4.6 step 3.
+// if GetMethod fails, the thrown value should be used.
+test(MyArray, {
+    nextVal: { value: 1, done: false },
+    modifier: (iterator, iterable) => {
+        Object.defineProperty(iterator, "return", {
+            get: function() {
+                iterable.closed = true;
+                throw "return getter throws";
+            }
+        });
+    },
+    exceptionVal: "return getter throws",
+    closed: true,
+});
+test(MyArray, {
+    nextVal: { value: 1, done: false },
+    modifier: (iterator, iterable) => {
+        Object.defineProperty(iterator, "return", {
+            get: function() {
+                iterable.closed = true;
+                return "non object";
+            }
+        });
+    },
+    exceptionType: TypeError,
+    closed: true,
+});
+test(MyArray, {
+    nextVal: { value: 1, done: false },
+    modifier: (iterator, iterable) => {
+        Object.defineProperty(iterator, "return", {
+            get: function() {
+                iterable.closed = true;
+                // Non callable.
+                return {};
+            }
+        });
+    },
+    exceptionType: TypeError,
+    closed: true,
+});
+
+// ES 2017 draft 7.4.6 steps 6.
+// if return method throws, the thrown value should be ignored.
+test(MyArray, {
+    nextVal: { value: 1, done: false },
+    modifier: (iterator, iterable) => {
+        iterator.return = function() {
+            iterable.closed = true;
+            throw "return throws";
+        };
+    },
+    exceptionVal: "defineProperty throws",
+    closed: true,
+});
+
+test(MyArray, {
+    nextVal: { value: 1, done: false },
+    modifier: (iterator, iterable) => {
+        iterator.return = function() {
+            iterable.closed = true;
+            return "non object";
+        };
+    },
+    exceptionVal: "defineProperty throws",
+    closed: true,
+});
+
+// == Error cases without close ==
+
+// ES 2017 draft 22.1.2.1 step 5.e.iii.
+test(Array, {
+    nextThrowVal: "next throws",
+    exceptionVal: "next throws",
+    closed: false,
+});
+
+test(Array, {
+    nextVal: { value: {}, get done() { throw "done getter throws"; } },
+    exceptionVal: "done getter throws",
+    closed: false,
+});
+
+// ES 2017 draft 22.1.2.1 step 5.e.v.
+test(Array, {
+    nextVal: { get value() { throw "value getter throws"; }, done: false },
+    exceptionVal: "value getter throws",
+    closed: false,
+});
+
+// == Successful cases ==
+
+test(Array, {
+    nextVal: { value: 1, done: false },
+    closed: false,
+});
+
+if (typeof reportCompare === 'function')
+  reportCompare(true, true);