Reland of Fix async/await memory leak (patchset #1 id:1 of https://co…

…dereview.chromium.org/2348403003/ )

This patch closes a memory leak in async/await where the desugaring
was creating a situation analagous to that described in v8:5002.
Intermediate Promises were being kept alive, so a long-running loop
would cause linear memory usage on the heap. This patch returns
undefined to the 'then' callback passed into PerformPromiseThen
in order to avoid this hazard. Test expectations are fixed to remove
expecting extraneous events which occurred on Promises that are
now not given unnecessarily complex resolution paths before being
thrown away.

This patch is a reland; originally, tests which exercised the memory
exhaustion were checked in. Although it's possible to find good parameters
for running such tests locally, it is difficult to automate the tests
between the rock of timeouts and the hard place of too-small heaps
causing memory exhaustion in some modes even when there is no leak.

BUG=v8:5390

Review-Url: https://codereview.chromium.org/2352933002
Cr-Commit-Position: refs/heads/master@{#39520}

src/js/harmony-async-await.js

@@ -66,10 +66,22 @@ function AsyncFunctionAwait(generator, awaited, mark) {
   // );
   var promise = PromiseCastResolved(awaited);
 
-  var onFulfilled =
-      (sentValue) => %_Call(AsyncFunctionNext, generator, sentValue);
-  var onRejected =
-      (sentError) => %_Call(AsyncFunctionThrow, generator, sentError);
+  var onFulfilled = sentValue => {
+    %_Call(AsyncFunctionNext, generator, sentValue);
+    // The resulting Promise is a throwaway, so it doesn't matter what it
+    // resolves to. What is important is that we don't end up keeping the
+    // whole chain of intermediate Promises alive by returning the value
+    // of AsyncFunctionNext, as that would create a memory leak.
+    return;
+  };
+  var onRejected = sentError => {
+    %_Call(AsyncFunctionThrow, generator, sentError);
+    // Similarly, returning the huge Promise here would cause a long
+    // resolution chain to find what the exception to throw is, and
+    // create a similar memory leak, and it does not matter what
+    // sort of rejection this intermediate Promise becomes.
+    return;
+  }
 
   if (mark && DEBUG_IS_ACTIVE && IsPromise(awaited)) {
     // Mark the reject handler callback such that it does not influence

src/parsing/parser.cc

@@ -4561,21 +4561,46 @@ Expression* Parser::ExpressionListToExpression(ZoneList<Expression*>* args) {
 
 Expression* Parser::RewriteAwaitExpression(Expression* value, int await_pos) {
   // yield do {
+  //   promise_tmp = .promise;
   //   tmp = <operand>;
-  //   tmp = %AsyncFunctionAwait(.generator_object, tmp);
+  //   %AsyncFunctionAwait(.generator_object, tmp);
+  //   promise_tmp
   // }
+  // The value of the expression is returned to the caller of the async
+  // function for the first yield statement; for this, .promise is the
+  // appropriate return value, being a Promise that will be fulfilled or
+  // rejected with the appropriate value by the desugaring. Subsequent yield
+  // occurrences will return to the AsyncFunctionNext call within the
+  // implemementation of the intermediate throwaway Promise's then handler.
+  // This handler has nothing useful to do with the value, as the Promise is
+  // ignored. If we yielded the value of the throwawayPromise that
+  // AsyncFunctionAwait creates as an intermediate, it would create a memory
+  // leak; we must return .promise instead;
+  // The operand needs to be evaluated on a separate statement in order to get
+  // a break location, and the .promise needs to be read earlier so that it
+  // doesn't insert a false location.
+  // TODO(littledan): investigate why this ordering is needed in more detail.
   Variable* generator_object_variable =
       function_state_->generator_object_variable();
 
   // If generator_object_variable is null,
+  // TODO(littledan): Is this necessary?
   if (!generator_object_variable) return value;
 
   const int nopos = kNoSourcePosition;
 
-  Variable* temp_var = NewTemporary(ast_value_factory()->empty_string());
-  Block* do_block = factory()->NewBlock(nullptr, 2, false, nopos);
+  Block* do_block = factory()->NewBlock(nullptr, 3, false, nopos);
+
+  Variable* promise_temp_var =
+      NewTemporary(ast_value_factory()->empty_string());
+  Expression* promise_assignment = factory()->NewAssignment(
+      Token::ASSIGN, factory()->NewVariableProxy(promise_temp_var),
+      BuildDotPromise(), nopos);
+  do_block->statements()->Add(
+      factory()->NewExpressionStatement(promise_assignment, nopos), zone());
 
   // Wrap value evaluation to provide a break location.
+  Variable* temp_var = NewTemporary(ast_value_factory()->empty_string());
   Expression* value_assignment = factory()->NewAssignment(
       Token::ASSIGN, factory()->NewVariableProxy(temp_var), value, nopos);
   do_block->statements()->Add(
@@ -4595,14 +4620,13 @@ Expression* Parser::RewriteAwaitExpression(Expression* value, int await_pos) {
   Expression* async_function_await =
       factory()->NewCallRuntime(Context::ASYNC_FUNCTION_AWAIT_CAUGHT_INDEX,
                                 async_function_await_args, nopos);
+  do_block->statements()->Add(
+      factory()->NewExpressionStatement(async_function_await, await_pos),
+      zone());
 
   // Wrap await to provide a break location between value evaluation and yield.
-  Expression* await_assignment = factory()->NewAssignment(
-      Token::ASSIGN, factory()->NewVariableProxy(temp_var),
-      async_function_await, nopos);
-  do_block->statements()->Add(
-      factory()->NewExpressionStatement(await_assignment, await_pos), zone());
-  Expression* do_expr = factory()->NewDoExpression(do_block, temp_var, nopos);
+  Expression* do_expr =
+      factory()->NewDoExpression(do_block, promise_temp_var, nopos);
 
   generator_object = factory()->NewVariableProxy(generator_object_variable);
   return factory()->NewYield(generator_object, do_expr, nopos,

test/mjsunit/harmony/debug-async-function-async-task-event.js

@@ -4,25 +4,26 @@
 
 // Flags: --harmony-async-await --expose-debug-as debug --allow-natives-syntax
 
+// The test observes the callbacks that async/await makes to the inspector
+// to make accurate stack traces. The limited number of events is an
+// indirect indication that we are not doing extra Promise processing that
+// could be associated with memory leaks (v8:5380).
+// TODO(littledan): Write a test that demonstrates that the memory leak in
+// the exception case is fixed.
+
 Debug = debug.Debug;
 
 var base_id = -1;
 var exception = null;
 var expected = [
-  "enqueue #1",
-  "willHandle #1",
-  "then #1",
-  "enqueue #2",
-  "enqueue #3",
-  "didHandle #1",
-  "willHandle #2",
-  "then #2",
-  "didHandle #2",
-  "willHandle #3",
-  "enqueue #4",
-  "didHandle #3",
-  "willHandle #4",
-  "didHandle #4",
+  'enqueue #1',
+  'willHandle #1',
+  'then #1',
+  'enqueue #2',
+  'didHandle #1',
+  'willHandle #2',
+  'then #2',
+  'didHandle #2',
 ];
 
 function assertLog(msg) {