IteratorClose should suppress GetMethod errors

https://bugs.webkit.org/show_bug.cgi?id=212378

Reviewed by Keith Miller.

JSTests:

* stress/custom-iterators.js:
* stress/iterator-return-abrupt-lookup-builtins.js: Added.
* test262/expectations.yaml: Mark 4 test cases as passing.

Source/JavaScriptCore:

This patch implements recent spec change [1] that prevents "return" method lookup error
from overriding outer exception, aligning JSC with V8 and SpiderMonkey.

It is accomplished by moving pushTry() before emitGetById() in BytecodeGenerator.cpp
(covered by test262 suite) and removal of RETURN_IF_EXCEPTION in IteratorOperations.cpp
(added a stress test).

Before this patch, JSC partly implemented the spec change [1] by suppressing TypeError
if "return" method of iterator was not callable.

BytecodeGenerator::emitDelegateYield() is intentionally left unchanged.
Also, this patch utilizes emitIteratorGenericClose() to avoid code duplication.
for/of microbenchmarks are neutral.

[1]: tc39/ecma262#1408

* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::emitGenericEnumeration):
(JSC::BytecodeGenerator::emitEnumeration):
* runtime/IteratorOperations.cpp:
(JSC::iteratorClose):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@262165 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Author: [email protected]

JSTests/ChangeLog

@@ -1,3 +1,14 @@
+2020-05-26  Alexey Shvayka  <[email protected]>
+
+        IteratorClose should suppress GetMethod errors
+        https://bugs.webkit.org/show_bug.cgi?id=212378
+
+        Reviewed by Keith Miller.
+
+        * stress/custom-iterators.js:
+        * stress/iterator-return-abrupt-lookup-builtins.js: Added.
+        * test262/expectations.yaml: Mark 4 test cases as passing.
+
 2020-05-23  Caio Lima  <[email protected]>
 
         [bmalloc] Fix OOM errors on MIPS after r261667

JSTests/stress/custom-iterators.js

@@ -135,7 +135,7 @@ try {
         throw "Error: Terminate iteration.";
     }
 } catch (e) {
-    if (String(e) !== "Error: looking up return.")
+    if (String(e) !== "Error: Terminate iteration.")
         throw e;
 }
 

JSTests/stress/iterator-return-abrupt-lookup-builtins.js

@@ -0,0 +1,28 @@
+function shouldThrow(func, expectedError) {
+    let actualError;
+    try {
+        func();
+    } catch (error) {
+        actualError = error;
+    }
+    if (actualError !== expectedError)
+        throw new Error(`bad error: ${actualError}`);
+}
+
+const iter = {
+    [Symbol.iterator]() { return this; },
+    next() { return { value: [], done: false }; },
+    get return() { throw 'return'; },
+};
+
+Map.prototype.set = () => { throw 'set'; };
+Set.prototype.add = () => { throw 'add'; };
+WeakMap.prototype.set = () => { throw 'set'; };
+WeakSet.prototype.add = () => { throw 'add'; };
+
+for (let i = 0; i < 1e4; ++i) {
+    shouldThrow(() => new Map(iter), 'set');
+    shouldThrow(() => new Set(iter), 'add');
+    shouldThrow(() => new WeakMap(iter), 'set');
+    shouldThrow(() => new WeakSet(iter), 'add');
+}

JSTests/test262/expectations.yaml

@@ -3258,9 +3258,6 @@ test/language/statements/for-await-of/async-func-decl-dstr-array-elem-nested-obj
   strict mode: 'Test262: This statement should not be evaluated.'
 test/language/statements/for-await-of/async-func-decl-dstr-array-elem-target-simple-strict.js:
   strict mode: 'Test262: This statement should not be evaluated.'
-test/language/statements/for-await-of/iterator-close-throw-get-method-abrupt.js:
-  default: 'Test262:AsyncTestFailure:Test262Error: Test262Error: Expected SameValue(«function Object() {'
-  strict mode: 'Test262:AsyncTestFailure:Test262Error: Test262Error: Expected SameValue(«function Object() {'
 test/language/statements/for-await-of/let-array-with-newline.js:
   default: 'Test262: This statement should not be evaluated.'
 test/language/statements/for-await-of/ticks-with-async-iter-resolved-promise-and-constructor-lookup-two.js:
@@ -3475,9 +3472,6 @@ test/language/statements/for-of/head-lhs-non-asnmt-trgt.js:
 test/language/statements/for-of/head-var-no-expr.js:
   default: 'Test262: This statement should not be evaluated.'
   strict mode: 'Test262: This statement should not be evaluated.'
