Async/await catch prediction for "the synchronous case"

Async/await catch prediction for "the synchronous case"

Handle the "synchronous case" by marking try/catch blocks introduced for
async functions as ASYNC_AWAIT and traversing up the stack, finding successive
Promises and returning caught if any of them are predicted to be caught.

BUG=v8:5167
Committed: https://crrev.com/7776370c587e59012613b4d468f0de7ad0d25a8a
Cr-Commit-Position: refs/heads/master@{#39433}

[Dan Ehrenberg, 2016-09-08 22:32:01]

littledan@chromium.org changed reviewers:
+ adamk@chromium.org, jgruber@chromium.org, kozyatinskiy@chromium.org

[Dan Ehrenberg, 2016-09-08 22:34:17]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 22:34:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 23:06:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 23:06:38]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-09-09 17:30:32]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 17:30:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 18:00:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:00:41]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-09-09 18:01:51]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 18:02:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-09 18:05:19]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 18:05:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 18:51:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:51:37]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-09 19:13:44]

This lgtm, but I'd like to at least get kozyatinskiy's opinion too (on the
semantics).

https://codereview.chromium.org/2325813002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2325813002/diff/60001/src/isolate.h#newcode676
src/isolate.h:676: Handle<Object> GetPromiseOnStackOnThrow();
Please give this a comment while you're changing what it does.

[Dan Ehrenberg, 2016-09-10 01:58:03]

https://codereview.chromium.org/2325813002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2325813002/diff/60001/src/isolate.h#newcode676
src/isolate.h:676: Handle<Object> GetPromiseOnStackOnThrow();
On 2016/09/09 at 19:13:44, adamk wrote:
> Please give this a comment while you're changing what it does.

Done

[Dan Ehrenberg, 2016-09-10 01:58:06]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 01:58:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 01:59:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 01:59:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8588)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23747)

[Dan Ehrenberg, 2016-09-10 02:19:47]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 02:19:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 02:51:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 02:51:03]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-09-12 10:27:32]

Looking good. Some comments:

https://codereview.chromium.org/2325813002/diff/1/src/runtime/runtime-interna...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2325813002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:312: // undefined, which will be interpreted by
uncaught as being a
'interpreted by uncaught' -> Typo?

https://codereview.chromium.org/2325813002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2325813002/diff/60001/src/isolate.h#newcode676
src/isolate.h:676: Handle<Object> GetPromiseOnStackOnThrow();
On 2016/09/10 01:58:03, Dan Ehrenberg wrote:
> On 2016/09/09 at 19:13:44, adamk wrote:
> > Please give this a comment while you're changing what it does.
> 
> Done

I think Adam meant a comment on GetPromiseOnStackOnThrow. I also believe a
comment would be nice, especially since the method does not return the top-most
promise anymore (which is kind of implied by the method name).

