Async/await catch prediction for "the synchronous case"

src/isolate.cc

Index: src/isolate.cc
diff --git a/src/isolate.cc b/src/isolate.cc
index 1b96583cb093cd44eb2022945e59b2a6bc185322..b9c35b7fae1abc8f9c874f604a5d547b1ace8b03 100644
--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -1703,29 +1703,59 @@ void Isolate::PopPromise() {
   global_handles()->Destroy(global_promise.location());
 }
 
+bool Isolate::PromiseHasUserDefinedRejectHandler(Handle<Object> promise) {
+  Handle<JSFunction> fun = promise_has_user_defined_reject_handler();
+  Handle<Object> has_reject_handler;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(this, has_reject_handler,

[jgruber, 2016/09/12 10:27:31]

We need to handle thrown exceptions, either by clearing them or passing them on
(or ToHandleChecked if fun should never throw). Clearing the exception sounds
wrong to me in this case, we should probably return a MaybeHandle<Object>.

[Dan Ehrenberg, 2016/09/12 22:12:11]

I think it's OK to handle the exceptions like this. An exception can only be
thrown if there is a bug, or if the page is shutting down. In either case, an
exception would just result in a misprediction that there is no error.

[jgruber, 2016/09/13 08:17:20]

If we don't care about any thrown exceptions in
promise_has_user_defined_reject_handler, then we still need to
clear_pending_exception() before returning false.

Continuing execution in V8 code when an exception is pending is a bug.
See e.g. Compiler::CompileBaseline for another case where a thrown exception is
intentionally swallowed.

[Dan Ehrenberg, 2016/09/13 22:34:55]

Done

[adamk, 2016/09/15 00:11:07]

Would Execution::TryCall() work instead of calling clear_pending_exception()
explicitly?

[Dan Ehrenberg, 2016/09/15 00:37:40]

Done

+                                   Execution::Call(this, fun, promise, 0, NULL),
+                                   false);
+  return has_reject_handler->IsTrue(this);
+}
 
 Handle<Object> Isolate::GetPromiseOnStackOnThrow() {
-  Handle<Object> undefined = factory()->undefined_value();
+  Handle<Object> retval = factory()->undefined_value();
   ThreadLocalTop* tltop = thread_local_top();
-  if (tltop->promise_on_stack_ == NULL) return undefined;
+  if (tltop->promise_on_stack_ == NULL) return retval;

[kozy, 2016/09/12 18:13:56]

Can we return factory()->undefined_value() here..

[Dan Ehrenberg, 2016/09/12 22:12:11]

Done

   // Find the top-most try-catch or try-finally handler.
   CatchType prediction = PredictExceptionCatcher();
   if (prediction == NOT_CAUGHT || prediction == CAUGHT_BY_EXTERNAL) {
-    return undefined;
+    return retval;

[kozy, 2016/09/12 18:13:56]

..and here, and then introduce retval variable.

[Dan Ehrenberg, 2016/09/12 22:12:11]

Done

   }
+  PromiseOnStack* promise_on_stack = tltop->promise_on_stack_;
   for (JavaScriptFrameIterator it(this); !it.done(); it.Advance()) {
     switch (PredictException(it.frame())) {
       case HandlerTable::UNCAUGHT:
-      case HandlerTable::ASYNC_AWAIT:
-        break;
+        continue;
       case HandlerTable::CAUGHT:
       case HandlerTable::DESUGARING:
-        return undefined;
+        if (retval->IsJSObject()) {

[jgruber, 2016/09/12 10:27:31]

Would a DCHECK(retval->IsJSPromise()) work here?

[Dan Ehrenberg, 2016/09/12 22:12:11]

No, as this may be occurring in a case that has nothing to do with async/await
(just like before the patch). It may return undefined, or may return a Promise
if we've seen async/await frames before.

[jgruber, 2016/09/13 08:17:20]

I meant 

if (retval->IsJSObject()) {
  DCHECK(retval->IsJSPromise());

or even

if (retval->IsJSPromise())

It seems like the only time retval is assigned anything other than undefined is
below when assigning promise_on_stack->promise().

Not a big deal, it'd just be nice to document if we only expect JSPromises here.

[Dan Ehrenberg, 2016/09/13 22:34:55]

Done

+          // Caught the result of an inner async/await invocation.
+          // Mark the inner promise as caught in the "synchronous case" so
+          // that Debug::OnException will see.
+          Handle<Symbol> key = factory()->promise_handled_hint_symbol();

[jgruber, 2016/09/12 10:27:31]

Which case is this so that the promise isn't marked by AsyncFunctionAwaitCaught?
Is it a delayed await, e.g. 'try { var promise = ...; await promise; } ...'?

Could you document that here? Do we have a covering test case?

Also, isn't the catch prediction known at the same time as when you mutate the
called runtime function in ast-numbering? Wouldn't it be better to do this
marking in a rewriting ast visitor pass as well?

And more generally, it would be nice to have all promise catch-prediction
related symbols documented somewhere in the codebase. By now there's quite a few
of them and understanding what they mean is tricky without spending some time
with the code.

[Dan Ehrenberg, 2016/09/12 22:12:11]

I added more comment text to try to explain. This is essential for the
synchronous case, where we haven't hit that code that runs afterwards yet.

+          JSObject::SetProperty(Handle<JSObject>::cast(retval), key,
+                                factory()->true_value(), STRICT)
+              .Assert();
+        }
+        return retval;
       case HandlerTable::PROMISE:
-        return tltop->promise_on_stack_->promise();
+        return promise_on_stack->promise();
+      case HandlerTable::ASYNC_AWAIT: {
+        // If in the initial portion of async/await, continue the loop to pop up
+        // successive async/await stack frames until an asynchronous one with
+        // dependents is found, or a non-async stack frame is encountered, in
+        // order to handle the synchronous async/await catch prediction case:
+        // assume that async function calls are awaited.
+        retval = promise_on_stack->promise();
+        if (PromiseHasUserDefinedRejectHandler(retval)) {
+          return retval;
+        }
+        promise_on_stack = promise_on_stack->prev();
+        continue;
+      }
     }
   }
-  return undefined;
+  return retval;
 }