Changeset 262165 in webkit

Timestamp:

May 26, 2020 3:16:10 PM (4 years ago)

Author:

Alexey Shvayka

Message:

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]: ​https://github.com/tc39/ecma262/pull/1408

• bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::emitGenericEnumeration):
(JSC::BytecodeGenerator::emitEnumeration):

• runtime/IteratorOperations.cpp:

(JSC::iteratorClose):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/custom-iterators.js (modified) (1 diff)
• JSTests/stress/iterator-return-abrupt-lookup-builtins.js (added)
• JSTests/test262/expectations.yaml (modified) (2 diffs)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp (modified) (2 diffs)
• Source/JavaScriptCore/runtime/IteratorOperations.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-05-26  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        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  <ticaiolima@gmail.com>
 

trunk/JSTests/stress/custom-iterators.js

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

trunk/JSTests/test262/expectations.yaml

@@ -3259 +3259 @@
 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.'
@@ -3476 +3473 @@
   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.'

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-05-26  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        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  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -4083 +4083 @@
             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());
 
@@ -4239 +4225 @@
             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());
 

trunk/Source/JavaScriptCore/runtime/IteratorOperations.cpp

@@ -94 +94 @@
         catchScope.clearException();
     }
+
     JSValue returnFunction = iterationRecord.iterator.get(globalObject, vm.propertyNames->returnKeyword);
-    RETURN_IF_EXCEPTION(throwScope, void());
-
-    if (returnFunction.isUndefined()) {
+    if (UNLIKELY(throwScope.exception()) || returnFunction.isUndefined()) {
         if (exception)
             throwException(globalObject, throwScope, exception);