Fix priority of exceptions being thrown from for-of loops

In the for-of desugaring, IteratorClose is a subtle thing to get right.
When return exists, the logic for which exception to throw is as follows:
1. Get the 'return' property and property any exception that might come from
  the property read
2. Call return, not yet propagating an exception if it's thrown.
3. If we are closing the iterator due to an exception, propagate that error.
4. If return threw, propagate that error.
5. Check if return's return value was not an object, and throw if so

Previously, we were effectively doing step 5 even if an exception "had already
been thrown" by step 3. Because this took place in a finally block, the exception
"won the race" and was the one propagated to the user. The fix is a simple change
to the desugaring to do step 5 only if step 3 didn't happen.

R=rossberg
BUG=v8:4775
LOG=Y

Review URL: https://codereview.chromium.org/1728973002

Cr-Commit-Position: refs/heads/master@{#34261}

src/parsing/parser.cc

@@ -6577,16 +6577,17 @@ void ParserTraits::BuildIteratorCloseForCompletion(
   //
   //   let iteratorReturn = iterator.return;
   //   if (!IS_NULL_OR_UNDEFINED(iteratorReturn)) {
-  //     let output;
   //     if (completion === BODY_THREW) {
   //       if (!IS_CALLABLE(iteratorReturn)) {
   //         throw MakeTypeError(kReturnMethodNotCallable);
   //       }
-  //       try { output = %_Call(iteratorReturn, iterator) } catch (_) { }
+  //       try { %_Call(iteratorReturn, iterator) } catch (_) { }
   //     } else {
-  //       output = %_Call(iteratorReturn, iterator);
+  //       let output = %_Call(iteratorReturn, iterator);
+  //       if (!IS_RECEIVER(output)) {
+  //         %ThrowIterResultNotAnObject(output);
+  //       }
   //     }
-  //     if (!IS_RECEIVER(output)) %ThrowIterResultNotAnObject(output);
   //   }
   //
 
@@ -6596,11 +6597,9 @@ void ParserTraits::BuildIteratorCloseForCompletion(
   auto scope = parser_->scope_;
   auto zone = parser_->zone();
 
-  // let output;
-  Variable* var_output = scope->NewTemporary(avfactory->empty_string());
 
   // let iteratorReturn = iterator.return;
-  Variable* var_return = var_output;  // Reusing the output variable.
+  Variable* var_return = scope->NewTemporary(avfactory->empty_string());
   Statement* get_return;
   {
     Expression* iterator_proxy = factory->NewVariableProxy(iterator);
@@ -6625,22 +6624,7 @@ void ParserTraits::BuildIteratorCloseForCompletion(
     check_return_callable = CheckCallable(var_return, throw_expr);
   }
 
-  // output = %_Call(iteratorReturn, iterator);
-  Statement* call_return;
-  {
-    auto args = new (zone) ZoneList<Expression*>(2, zone);
-    args->Add(factory->NewVariableProxy(var_return), zone);
-    args->Add(factory->NewVariableProxy(iterator), zone);
-    Expression* call =
-        factory->NewCallRuntime(Runtime::kInlineCall, args, nopos);
-
-    Expression* output_proxy = factory->NewVariableProxy(var_output);
-    Expression* assignment = factory->NewAssignment(
-        Token::ASSIGN, output_proxy, call, nopos);
-    call_return = factory->NewExpressionStatement(assignment, nopos);
-  }
-
-  // try { output = %_Call(iteratorReturn, iterator) } catch (_) { }
+  // try { %_Call(iteratorReturn, iterator) } catch (_) { }
   Statement* try_call_return;
   {
     auto args = new (zone) ZoneList<Expression*>(2, zone);
@@ -6649,12 +6633,10 @@ void ParserTraits::BuildIteratorCloseForCompletion(
 
     Expression* call =
         factory->NewCallRuntime(Runtime::kInlineCall, args, nopos);
-    Expression* assignment = factory->NewAssignment(
-        Token::ASSIGN, factory->NewVariableProxy(var_output), call, nopos);
 
     Block* try_block = factory->NewBlock(nullptr, 1, false, nopos);
-    try_block->statements()->Add(
-        factory->NewExpressionStatement(assignment, nopos), zone);
+    try_block->statements()->Add(factory->NewExpressionStatement(call, nopos),
+                                 zone);
 
     Block* catch_block = factory->NewBlock(nullptr, 0, false, nopos);
 
@@ -6667,29 +6649,27 @@ void ParserTraits::BuildIteratorCloseForCompletion(
         try_block, catch_scope, catch_variable, catch_block, nopos);
   }
 
-  // if (completion === ABRUPT_THROW) {
-  //   #check_return_callable;
-  //   #try_call_return;
-  // } else {
-  //   #call_return;
+  // let output = %_Call(iteratorReturn, iterator);
+  // if (!IS_RECEIVER(output)) {
+  //   %ThrowIteratorResultNotAnObject(output);
   // }
-  Statement* call_return_carefully;
+  Block* validate_return;
   {
-    Expression* condition = factory->NewCompareOperation(
-        Token::EQ_STRICT, factory->NewVariableProxy(completion),
-        factory->NewSmiLiteral(BODY_THREW, nopos), nopos);
-
-    Block* then_block = factory->NewBlock(nullptr, 2, false, nopos);
-    then_block->statements()->Add(check_return_callable, zone);
-    then_block->statements()->Add(try_call_return, zone);
+    Variable* var_output = scope->NewTemporary(avfactory->empty_string());
+    Statement* call_return;
+    {
+      auto args = new (zone) ZoneList<Expression*>(2, zone);
+      args->Add(factory->NewVariableProxy(var_return), zone);
+      args->Add(factory->NewVariableProxy(iterator), zone);
+      Expression* call =
+          factory->NewCallRuntime(Runtime::kInlineCall, args, nopos);
 
-    call_return_carefully =
-        factory->NewIfStatement(condition, then_block, call_return, nopos);
-  }
+      Expression* output_proxy = factory->NewVariableProxy(var_output);
+      Expression* assignment =
+          factory->NewAssignment(Token::ASSIGN, output_proxy, call, nopos);
+      call_return = factory->NewExpressionStatement(assignment, nopos);
+    }
 
-  // if (!IS_RECEIVER(output)) %ThrowIteratorResultNotAnObject(output);
-  Statement* validate_output;
-  {
     Expression* is_receiver_call;
     {
       auto args = new (zone) ZoneList<Expression*>(1, zone);
@@ -6707,8 +6687,32 @@ void ParserTraits::BuildIteratorCloseForCompletion(
       throw_call = factory->NewExpressionStatement(call, nopos);
     }
 
-    validate_output = factory->NewIfStatement(
+    Statement* check_return = factory->NewIfStatement(
         is_receiver_call, factory->NewEmptyStatement(nopos), throw_call, nopos);
+
+    validate_return = factory->NewBlock(nullptr, 2, false, nopos);
+    validate_return->statements()->Add(call_return, zone);
+    validate_return->statements()->Add(check_return, zone);
+  }
+
+  // if (completion === BODY_THREW) {
+  //   #check_return_callable;
+  //   #try_call_return;
+  // } else {
+  //   #validate_return;
+  // }
+  Statement* call_return_carefully;
+  {
+    Expression* condition = factory->NewCompareOperation(
+        Token::EQ_STRICT, factory->NewVariableProxy(completion),
+        factory->NewSmiLiteral(BODY_THREW, nopos), nopos);
+
+    Block* then_block = factory->NewBlock(nullptr, 2, false, nopos);
+    then_block->statements()->Add(check_return_callable, zone);
+    then_block->statements()->Add(try_call_return, zone);
+
+    call_return_carefully =
+        factory->NewIfStatement(condition, then_block, validate_return, nopos);
   }
 
   // if (!IS_NULL_OR_UNDEFINED(iteratorReturn)) { ... }
@@ -6718,12 +6722,9 @@ void ParserTraits::BuildIteratorCloseForCompletion(
         Token::EQ, factory->NewVariableProxy(var_return),
         factory->NewNullLiteral(nopos), nopos);
 
-    Block* block = factory->NewBlock(nullptr, 2, false, nopos);
-    block->statements()->Add(call_return_carefully, zone);
-    block->statements()->Add(validate_output, zone);
-
-    maybe_call_return = factory->NewIfStatement(
-        condition, factory->NewEmptyStatement(nopos), block, nopos);
+    maybe_call_return =
+        factory->NewIfStatement(condition, factory->NewEmptyStatement(nopos),
+                                call_return_carefully, nopos);
   }
 
 
@@ -6746,7 +6747,7 @@ Statement* ParserTraits::FinalizeForOfStatement(ForOfStatement* loop, int pos) {
   //     throw e;
   //   } finally {
   //     if (!(completion === BODY_COMPLETED || IS_UNDEFINED(#iterator))) {
-  //       #BuildIteratorClose(#iterator, completion)  // See above.
+  //       #BuildIteratorCloseForCompletion(#iterator, completion)
   //     }
   //   }
   //
@@ -6793,7 +6794,7 @@ Statement* ParserTraits::FinalizeForOfStatement(ForOfStatement* loop, int pos) {
   }
 
   // if (!(completion === BODY_COMPLETED || IS_UNDEFINED(#iterator))) {
-  //   #BuildIteratorClose(#iterator, completion)
+  //   #BuildIteratorCloseForCompletion(#iterator, completion)
   // }
   Block* maybe_close;
   {

test/mjsunit/harmony/iterator-close.js

@@ -87,13 +87,15 @@ function* g() { yield 42; return 88 };
     for (x of g()) { break; }
   }, TypeError);
 
-  assertThrows(() => {
+  // Throw from the body of a for loop 'wins' vs throw
+  // originating from a bad 'return' value.
+  assertThrowsEquals(() => {
     for (let x of g()) { throw 666; }
-  }, TypeError);
+  }, 666);
 
-  assertThrows(() => {
+  assertThrowsEquals(() => {
     for (x of g()) { throw 666; }
-  }, TypeError);
+  }, 666);
 
   assertThrows(() => {
     for (let x of g()) { return 666; }

test/test262/test262.status

@@ -55,18 +55,6 @@
   # https://code.google.com/p/v8/issues/detail?id=4163
   'built-ins/GeneratorPrototype/next/context-constructor-invocation': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=4775
-  'built-ins/Array/from/iter-map-fn-err': [FAIL],
-  'built-ins/Map/iterator-close-after-set-failure': [FAIL],
-  'built-ins/Map/iterator-item-first-entry-returns-abrupt': [FAIL],
-  'built-ins/Map/iterator-item-second-entry-returns-abrupt': [FAIL],
-  'built-ins/Promise/race/iter-close': [PASS, FAIL],
-  'built-ins/Set/set-iterator-close-after-add-failure': [FAIL],
-  'built-ins/WeakMap/iterator-close-after-set-failure': [FAIL],
-  'built-ins/WeakMap/iterator-item-first-entry-returns-abrupt': [FAIL],
-  'built-ins/WeakMap/iterator-item-second-entry-returns-abrupt': [FAIL],
-  'built-ins/WeakSet/iterator-close-after-add-failure': [FAIL],
-
   # https://code.google.com/p/v8/issues/detail?id=4348
   'built-ins/String/prototype/Symbol.iterator/this-val-non-obj-coercible': [FAIL],