Bug 1629795 - Part 3: Ignore exceptions in IteratorClose for Throw completions. r=arai

Implements the changes from: https://github.com/tc39/ecma262/pull/1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820

--HG--
extra : moz-landing-system : lando

js/src/frontend/BytecodeEmitter.cpp

@@ -2857,17 +2857,49 @@ bool BytecodeEmitter::emitIteratorCloseInScope(
       ".close() on iterators is prohibited in self-hosted code because it "
       "can run user-modifiable iteration code");
 
-  // Generate inline logic corresponding to IteratorClose (ES 7.4.6).
+  // Generate inline logic corresponding to IteratorClose (ES2021 7.4.6) and
+  // AsyncIteratorClose (ES2021 7.4.7). Steps numbers apply to both operations.
   //
   // Callers need to ensure that the iterator object is at the top of the
   // stack.
 
+  // For non-Throw completions, we emit the equivalent of:
+  //
+  // var returnMethod = GetMethod(iterator, "return");
+  // if (returnMethod !== undefined) {
+  //   var innerResult = [Await] Call(returnMethod, iterator);
+  //   CheckIsObj(innerResult);
+  // }
+  //
+  // Whereas for Throw completions, we emit:
+  //
+  // try {
+  //   var returnMethod = GetMethod(iterator, "return");
+  //   if (returnMethod !== undefined) {
+  //     [Await] Call(returnMethod, iterator);
+  //   }
+  // } catch {}
+
+  Maybe<TryEmitter> tryCatch;
+
+  if (completionKind == CompletionKind::Throw) {
+    tryCatch.emplace(this, TryEmitter::Kind::TryCatch,
+                     TryEmitter::ControlKind::NonSyntactic);
+
+    if (!tryCatch->emitTry()) {
+      //            [stack] ... ITER
+      return false;
+    }
+  }
+
   if (!emit1(JSOp::Dup)) {
     //              [stack] ... ITER ITER
     return false;
   }
 
