Allow microtasks to throw exceptions and handle them gracefully

Allow microtasks to throw exceptions and handle them gracefully

If the embedder calls V8::TerminateExecution while we're running microtasks, bail out
and clear any pending microtasks.

All other exceptions are simply swallowed. No current Blink or V8 microtasks throw, this
just ensures something sane happens if another embedder decides to pass a throwing
microtask (or if ours unexpectedly throw due to, e.g., stack exhaustion).

BUG=371566
LOG=Y
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21574

[adamk, 2014-05-22 07:39:52]

This still needs a test. There's a Blink test in
https://codereview.chromium.org/273913002 for which I've verified this is a fix.
But is there a way to write a multithreaded mjsunit test in V8?

[jochen (gone - plz use gerrit), 2014-05-22 07:45:38]

would the stuff in cctest/test-thread-termination.cc work for you? See the
TerminatorThread in there

https://codereview.chromium.org/294943009/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/294943009/diff/1/src/isolate.cc#newcode2299
src/isolate.cc:2299: handle_scope_implementer()->DecrementCallDepth();
why not just break?

[adamk, 2014-05-22 08:02:16]

I'm restructuring this a bit based on feedback from Dan (to just ignore
non-termination exceptions instead of ASSERTing).

https://codereview.chromium.org/294943009/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/294943009/diff/1/src/isolate.cc#newcode2299
src/isolate.cc:2299: handle_scope_implementer()->DecrementCallDepth();
On 2014/05/22 07:45:38, jochen wrote:
> why not just break?

We're inside a while and a for loop. I would have added a goto but I didn't see
any others in v8...

[adamk, 2014-05-22 10:48:47]

The try failures are real. I wasn't seeing them locally, but that's because I
hadn't yet merged https://code.google.com/p/v8/source/detail?r=21427, which
apparently breaks my call to v8::TryCatch::Reset().

Yang and/or Michael, can you comment on the correct way to silently swallow
exceptions inside RunMicrotasks (or perhaps comment if you think that's the
wrong behavior)?

[Yang, 2014-05-22 10:50:44]

On 2014/05/22 10:48:47, adamk wrote:
> The try failures are real. I wasn't seeing them locally, but that's because I
> hadn't yet merged https://code.google.com/p/v8/source/detail?r=21427, which
> apparently breaks my call to v8::TryCatch::Reset().
> 
> Yang and/or Michael, can you comment on the correct way to silently swallow
> exceptions inside RunMicrotasks (or perhaps comment if you think that's the
> wrong behavior)?

You can find an example of swallowing exceptions in Execution::TryCall. We use
the external v8::TryCatch.

[adamk, 2014-05-22 11:23:17]

Thanks for the tip, now using TryCall this should work with mstarzinger's change
merged.

Also added a note to v8.h about the new behavior.

[Michael Starzinger, 2014-05-22 11:27:58]

DBC.

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc#newcode2294
src/isolate.cc:2294: goto done;
Pretty please with a cherry on top, no gotos ... shouldn't a simple break of the
for-loop actually implicitly kick you out of the while-loop as well?

[Yang, 2014-05-22 11:33:25]

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc#newcode2294
src/isolate.cc:2294: goto done;
On 2014/05/22 11:27:59, Michael Starzinger wrote:
> Pretty please with a cherry on top, no gotos ... shouldn't a simple break of
the
> for-loop actually implicitly kick you out of the while-loop as well?

Even nicer could be just a return here. Instead of incrementing/decrementing the
call depth at the start and end, why not use a scope? We even have one, called
SuppressMicrotaskExecutionScope. Might be a misnomer here, but a comment or two
should fix that?

[adamk, 2014-05-22 11:49:25]

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc#newcode2294
src/isolate.cc:2294: goto done;
On 2014/05/22 11:27:59, Michael Starzinger wrote:
> Pretty please with a cherry on top, no gotos ... shouldn't a simple break of
the
> for-loop actually implicitly kick you out of the while-loop as well?

Hmm, that seems a bit fragile to me, if someone changes the while loop later we
might get into trouble. But I'm happy to get rid of the goto.

Instead I've switched to using the SuppressMicrotaskExecutionScope already
written as part of the V8 API. It's a little weird to use something from the API
here but I feel weirder writing the same code again just to avoid a few
reinterpret_casts. Let me know what you think.

[adamk, 2014-05-22 11:58:01]

