[ignition] Fix iteration finalization exception suppression

The IteratorClose spec specifies that exceptions in
%GetMethod(iterator.return) are not suppressed by exceptions in the
given continuation (body of a loop, assignments in destructuring),
while exceptions in the execution of iterator.return() are.

This means that we have to split out the property access + a typeof
check to be outside the try-catch, and keep the call inside of it.

The non-split version is only for cases when there is no 'throws'
continuation (as is the case for yield* calling IteratorClose), so
the existing BuildIteratorClose can be renamed to reflect this.

Change-Id: Id71aea4fddd6ffb986bd9aaa09d29615a8800f71
Reviewed-on: https://chromium-review.googlesource.com/c/1402789Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58694}

src/interpreter/bytecode-generator.cc

@@ -3083,58 +3083,120 @@ BytecodeGenerator::AssignmentLhsData BytecodeGenerator::PrepareAssignmentLhs(
 //
 // In pseudo-code, this builds:
 //
-// try {
-//   if (!done) iterator.close()
-// } catch (e) {
-//   if (iteration_continuation != RETHROW)
-//     rethrow e
+// if (!done) {
+//   let method = iterator.return
+//   if (method !== null && method !== undefined) {
+//     if (typeof(method) !== "function") throw TypeError
+//     try {
+//       let return_val = method.call(iterator)
+//       if (!%IsObject(return_val)) throw TypeError
+//     } catch (e) {
+//       if (iteration_continuation != RETHROW)
+//         rethrow e
+//     }
+//   }
 // }
 void BytecodeGenerator::BuildFinalizeIteration(
     IteratorRecord iterator, Register done,
     Register iteration_continuation_token) {
-  RegisterAllocationScope scope(this);
-  TryCatchBuilder try_control_builder(builder(), nullptr, nullptr,
-                                      HandlerTable::DESUGARING);
+  RegisterAllocationScope register_scope(this);
+  BytecodeLabels iterator_is_done(zone());
 
-  // Preserve the context in a dedicated register, so that it can be restored
-  // when the handler is entered by the stack-unwinding machinery.
-  // TODO(mstarzinger): Be smarter about register allocation.
-  Register context = register_allocator()->NewRegister();
-  builder()->MoveRegister(Register::current_context(), context);
+  // if (!done) {
+  builder()->LoadAccumulatorWithRegister(done).JumpIfTrue(
+      ToBooleanMode::kConvertToBoolean, iterator_is_done.New());
 
-  // Evaluate the try-block inside a control scope. This simulates a handler
-  // that is intercepting 'throw' control commands.
-  try_control_builder.BeginTry(context);
-  {
-    ControlScopeForTryCatch scope(this, &try_control_builder);
+  //   method = iterator.return
+  //   if (method !== null && method !== undefined) {
+  Register method = register_allocator()->NewRegister();
+  builder()
+      ->LoadNamedProperty(iterator.object(),
+                          ast_string_constants()->return_string(),
+                          feedback_index(feedback_spec()->AddLoadICSlot()))
+      .StoreAccumulatorInRegister(method)
+      .JumpIfUndefined(iterator_is_done.New())
+      .JumpIfNull(iterator_is_done.New());
 
-    // if (!done) iterator.close()
-    BytecodeLabel iterator_is_done;
-    builder()->LoadAccumulatorWithRegister(done).JumpIfTrue(
-        ToBooleanMode::kConvertToBoolean, &iterator_is_done);
-    BuildIteratorClose(iterator);
-    builder()->Bind(&iterator_is_done);
+  //     if (typeof(method) !== "function") throw TypeError
+  BytecodeLabel if_callable;
+  builder()
+      ->CompareTypeOf(TestTypeOfFlags::LiteralFlag::kFunction)
+      .JumpIfTrue(ToBooleanMode::kAlreadyBoolean, &if_callable);
+  {
+    // throw %NewTypeError(kReturnMethodNotCallable)
+    RegisterAllocationScope register_scope(this);
+    RegisterList new_type_error_args = register_allocator()->NewRegisterList(2);
+    builder()
+        ->LoadLiteral(Smi::FromEnum(MessageTemplate::kReturnMethodNotCallable))
+        .StoreAccumulatorInRegister(new_type_error_args[0])
+        .LoadLiteral(ast_string_constants()->empty_string())
+        .StoreAccumulatorInRegister(new_type_error_args[1])
+        .CallRuntime(Runtime::kNewTypeError, new_type_error_args)
+        .Throw();
   }
-  try_control_builder.EndTry();
+  builder()->Bind(&if_callable);
 
-  // catch (e) {
-  //   if (iteration_continuation != RETHROW)
-  //     rethrow e
-  // }
+  //     try {
+  //       let return_val = method.call(iterator)
+  //       if (!%IsObject(return_val)) throw TypeError
+  //     }
 
-  // Reuse context register to store the exception.
-  Register close_exception = context;
-  builder()->StoreAccumulatorInRegister(close_exception);
+  {
+    RegisterAllocationScope register_scope(this);
+    TryCatchBuilder try_control_builder(builder(), nullptr, nullptr,
+                                        HandlerTable::UNCAUGHT);
+
+    // Preserve the context in a dedicated register, so that it can be restored
+    // when the handler is entered by the stack-unwinding machinery.
+    // TODO(mstarzinger): Be smarter about register allocation.
+    Register context = register_allocator()->NewRegister();
+    builder()->MoveRegister(Register::current_context(), context);
+
+    // Evaluate the try-block inside a control scope. This simulates a handler
+    // that is intercepting 'throw' control commands.
+    try_control_builder.BeginTry(context);
+    {
+      ControlScopeForTryCatch scope(this, &try_control_builder);
 
-  BytecodeLabel suppress_close_exception;
-  builder()
-      ->LoadLiteral(Smi::FromInt(ControlScope::DeferredCommands::kRethrowToken))
-      .CompareReference(iteration_continuation_token)
-      .JumpIfTrue(ToBooleanMode::kAlreadyBoolean, &suppress_close_exception)
-      .LoadAccumulatorWithRegister(close_exception)
-      .ReThrow()
-      .Bind(&suppress_close_exception);
-  try_control_builder.EndCatch();
+      RegisterList args(iterator.object());
+      builder()->CallProperty(method, args,
+                              feedback_index(feedback_spec()->AddCallICSlot()));
+      builder()->JumpIfJSReceiver(iterator_is_done.New());
+      {
+        // Throw this exception inside the try block so that it is suppressed by
+        // the iteration continuation if necessary.
+        RegisterAllocationScope register_scope(this);
+        Register return_result = register_allocator()->NewRegister();
+        builder()
+            ->StoreAccumulatorInRegister(return_result)
+            .CallRuntime(Runtime::kThrowIteratorResultNotAnObject,
+                         return_result);
+      }
+    }
+    try_control_builder.EndTry();
+
+    //     catch (e) {
+    //       if (iteration_continuation != RETHROW)
+    //         rethrow e
+    //     }
+
+    // Reuse context register to store the exception.
+    Register close_exception = context;
+    builder()->StoreAccumulatorInRegister(close_exception);
+
+    BytecodeLabel suppress_close_exception;
+    builder()
+        ->LoadLiteral(
+            Smi::FromInt(ControlScope::DeferredCommands::kRethrowToken))
+        .CompareReference(iteration_continuation_token)
+        .JumpIfTrue(ToBooleanMode::kAlreadyBoolean, &suppress_close_exception)
+        .LoadAccumulatorWithRegister(close_exception)
+        .ReThrow()
+        .Bind(&suppress_close_exception);
+    try_control_builder.EndCatch();
+  }
+
+  iterator_is_done.Bind(builder());
 }
 
 // Get the default value of a destructuring target. Will mutate the
@@ -3212,7 +3274,7 @@ void BytecodeGenerator::BuildDestructuringArrayAssignment(
   builder()->LoadFalse();
   builder()->StoreAccumulatorInRegister(done);
   TryFinallyBuilder try_control_builder(builder(), nullptr, nullptr,
-                                        HandlerTable::DESUGARING);
+                                        HandlerTable::UNCAUGHT);
 
   // Keep a continuation token and result for exceptions and returns in the
   // destructuring body, so that we can close the iterator on abrupt

test/cctest/interpreter/bytecode_expectations/DestructuringAssignment.golden

@@ -10,9 +10,9 @@ snippet: "
   var x, a = [0,1,2,3];
   [x] = a;
 "
-frame size: 14
+frame size: 15
 parameter count: 1
-bytecode array length: 160
+bytecode array length: 178
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   45 S> */ B(CreateArrayLiteral), U8(0), U8(0), U8(37),
@@ -58,24 +58,31 @@ bytecodes: [
                 B(LdaTheHole),
                 B(SetPendingMessage),
                 B(Star), R(10),
-                B(Mov), R(context), R(12),
                 B(Ldar), R(7),
-                B(JumpIfToBooleanTrue), U8(27),
+                B(JumpIfToBooleanTrue), U8(60),
                 B(LdaNamedProperty), R(4), U8(5), U8(13),
-                B(JumpIfUndefined), U8(21),
-                B(JumpIfNull), U8(19),
-                B(Star), R(13),
-                B(CallProperty0), R(13), R(4), U8(15),
-                B(Jump), U8(2),
-                B(JumpIfJSReceiver), U8(9),
+                B(Star), R(12),
+                B(JumpIfUndefined), U8(52),
+                B(JumpIfNull), U8(50),
+                B(TestTypeOf), U8(6),
+                B(JumpIfTrue), U8(18),
+                B(Wide), B(LdaSmi), I16(154),
                 B(Star), R(13),
-                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(13), U8(1),
+                B(LdaConstant), U8(6),
+                B(Star), R(14),
+                B(CallRuntime), U16(Runtime::kNewTypeError), R(13), U8(2),
+                B(Throw),
+                B(Mov), R(context), R(13),
+                B(CallProperty0), R(12), R(4), U8(15),
+                B(JumpIfJSReceiver), U8(21),
+                B(Star), R(14),
+                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(14), U8(1),
                 B(Jump), U8(12),
-                B(Star), R(12),
+                B(Star), R(13),
                 B(LdaZero),
                 B(TestReferenceEqual), R(8),
                 B(JumpIfTrue), U8(5),
-                B(Ldar), R(12),
+                B(Ldar), R(13),
                 B(ReThrow),
                 B(Ldar), R(10),
                 B(SetPendingMessage),
@@ -94,10 +101,11 @@ constant pool: [
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["done"],
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["value"],
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["return"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE [""],
 ]
 handlers: [
   [44, 86, 94],
-  [106, 135, 137],
+  [140, 153, 155],
 ]
 
 ---
@@ -105,9 +113,9 @@ snippet: "
   var x, y, a = [0,1,2,3];
   [,x,...y] = a;
 "
-frame size: 15
+frame size: 16
 parameter count: 1
-bytecode array length: 258
+bytecode array length: 276
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   48 S> */ B(CreateArrayLiteral), U8(0), U8(0), U8(37),
@@ -194,24 +202,31 @@ bytecodes: [
                 B(LdaTheHole),
                 B(SetPendingMessage),
                 B(Star), R(11),
-                B(Mov), R(context), R(13),
                 B(Ldar), R(8),
-                B(JumpIfToBooleanTrue), U8(27),
+                B(JumpIfToBooleanTrue), U8(60),
                 B(LdaNamedProperty), R(5), U8(5), U8(23),
-                B(JumpIfUndefined), U8(21),
-                B(JumpIfNull), U8(19),
-                B(Star), R(14),
-                B(CallProperty0), R(14), R(5), U8(25),
-                B(Jump), U8(2),
-                B(JumpIfJSReceiver), U8(9),
+                B(Star), R(13),
+                B(JumpIfUndefined), U8(52),
+                B(JumpIfNull), U8(50),
+                B(TestTypeOf), U8(6),
+                B(JumpIfTrue), U8(18),
+                B(Wide), B(LdaSmi), I16(154),
                 B(Star), R(14),
-                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(14), U8(1),
+                B(LdaConstant), U8(6),
+                B(Star), R(15),
+                B(CallRuntime), U16(Runtime::kNewTypeError), R(14), U8(2),
+                B(Throw),
+                B(Mov), R(context), R(14),
+                B(CallProperty0), R(13), R(5), U8(25),
+                B(JumpIfJSReceiver), U8(21),
+                B(Star), R(15),
+                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(15), U8(1),
                 B(Jump), U8(12),
-                B(Star), R(13),
+                B(Star), R(14),
                 B(LdaZero),
                 B(TestReferenceEqual), R(9),
                 B(JumpIfTrue), U8(5),
-                B(Ldar), R(13),
+                B(Ldar), R(14),
                 B(ReThrow),
                 B(Ldar), R(11),
                 B(SetPendingMessage),
@@ -230,10 +245,11 @@ constant pool: [
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["done"],
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["value"],
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["return"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE [""],
 ]
 handlers: [
   [44, 184, 192],
-  [204, 233, 235],
+  [238, 251, 253],
 ]
 
 ---
@@ -241,9 +257,9 @@ snippet: "
   var x={}, y, a = [0];
   [x.foo,y=4] = a;
 "
-frame size: 16
+frame size: 17
 parameter count: 1
-bytecode array length: 211
+bytecode array length: 229
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   40 S> */ B(CreateEmptyObjectLiteral),
@@ -310,24 +326,31 @@ bytecodes: [
                 B(LdaTheHole),
                 B(SetPendingMessage),
                 B(Star), R(11),
-                B(Mov), R(context), R(14),
                 B(Ldar), R(8),
-                B(JumpIfToBooleanTrue), U8(27),
+                B(JumpIfToBooleanTrue), U8(60),
                 B(LdaNamedProperty), R(5), U8(6), U8(17),
-                B(JumpIfUndefined), U8(21),
-                B(JumpIfNull), U8(19),
-                B(Star), R(15),
-                B(CallProperty0), R(15), R(5), U8(19),
-                B(Jump), U8(2),
-                B(JumpIfJSReceiver), U8(9),
+                B(Star), R(14),
+                B(JumpIfUndefined), U8(52),
+                B(JumpIfNull), U8(50),
+                B(TestTypeOf), U8(6),
+                B(JumpIfTrue), U8(18),
+                B(Wide), B(LdaSmi), I16(154),
                 B(Star), R(15),
-                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(15), U8(1),
+                B(LdaConstant), U8(7),
+                B(Star), R(16),
+                B(CallRuntime), U16(Runtime::kNewTypeError), R(15), U8(2),
+                B(Throw),
+                B(Mov), R(context), R(15),
+                B(CallProperty0), R(14), R(5), U8(19),
+                B(JumpIfJSReceiver), U8(21),
+                B(Star), R(16),
+                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(16), U8(1),
                 B(Jump), U8(12),
-                B(Star), R(14),
+                B(Star), R(15),
                 B(LdaZero),
                 B(TestReferenceEqual), R(9),
                 B(JumpIfTrue), U8(5),
-                B(Ldar), R(14),
+                B(Ldar), R(15),
                 B(ReThrow),
                 B(Ldar), R(11),
                 B(SetPendingMessage),
@@ -347,10 +370,11 @@ constant pool: [
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["value"],
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["foo"],
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["return"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE [""],
 ]
 handlers: [
   [47, 137, 145],
-  [157, 186, 188],
+  [191, 204, 206],
 ]
 
 ---

test/mjsunit/es6/destructuring-assignment.js

@@ -574,3 +574,58 @@ assertEquals(oz, [1, 2, 3, 4, 5]);
   assertEquals(1, ext("let x; ({x} = { x: super() })").x);
   assertEquals(1, ext("let x, y; ({ x: y } = { x } = { x: super() })").x);
 })();
+
+(function testInvalidReturn() {
+  function* g() { yield 1; }
+
+  let executed_x_setter;
+  let executed_return;
+  var a = {
+    set x(val) {
+      executed_x_setter = true;
+      throw 3;
+    }
+  };
+
+  // The exception from the execution of g().return() should be suppressed by
+  // the setter error.
+  executed_x_setter = false;
+  executed_return = false;
+  g.prototype.return = function() {
+    executed_return = true;
+    throw 4;
+  };
+  assertThrowsEquals("[a.x] = g()", 3);
+  assertTrue(executed_x_setter);
+  assertTrue(executed_return);
+
+  // The exception from g().return() not returning an object should be
+  // suppressed by the setter error.
+  executed_x_setter = false;
+  executed_return = false;
+  g.prototype.return = function() {
+    assertTrue(executed_return);
+    return null;
+  };
+  assertThrowsEquals("[a.x] = g()", 3);
+  assertTrue(executed_x_setter);
+  assertTrue(executed_return);
+
+  // The TypeError from g().return not being a method should suppress the setter
+  // error.
+  executed_x_setter = false;
+  g.prototype.return = "not a method";
+  assertThrows("[a.x] = g()", TypeError);
+  assertTrue(executed_x_setter);
+
+  // The exception from the access of g().return should suppress the setter
+  // error.
+  executed_x_setter = false;
+  Object.setPrototypeOf(g.prototype, {
+    get return() {
+      throw 4;
+    }
+  });
+  assertThrowsEquals("[a.x] = g()", 4);
+  assertTrue(executed_x_setter);
+})