-  // Step 3.
+  // Steps 1-2 are assertions, step 3 is implicit.
+
+  // Step 4.
   //
   // Get the "return" method.
   if (!emitAtomOp(JSOp::CallProp, cx->names().return_)) {
@@ -2875,7 +2907,7 @@ bool BytecodeEmitter::emitIteratorCloseInScope(
     return false;
   }
 
-  // Step 4.
+  // Step 5.
   //
   // Do nothing if "return" is undefined or null.
   InternalIfEmitter ifReturnMethodIsDefined(this);
@@ -2889,126 +2921,39 @@ bool BytecodeEmitter::emitIteratorCloseInScope(
     return false;
   }
 
-  if (completionKind == CompletionKind::Throw) {
+  // Steps 5.c, 7.
-    // 7.4.6 IteratorClose ( iterator, completion )
-    //   ...
-    //   3. Let return be ? GetMethod(iterator, "return").
-    //   4. If return is undefined, return Completion(completion).
-    //   5. Let innerResult be Call(return, iterator, « »).
-    //   6. If completion.[[Type]] is throw, return Completion(completion).
-    //   7. If innerResult.[[Type]] is throw, return
-    //      Completion(innerResult).
-    //
-    // For CompletionKind::Normal case, JSOp::Call for step 5 checks if RET
-    // is callable, and throws if not.  Since step 6 doesn't match and
-    // error handling in step 3 and step 7 can be merged.
-    //
-    // For CompletionKind::Throw case, an error thrown by JSOp::Call for
-    // step 5 is ignored by try-catch.  So we should check if RET is
-    // callable here, outside of try-catch, and the throw immediately if
-    // not.
-    CheckIsCallableKind kind = CheckIsCallableKind::IteratorReturn;
-    if (!emitCheckIsCallable(kind)) {
-      //            [stack] ... ITER RET
-      return false;
-    }
-  }
-
-  // Steps 5, 8.
   //
-  // Call "return" if it is not undefined or null, and check that it returns
+  // Call the "return" method.
-  // an Object.
   if (!emit1(JSOp::Swap)) {
     //              [stack] ... RET ITER
     return false;
   }
 
-  Maybe<TryEmitter> tryCatch;
-
-  if (completionKind == CompletionKind::Throw) {
-    tryCatch.emplace(this, TryEmitter::Kind::TryCatch,
-                     TryEmitter::ControlKind::NonSyntactic);
-
-    // Mutate stack to balance stack for try-catch.
-    if (!emit1(JSOp::Undefined)) {
-      //            [stack] ... RET ITER UNDEF
-      return false;
-    }
-    if (!tryCatch->emitTry()) {
-      //            [stack] ... RET ITER UNDEF
-      return false;
-    }
-    if (!emitDupAt(2, 2)) {
-      //            [stack] ... RET ITER UNDEF RET
-      return false;
-    }
-  }
-
   if (!emitCall(JSOp::Call, 0)) {
-    //              [stack] ... ... RESULT
+    //              [stack] ... RESULT
     return false;
   }
 
+  // 7.4.7 AsyncIteratorClose, step 5.d.
   if (iterKind == IteratorKind::Async) {
     if (completionKind != CompletionKind::Throw) {
       // Await clobbers rval, so save the current rval.
       if (!emit1(JSOp::GetRval)) {
-        //          [stack] ... ... RESULT RVAL
+        //          [stack] ... RESULT RVAL
         return false;
       }
       if (!emit1(JSOp::Swap)) {
-        //          [stack] ... ... RVAL RESULT
+        //          [stack] ... RVAL RESULT
         return false;
       }
     }
+
     if (!emitAwaitInScope(currentScope)) {
-      //            [stack] ... ... RVAL? RESULT
-      return false;
-    }
-  }
-
-  if (completionKind == CompletionKind::Throw) {
-    if (!emit1(JSOp::Swap)) {
-      //            [stack] ... RET ITER RESULT UNDEF
-      return false;
-    }
-    if (!emit1(JSOp::Pop)) {
-      //            [stack] ... RET ITER RESULT
-      return false;
-    }
-
-    if (!tryCatch->emitCatch()) {
-      //            [stack] ... RET ITER RESULT EXC
-      return false;
-    }
-
-    // Just ignore the exception thrown by call and await.
-    if (!emit1(JSOp::Pop)) {
-      //            [stack] ... RET ITER RESULT
-      return false;
-    }
-
-    if (!tryCatch->emitEnd()) {
-      //            [stack] ... RET ITER RESULT
-      return false;
-    }
-
-    // Restore stack.
-    if (!emit2(JSOp::Unpick, 2)) {
-      //            [stack] ... RESULT RET ITER
-      return false;
-    }
-    if (!emitPopN(2)) {
-      //            [stack] ... RESULT
-      return false;
-    }
-  } else {
-    if (!emitCheckIsObj(CheckIsObjectKind::IteratorReturn)) {
       //            [stack] ... RVAL? RESULT
       return false;
     }
 
-    if (iterKind == IteratorKind::Async) {
+    if (completionKind != CompletionKind::Throw) {
       if (!emit1(JSOp::Swap)) {
         //          [stack] ... RESULT RVAL
         return false;
@@ -3020,6 +2965,17 @@ bool BytecodeEmitter::emitIteratorCloseInScope(
     }
   }
 
+  // Step 6 (Handled in caller).
+
+  // Step 8.
+  if (completionKind != CompletionKind::Throw) {
+    // Check that the "return" result is an object.
+    if (!emitCheckIsObj(CheckIsObjectKind::IteratorReturn)) {
+      //            [stack] ... RESULT
+      return false;
+    }
+  }
+
   if (!ifReturnMethodIsDefined.emitElse()) {
     //              [stack] ... ITER RET
     return false;
@@ -3034,6 +2990,26 @@ bool BytecodeEmitter::emitIteratorCloseInScope(
     return false;
   }
 
+  if (completionKind == CompletionKind::Throw) {
+    if (!tryCatch->emitCatch()) {
+      //            [stack] ... ITER EXC
+      return false;
+    }
+
+    // Just ignore the exception thrown by call and await.
+    if (!emit1(JSOp::Pop)) {
+      //            [stack] ... ITER
+      return false;
+    }
+
+    if (!tryCatch->emitEnd()) {
+      //            [stack] ... ITER
+      return false;
+    }
+  }
+
+  // Step 9 (Handled in caller).
+
   return emit1(JSOp::Pop);
   //                [stack] ...
 }

js/src/tests/jstests.list

@@ -456,13 +456,6 @@ skip script test262/built-ins/FinalizationRegistry/prototype/cleanupSome/cleanup
 skip script test262/built-ins/FinalizationRegistry/prototype/cleanupSome/reentrancy.js
 skip script test262/built-ins/FinalizationRegistry/prototype/unregister/unregister-cleaned-up-cell.js
 
-# https://github.com/tc39/ecma262/pull/1408
-# https://bugzilla.mozilla.org/show_bug.cgi?id=1629795
-skip script test262/language/statements/for-await-of/iterator-close-throw-get-method-non-callable.js
-skip script test262/language/statements/for-await-of/iterator-close-throw-get-method-abrupt.js
-skip script test262/language/statements/for-of/iterator-close-throw-get-method-non-callable.js
-skip script test262/language/statements/for-of/iterator-close-throw-get-method-abrupt.js
-
 
 ###########################################################
 # Tests disabled due to issues in test262 importer script #

js/src/tests/non262/Array/from-iterator-close.js

@@ -81,8 +81,8 @@ test(MyArray, {
     closed: true,
 });
 
-// ES 2017 draft 7.4.6 step 3.
+// ES 2021 draft 7.4.6 step 5.
-// if GetMethod fails, the thrown value should be used.
+// if GetMethod fails, the thrown value should be ignored.
 test(MyArray, {
     nextVal: { value: 1, done: false },
     modifier: (iterator, iterable) => {
@@ -93,7 +93,7 @@ test(MyArray, {
             }
         });
     },
-    exceptionVal: "return getter throws",
+    exceptionVal: "defineProperty throws",
     closed: true,
 });
 test(MyArray, {
@@ -106,7 +106,7 @@ test(MyArray, {
             }
         });
     },
-    exceptionType: TypeError,
+    exceptionVal: "defineProperty throws",
     closed: true,
 });
 test(MyArray, {
@@ -120,7 +120,7 @@ test(MyArray, {
             }
         });
     },
-    exceptionType: TypeError,
+    exceptionVal: "defineProperty throws",
     closed: true,
 });
 