https://codereview.chromium.org/2325813002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2325813002/diff/80001/src/isolate.h#newcode678
src/isolate.h:678: // Heuristically guess whether a Promise is handled by user
catch handler
Is this comment meant for PromiseHasUserDefinedRejectHandler? If yes, isn't it
somewhat misleading since that only reads and returns a property?

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1709: ASSIGN_RETURN_ON_EXCEPTION_VALUE(this, has_reject_handler,
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>.

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1731: if (retval->IsJSObject()) {
Would a DCHECK(retval->IsJSPromise()) work here?

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1735: Handle<Symbol> key =
factory()->promise_handled_hint_symbol();
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.

https://codereview.chromium.org/2325813002/diff/100001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2325813002/diff/100001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:303:
RUNTIME_FUNCTION(Runtime_PromiseRejectEventFromStack) {
Just as an aside, maybe we could move these to runtime-promise.cc at some point.

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

lgtm

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1718: if (tltop->promise_on_stack_ == NULL) return retval;
Can we return factory()->undefined_value() here..

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1722: return retval;
..and here, and then introduce retval variable.

https://codereview.chromium.org/2325813002/diff/100001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-debug-caught-exception-cases.js (right):

https://codereview.chromium.org/2325813002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception-cases.js:143: if (caught)
Debug.clearBreakOnException();
Please format it as if in line 133.

[Dan Ehrenberg, 2016-09-12 22:06:27]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 22:06:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-12 22:12:12]

https://codereview.chromium.org/2325813002/diff/1/src/runtime/runtime-interna...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2325813002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:312: // undefined, which will be interpreted by
uncaught as being a
On 2016/09/12 at 10:27:31, jgruber wrote:
> 'interpreted by uncaught' -> Typo?

Fixed

https://codereview.chromium.org/2325813002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2325813002/diff/60001/src/isolate.h#newcode676
src/isolate.h:676: Handle<Object> GetPromiseOnStackOnThrow();
On 2016/09/12 at 10:27:31, jgruber wrote:
> On 2016/09/10 01:58:03, Dan Ehrenberg wrote:
> > On 2016/09/09 at 19:13:44, adamk wrote:
> > > Please give this a comment while you're changing what it does.
> > 
> > Done
> 
> I think Adam meant a comment on GetPromiseOnStackOnThrow. I also believe a
comment would be nice, especially since the method does not return the top-most
promise anymore (which is kind of implied by the method name).

Oops, gave a comment on that too. It's still returning a Promise that's on the
stack!

https://codereview.chromium.org/2325813002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2325813002/diff/80001/src/isolate.h#newcode678
src/isolate.h:678: // Heuristically guess whether a Promise is handled by user
catch handler
On 2016/09/12 at 10:27:31, jgruber wrote:
> Is this comment meant for PromiseHasUserDefinedRejectHandler? If yes, isn't it
somewhat misleading since that only reads and returns a property?

Not sure what you mean; this function calls out to a function implemented in
JavaScript to do a dependency graph traversal to answer the question.

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1709: ASSIGN_RETURN_ON_EXCEPTION_VALUE(this, has_reject_handler,
On 2016/09/12 at 10:27:31, jgruber wrote:
> 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>.

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.

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1718: if (tltop->promise_on_stack_ == NULL) return retval;
On 2016/09/12 at 18:13:56, kozyatinskiy wrote:
> Can we return factory()->undefined_value() here..

Done

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1722: return retval;
On 2016/09/12 at 18:13:56, kozyatinskiy wrote:
> ..and here, and then introduce retval variable.

Done

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1731: if (retval->IsJSObject()) {
On 2016/09/12 at 10:27:31, jgruber wrote:
> Would a DCHECK(retval->IsJSPromise()) work here?

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.

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1735: Handle<Symbol> key =
factory()->promise_handled_hint_symbol();
On 2016/09/12 at 10:27:31, jgruber wrote:
> 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.

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.

https://codereview.chromium.org/2325813002/diff/100001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-debug-caught-exception-cases.js (right):

https://codereview.chromium.org/2325813002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception-cases.js:143: if (caught)
Debug.clearBreakOnException();
On 2016/09/12 at 18:13:56, kozyatinskiy wrote:
> Please format it as if in line 133.

Done

[commit-bot: I haz the power, 2016-09-12 22:24:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 22:24:30]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

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

lgtm modulo test failures and comments below

https://codereview.chromium.org/2325813002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2325813002/diff/80001/src/isolate.h#newcode678
src/isolate.h:678: // Heuristically guess whether a Promise is handled by user
catch handler
On 2016/09/12 22:12:11, Dan Ehrenberg wrote:
> On 2016/09/12 at 10:27:31, jgruber wrote:
> > Is this comment meant for PromiseHasUserDefinedRejectHandler? If yes, isn't
it
> somewhat misleading since that only reads and returns a property?
> 
> Not sure what you mean; this function calls out to a function implemented in
> JavaScript to do a dependency graph traversal to answer the question.

You're right. Sorry, my mistake.

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1709: ASSIGN_RETURN_ON_EXCEPTION_VALUE(this, has_reject_handler,
On 2016/09/12 22:12:11, Dan Ehrenberg wrote:
> On 2016/09/12 at 10:27:31, jgruber wrote:
> > 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>.
> 
> 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.

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.

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1731: if (retval->IsJSObject()) {
On 2016/09/12 22:12:11, Dan Ehrenberg wrote:
> On 2016/09/12 at 10:27:31, jgruber wrote:
> > Would a DCHECK(retval->IsJSPromise()) work here?
> 
> 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.

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:33:16]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 22:33:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-13 22:34:56]

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1709: ASSIGN_RETURN_ON_EXCEPTION_VALUE(this, has_reject_handler,
On 2016/09/13 at 08:17:20, jgruber wrote:
> On 2016/09/12 22:12:11, Dan Ehrenberg wrote:
> > On 2016/09/12 at 10:27:31, jgruber wrote:
> > > 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>.
> > 
> > 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.
> 
> 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.

Done

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1731: if (retval->IsJSObject()) {
On 2016/09/13 at 08:17:20, jgruber wrote:
> On 2016/09/12 22:12:11, Dan Ehrenberg wrote:
> > On 2016/09/12 at 10:27:31, jgruber wrote:
> > > Would a DCHECK(retval->IsJSPromise()) work here?
> > 
> > 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.
> 
> 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.

Done

[commit-bot: I haz the power, 2016-09-13 23:03:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 23:03:17]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-09-14 00:17:35]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 00:17:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 00:40:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 00:40:39]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-09-14 01:20:52]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 01:21:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-14 01:22:17]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 01:22:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 01:50:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 01:50:50]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1709: ASSIGN_RETURN_ON_EXCEPTION_VALUE(this, has_reject_handler,
On 2016/09/13 22:34:55, Dan Ehrenberg wrote:
> On 2016/09/13 at 08:17:20, jgruber wrote:
> > On 2016/09/12 22:12:11, Dan Ehrenberg wrote:
> > > On 2016/09/12 at 10:27:31, jgruber wrote:
> > > > 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>.
> > > 
> > > 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.
> > 
> > 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.
> 
> Done

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

[Dan Ehrenberg, 2016-09-15 00:37:26]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 00:37:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2325813002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:1709: ASSIGN_RETURN_ON_EXCEPTION_VALUE(this, has_reject_handler,
On 2016/09/15 at 00:11:07, adamk wrote:
> On 2016/09/13 22:34:55, Dan Ehrenberg wrote:
> > On 2016/09/13 at 08:17:20, jgruber wrote:
> > > On 2016/09/12 22:12:11, Dan Ehrenberg wrote:
> > > > On 2016/09/12 at 10:27:31, jgruber wrote:
> > > > > 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>.
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > Done
> 
> Would Execution::TryCall() work instead of calling clear_pending_exception()
explicitly?

Done

[adamk, 2016-09-15 00:44:11]

lgtm

[Dan Ehrenberg, 2016-09-15 00:48:29]

The CQ bit was unchecked by littledan@chromium.org

[Dan Ehrenberg, 2016-09-15 00:48:32]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-09-15 00:48:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2325813002/#ps220001
(title: "TryCall")

[commit-bot: I haz the power, 2016-09-15 00:48:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 01:16:21]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-09-15 01:16:52]

Description was changed from

==========
Async/await catch prediction for "the synchronous case"

Handle the "synchronous case" by marking try/catch blocks introduced for
async functions as ASYNC_AWAIT and traversing up the stack, finding successive
Promises and returning caught if any of them are predicted to be caught.

BUG=v8:5167
==========

to

==========
Async/await catch prediction for "the synchronous case"

Handle the "synchronous case" by marking try/catch blocks introduced for
async functions as ASYNC_AWAIT and traversing up the stack, finding successive
Promises and returning caught if any of them are predicted to be caught.

BUG=v8:5167

Committed: https://crrev.com/7776370c587e59012613b4d468f0de7ad0d25a8a
Cr-Commit-Position: refs/heads/master@{#39433}
==========

[commit-bot: I haz the power, 2016-09-15 01:16:54]

Patchset 12 (id:??) landed as
https://crrev.com/7776370c587e59012613b4d468f0de7ad0d25a8a
Cr-Commit-Position: refs/heads/master@{#39433}