-test/language/statements/for-of/iterator-close-throw-get-method-abrupt.js:
-  default: 'Test262Error: Expected a Test262Error but got a Object'
-  strict mode: 'Test262Error: Expected a Test262Error but got a Object'
 test/language/statements/for-of/let-array-with-newline.js:
   default: 'Test262: This statement should not be evaluated.'
 test/language/statements/for/head-lhs-let.js:

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,32 @@
+2020-05-26  Alexey Shvayka  <[email protected]>
+
+        IteratorClose should suppress GetMethod errors
+        https://bugs.webkit.org/show_bug.cgi?id=212378
+
+        Reviewed by Keith Miller.
+
+        This patch implements recent spec change [1] that prevents "return" method lookup error
+        from overriding outer exception, aligning JSC with V8 and SpiderMonkey.
+
+        It is accomplished by moving pushTry() before emitGetById() in BytecodeGenerator.cpp
+        (covered by test262 suite) and removal of RETURN_IF_EXCEPTION in IteratorOperations.cpp
+        (added a stress test).
+
+        Before this patch, JSC partly implemented the spec change [1] by suppressing TypeError
+        if "return" method of iterator was not callable.
+
+        BytecodeGenerator::emitDelegateYield() is intentionally left unchanged.
+        Also, this patch utilizes emitIteratorGenericClose() to avoid code duplication.
+        for/of microbenchmarks are neutral.
+
+        [1]: https://github.com/tc39/ecma262/pull/1408
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::emitGenericEnumeration):
+        (JSC::BytecodeGenerator::emitEnumeration):
+        * runtime/IteratorOperations.cpp:
+        (JSC::iteratorClose):
+
 2020-05-26  Mark Lam  <[email protected]>
 
         SamplingProfiler::takeSample() should not assume that ENABLE(WEBASSEMBLY) means Wasm is enabled.

Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -4082,26 +4082,12 @@ void BytecodeGenerator::emitGenericEnumeration(ThrowableExpressionData* node, Ex
             emitLabel(finallyBodyLabel.get());
             restoreScopeRegister();
 
-            Ref<Label> finallyDone = newLabel();
-
-            RefPtr<RegisterID> returnMethod = emitGetById(newTemporary(), iterator.get(), propertyNames().returnKeyword);
-            emitJumpIfTrue(emitIsUndefined(newTemporary(), returnMethod.get()), finallyDone.get());
-
             Ref<Label> returnCallTryStart = newLabel();
             emitLabel(returnCallTryStart.get());
             TryData* returnCallTryData = pushTry(returnCallTryStart.get(), catchLabel.get(), HandlerType::SynthesizedCatch);
 
-            CallArguments returnArguments(*this, nullptr);
-            move(returnArguments.thisRegister(), iterator.get());
-            emitCall(value.get(), returnMethod.get(), NoExpectedFunction, returnArguments, node->divot(), node->divotStart(), node->divotEnd(), DebuggableCall::No);
-
-            if (isForAwait)
-                emitAwait(value.get());
-
-            emitJumpIfTrue(emitIsObject(newTemporary(), value.get()), finallyDone.get());
-            emitThrowTypeError("Iterator result interface is not an object."_s);
-
-            emitLabel(finallyDone.get());
+            emitIteratorGenericClose(iterator.get(), node, shouldEmitAwait);
+            Ref<Label> finallyDone = newEmittedLabel();
             emitFinallyCompletion(finallyContext, endCatchLabel.get());
 
             popTry(returnCallTryData, finallyDone.get());
@@ -4238,23 +4224,12 @@ void BytecodeGenerator::emitEnumeration(ThrowableExpressionData* node, Expressio
             emitLabel(finallyBodyLabel.get());
             restoreScopeRegister();
 
-            Ref<Label> finallyDone = newLabel();
-
-            RefPtr<RegisterID> returnMethod = emitGetById(newTemporary(), iterator.get(), propertyNames().returnKeyword);
-            emitJumpIfTrue(emitIsUndefined(newTemporary(), returnMethod.get()), finallyDone.get());
-
             Ref<Label> returnCallTryStart = newLabel();
             emitLabel(returnCallTryStart.get());
             TryData* returnCallTryData = pushTry(returnCallTryStart.get(), catchLabel.get(), HandlerType::SynthesizedCatch);
 
-            CallArguments returnArguments(*this, nullptr);
-            move(returnArguments.thisRegister(), iterator.get());
-            emitCall(value.get(), returnMethod.get(), NoExpectedFunction, returnArguments, node->divot(), node->divotStart(), node->divotEnd(), DebuggableCall::No);
-
-            emitJumpIfTrue(emitIsObject(newTemporary(), value.get()), finallyDone.get());
-            emitThrowTypeError("Iterator result interface is not an object."_s);
-
-            emitLabel(finallyDone.get());
+            emitIteratorGenericClose(iterator.get(), node, EmitAwait::No);
+            Ref<Label> finallyDone = newEmittedLabel();
             emitFinallyCompletion(finallyContext, endCatchLabel.get());
 
             popTry(returnCallTryData, finallyDone.get());

Source/JavaScriptCore/runtime/IteratorOperations.cpp

@@ -93,10 +93,9 @@ void iteratorClose(JSGlobalObject* globalObject, IterationRecord iterationRecord
         exception = catchScope.exception();
         catchScope.clearException();
     }
-    JSValue returnFunction = iterationRecord.iterator.get(globalObject, vm.propertyNames->returnKeyword);
-    RETURN_IF_EXCEPTION(throwScope, void());
 
-    if (returnFunction.isUndefined()) {
+    JSValue returnFunction = iterationRecord.iterator.get(globalObject, vm.propertyNames->returnKeyword);
+    if (UNLIKELY(throwScope.exception()) || returnFunction.isUndefined()) {
         if (exception)
             throwException(globalObject, throwScope, exception);
         return;