js/src/tests/non262/Map/constructor-iterator-close.js

@@ -120,9 +120,9 @@ test([MySet, MyWeakSet], {
     closed: true,
 });
 
-// ES 2017 draft 7.4.6 step 3.
+// ES 2021 draft 7.4.6 step 5.
-// if GetMethod fails, the thrown value should be used.
+// if GetMethod fails, the thrown value should be ignored.
-test([MyMap, MySet, MyWeakMap, MyWeakSet], {
+test([MyMap, MyWeakMap], {
     nextVal: { value: [{}, {}], done: false },
     modifier: (iterator, iterable) => {
         Object.defineProperty(iterator, "return", {
@@ -132,10 +132,23 @@ test([MyMap, MySet, MyWeakMap, MyWeakSet], {
             }
         });
     },
-    exceptionVal: "return getter throws",
+    exceptionVal: "setter throws",
     closed: true,
 });
-test([MyMap, MySet, MyWeakMap, MyWeakSet], {
+test([MySet, MyWeakSet], {
+  nextVal: { value: [{}, {}], done: false },
+  modifier: (iterator, iterable) => {
+      Object.defineProperty(iterator, "return", {
+          get: function() {
+              iterable.closed = true;
+              throw "return getter throws";
+          }
+      });
+  },
+  exceptionVal: "adder throws",
+  closed: true,
+});
+test([MyMap, MyWeakMap], {
     nextVal: { value: [{}, {}], done: false },
     modifier: (iterator, iterable) => {
         Object.defineProperty(iterator, "return", {
@@ -145,10 +158,23 @@ test([MyMap, MySet, MyWeakMap, MyWeakSet], {
             }
         });
     },
-    exceptionType: TypeError,
+    exceptionVal: "setter throws",
     closed: true,
 });
-test([MyMap, MySet, MyWeakMap, MyWeakSet], {
+test([MySet, MyWeakSet], {
+  nextVal: { value: [{}, {}], done: false },
+  modifier: (iterator, iterable) => {
+      Object.defineProperty(iterator, "return", {
+          get: function() {
+              iterable.closed = true;
+              return "non object";
+          }
+      });
+  },
+  exceptionVal: "adder throws",
+  closed: true,
+});
+test([MyMap, MyWeakMap], {
     nextVal: { value: [{}, {}], done: false },
     modifier: (iterator, iterable) => {
         Object.defineProperty(iterator, "return", {
@@ -159,9 +185,23 @@ test([MyMap, MySet, MyWeakMap, MyWeakSet], {
             }
         });
     },
-    exceptionType: TypeError,
+    exceptionVal: "setter throws",
     closed: true,
 });
+test([MySet, MyWeakSet], {
+  nextVal: { value: [{}, {}], done: false },
+  modifier: (iterator, iterable) => {
+      Object.defineProperty(iterator, "return", {
+          get: function() {
+              iterable.closed = true;
+              // Non callable.
+              return {};
+          }
+      });
+  },
+  exceptionVal: "adder throws",
+  closed: true,
+});
 
 // ES 2017 draft 7.4.6 steps 6.
 // if return method throws, the thrown value should be ignored.