On 2014/05/22 11:33:25, Yang wrote:
> https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc#newcode2294
> src/isolate.cc:2294: goto done;
> On 2014/05/22 11:27:59, Michael Starzinger wrote:
> > Pretty please with a cherry on top, no gotos ... shouldn't a simple break of
> the
> > for-loop actually implicitly kick you out of the while-loop as well?
> 
> Even nicer could be just a return here. Instead of incrementing/decrementing
the
> call depth at the start and end, why not use a scope? We even have one, called
> SuppressMicrotaskExecutionScope. Might be a misnomer here, but a comment or
two
> should fix that?

Heh, mid-air collision, I was doing that while you sent this.

[Yang, 2014-05-22 11:59:36]

On 2014/05/22 11:58:01, adamk wrote:
> On 2014/05/22 11:33:25, Yang wrote:
> > https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc
> > File src/isolate.cc (right):
> > 
> >
>
https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc#newcode2294
> > src/isolate.cc:2294: goto done;
> > On 2014/05/22 11:27:59, Michael Starzinger wrote:
> > > Pretty please with a cherry on top, no gotos ... shouldn't a simple break
of
> > the
> > > for-loop actually implicitly kick you out of the while-loop as well?
> > 
> > Even nicer could be just a return here. Instead of incrementing/decrementing
> the
> > call depth at the start and end, why not use a scope? We even have one,
called
> > SuppressMicrotaskExecutionScope. Might be a misnomer here, but a comment or
> two
> > should fix that?
> 
> Heh, mid-air collision, I was doing that while you sent this.

:)

[danno-g, 2014-05-22 13:35:06]

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc#newcode2294
src/isolate.cc:2294: goto done;
DBC: Please, no goto. Either the explicit return or the simple break is much
preferable to me over the goto.

On 2014/05/22 11:49:25, adamk wrote:
> On 2014/05/22 11:27:59, Michael Starzinger wrote:
> > Pretty please with a cherry on top, no gotos ... shouldn't a simple break of
> the
> > for-loop actually implicitly kick you out of the while-loop as well?
> 
> Hmm, that seems a bit fragile to me, if someone changes the while loop later
we
> might get into trouble. But I'm happy to get rid of the goto.
> 
> Instead I've switched to using the SuppressMicrotaskExecutionScope already
> written as part of the V8 API. It's a little weird to use something from the
API
> here but I feel weirder writing the same code again just to avoid a few
> reinterpret_casts. Let me know what you think.

[adamk, 2014-05-22 13:48:50]

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/294943009/diff/50001/src/isolate.cc#newcode2294
src/isolate.cc:2294: goto done;
No need to worry, the goto is long gone. I added it before the break was even
possible, as jochen had asked about it on the initial path (and I'd said "hmm, I
could use a goto, but I don't see any in V8...").

Sounds like y'all have an email filter that flags goto :)

On 2014/05/22 13:35:07, danno-g wrote:
> DBC: Please, no goto. Either the explicit return or the simple break is much
> preferable to me over the goto.
> 
> On 2014/05/22 11:49:25, adamk wrote:
> > On 2014/05/22 11:27:59, Michael Starzinger wrote:
> > > Pretty please with a cherry on top, no gotos ... shouldn't a simple break
of
> > the
> > > for-loop actually implicitly kick you out of the while-loop as well?
> > 
> > Hmm, that seems a bit fragile to me, if someone changes the while loop later
> we
> > might get into trouble. But I'm happy to get rid of the goto.
> > 
> > Instead I've switched to using the SuppressMicrotaskExecutionScope already
> > written as part of the V8 API. It's a little weird to use something from the
> API
> > here but I feel weirder writing the same code again just to avoid a few
> > reinterpret_casts. Let me know what you think.
>

[adamk, 2014-05-28 00:40:15]

Ping? Don't worry, no gotos anymore!

[Michael Starzinger, 2014-05-28 11:51:01]

LGTM from my end. Not sure who the original intended reviewer for this was
though.

[adamk, 2014-05-28 16:19:58]

On 2014/05/28 11:51:01, Michael Starzinger wrote:
> LGTM from my end. Not sure who the original intended reviewer for this was
> though.

Thanks! I think Jochen was the original reviewer on this one, in case he cares
to look again.

[adamk, 2014-05-28 18:40:08]

Committed patchset #5 manually as r21574 (presubmit successful).