[JSC] Iterator.prototype.reduce() doesn't properly forward return() call to underlying iterator

Alexey Shvayka · Alexey Shvayka · commit a6b23120280d · 2024-10-03T17:39:31.000-07:00
https://bugs.webkit.org/show_bug.cgi?id=280813 <rdar://problem/137181340> Reviewed by Yusuke Suzuki. If an exception occurs in `reducer` [1], for/of correctly retrieves "return" method from the underlying iterator (via forwarding getter on the wrapper), yet it calls the method with `this` value of the wrapper instead of underlying iterator, which for example makes Generator.prototype.return() throw a TypeError. This is a very subtle bug because if IteratorClose is called due to throw completion, it "swallows" throw completion of return() call [2]. While this can be fixed by rewriting iterator wrapper just like one in flatMap(), this patch instead utilizes @iteratorGenericNext() and @ifAbruptCloseIterator() to get rid of the wrapper for performance and code expressiveness reasons. [1]: https://tc39.es/proposal-iterator-helpers/#sec-iteratorprototype.reduce (steps 7.c-d) [2]: https://tc39.es/ecma262/#sec-iteratorclose (steps 5-6) * JSTests/stress/iterator-prototype-reduce.js: (sameValue.const.gen): (sameValue): * Source/JavaScriptCore/builtins/JSIteratorPrototype.js: (reduce): (reduce.wrapper.return.next): Deleted. (reduce.wrapper.return.get return): Deleted. (reduce.wrapper.iterator): Deleted. Canonical link: https://commits.webkit.org/284648@main
diff --git a/JSTests/stress/iterator-prototype-reduce.js b/JSTests/stress/iterator-prototype-reduce.js
@@ -136,3 +136,27 @@ function shouldThrow(fn, error, message) {
     sameValue(nextGetCount, 1);
     sameValue(returnGetCount, 1);
 }
+
+{
+    let finallyReached = false;
+    let yield3Reached = false;
+    const gen = (function*() {
+        yield 1;
+
+        try { yield 2; }
+        finally { finallyReached = true; }
+
+        yield3Reached = true;
+        yield 3;
+    })();
+
+    shouldThrow(() => {
+        gen.reduce((_, value) => {
+            if (value === 2)
+                throw new Error("my error");
+        });
+    }, Error, "my error");
+
+    assert(finallyReached, "Generator.prototype.return() invoked the generator function");
+    sameValue(yield3Reached, false);
+}
diff --git a/Source/JavaScriptCore/builtins/JSIteratorPrototype.js b/Source/JavaScriptCore/builtins/JSIteratorPrototype.js
@@ -283,33 +283,29 @@ function reduce(reducer /*, initialValue */)
 
     var initialValue = @argument(1);
 
-    var iteratedIterator = this;
+    var iterated = this;
     var iteratedNextMethod = this.next;
 
     var accumulator;
     var counter = 0;
     if (initialValue === @undefined) {
-        var result = iteratedNextMethod.@call(iteratedIterator);
-        if (!@isObject(result))
-            @throwTypeError("Iterator result interface is not an object.");
+        var result = @iteratorGenericNext(iteratedNextMethod, iterated);
         if (result.done)
             @throwTypeError("Iterator.prototype.reduce requires an initial value or an iterator that is not done.");
         accumulator = result.value;
         counter = 1;
     } else
         accumulator = initialValue;
 
-    var wrapper = {
-        @@iterator: function()
-            {
-                return {
-                    next: function() { return iteratedNextMethod.@call(iteratedIterator); },
-                    get return() { return iteratedIterator.return; },
-                };
-            },
-    };
-    for (var item of wrapper) {
-        accumulator = reducer(accumulator, item, counter++);
+    for (;;) {
+        var result = @iteratorGenericNext(iteratedNextMethod, iterated);
+        if (result.done)
+            break;
+
+        var value = result.value;
+        @ifAbruptCloseIterator(iterated, (
+            accumulator = reducer(accumulator, value, counter++)
+        ));
     }
 
     return accumulator;
