How to rescue issues caused by v8::Isolate termination

[Yuki Shiino]

Hi V8-team and platform-architecture-dev,

TL;DR: We'd like to extend v8::TryCatch APIs a little.

We're having an issue how to handle worker termination, i.e. v8::Isolate termination.  Roughly speaking, the situation is like below.

Main thread:

    v8::Isolate* worker_isolate = ...;

    worker_isolate->TerminateExecution();  // Terminates the worker isolate.

Worker thread (worker isolate):

    v8::TryCatch try_catch(isolate);

    DoSomethingWithV8();  // The Isolate terminates here.

    if (try_catch.HasCaught()) {  // => true due to termination

      // Handle error or termination.

      return;

    }

No problem so far.  Worker thread MUST NOT run any V8 code no longer because the Isolate is terminating.  However, Blink is not perfect, and it's pretty tough for Blink to stop everything with 100% correctness.  Occasionally (or rarely) Blink continues running more V8 code like below.

Worker thread (worker isolate):

    v8::TryCatch try_catch(isolate);

    DoSomethingElseWithV8();

    if (try_catch.HasCaught()) {  // => false because no new exception is thrown inside the v8::TryCatch.

      return;  // Blink doesn't reach here.

    }

    // Blink reach here instead.  :(

If v8::TryCatch::HasCaught() returned true, Blink would be able to handle it as error and Blink would work much better than now (Blink is now crashing).

So, proposals here are something like below (not yet so concrete).

a) Make v8::TryCatch::HasCaught() return true if there already exists a pending exception.  (Maybe this is no good.)

b) Make a new API like v8::TryCatch::HasPendingException() and make it return true.  Blink rewrites all HasCaught() to the new one.

Similarly,

a2) Make v8::TryCatch::Exception() return a pending exception if there already exists.

b2) Make a new API like v8::TryCatch::PendingException() and make it return a thrown exception or pending exception in the isolate if any.  Blink rewrites all Exception() to the new one.

What do you think of the issue and proposals?

Cheers,

Yuki Shiino

[Yutaka Hirano]

+ricea@

[Kentaro Hara]

Sorry, I'm not sure if I understand your example...

On Wed, Nov 7, 2018 at 2:21 AM Yutaka Hirano <yhi...@chromium.org> wrote:

+ricea@

On Wed, Nov 7, 2018 at 6:44 PM Yuki Shiino <yukis...@chromium.org> wrote:

Hi V8-team and platform-architecture-dev,

TL;DR: We'd like to extend v8::TryCatch APIs a little.

We're having an issue how to handle worker termination, i.e. v8::Isolate termination.  Roughly speaking, the situation is like below.

Main thread:

    v8::Isolate* worker_isolate = ...;

    worker_isolate->TerminateExecution();  // Terminates the worker isolate.

Worker thread (worker isolate):

    v8::TryCatch try_catch(isolate);

    DoSomethingWithV8();  // The Isolate terminates here.

    if (try_catch.HasCaught()) {  // => true due to termination

      // Handle error or termination.

      return;

    }

No problem so far.  Worker thread MUST NOT run any V8 code no longer because the Isolate is terminating.  However, Blink is not perfect, and it's pretty tough for Blink to stop everything with 100% correctness.  Occasionally (or rarely) Blink continues running more V8 code like below.

Worker thread (worker isolate):

    v8::TryCatch try_catch(isolate);

    DoSomethingElseWithV8();

    if (try_catch.HasCaught()) {  // => false because no new exception is thrown inside the v8::TryCatch.

      return;  // Blink doesn't reach here.

    }

    // Blink reach here instead.  :(

If v8::TryCatch::HasCaught() returned true, Blink would be able to handle it as error and Blink would work much better than now (Blink is now crashing).

Are you assuming that the worker is terminated while it's calling DoSomethingElseWithV8()? If yes, why does HasCaught() not return true? If no, what's a problem of continuing execution?

So, proposals here are something like below (not yet so concrete).

a) Make v8::TryCatch::HasCaught() return true if there already exists a pending exception.  (Maybe this is no good.)

b) Make a new API like v8::TryCatch::HasPendingException() and make it return true.  Blink rewrites all HasCaught() to the new one.

Similarly,

a2) Make v8::TryCatch::Exception() return a pending exception if there already exists.

b2) Make a new API like v8::TryCatch::PendingException() and make it return a thrown exception or pending exception in the isolate if any.  Blink rewrites all Exception() to the new one.

What do you think of the issue and proposals?

Cheers,

Yuki Shiino

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABihn6ER8q9QABC3ROCSkm6V4w%2BDb0xEqRvjgzwBpMG9DmiG1Q%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Yuki Shiino]

Are you assuming that the worker is terminated while it's calling DoSomethingElseWithV8()? If yes, why does HasCaught() not return true? If no, what's a problem of continuing execution?

No, the worker is terminated while DoSomethingWithV8 (without "Else"), and Blink continues running more V8 code at DoSomethingElseWithV8.  Two pieces of code run consecutively.

Worker thread (worker isolate):

    {

      v8::TryCatch try_catch(isolate);

      DoSomethingWithV8();  // The Isolate terminates here.

      if (try_catch.HasCaught()) {  // => true due to termination

        // Handle error or termination.

        return;

      }

    }

    {

      v8::TryCatch try_catch(isolate);

      DoSomethingElseWithV8();  // V8 APIs fail because the Isolate is terminating.

      if (try_catch.HasCaught()) {  // => false because no new exception is thrown inside the v8::TryCatch.

        return;  // Blink doesn't reach here.

      }

      // Blink reach here instead.  :(

    }

IIUC, the second try_catch.HasCaught() doesn't return true because there is no new exception thrown inside the v8::TryCatch scope, although V8 APIs fail inside DoSomethingElseWithV8 due to a pending termination exception.

Cheers,

Yuki Shiino

2018年11月8日(木) 3:18 Kentaro Hara <har...@chromium.org>:

[Kentaro Hara]

Thanks, I got it :)

My proposal would be:

(a3) Make HasCaught() return true when the isolate is terminated.

I'm not sure if (a) or (a2) is a good idea because the fact that we didn't call ReThrow() means that we (intentionally) suppressed the exception. Thoughts?

[Yuki Shiino]

+adamk, cbruni to get more attention from V8 team.  This needs V8 team's support.

Actually, I intended haraken's (a3) at my proposal (a), but I'm afraid that changing an existing API HasCaught() would break backward compatibility.  So, I'm also proposing to add a new V8 API like v8::TryCatch::HasPendingException or v8::TryCatch::HasCaughtOrTerminated or whatever we call it.

By the way, the example code is just an example.  Usually we don't have two TryCatch blocks next to each other, and Blink is rethrowing the exception in most cases.  I just forgot to write rethrow in the example.  That's not a point.  Let me revise the example.

Main thread:

    v8::Isolate* worker_isolate = ...;

    worker_isolate->TerminateExecution();  // Terminates the worker isolate.

Worker thread (worker isolate):

    Foo();  // The v8::Isolate terminates during execution of Foo.

    Bar();

where Foo and Bar are the followings.

    void Foo() {

      v8::TryCatch try_catch1(isolate);

      // Call V8 APIs.  The v8::Isolate gets terminated at this point.

      if (try_catch1.HasCaught()) {  // => true due to the termination exception.

        // Rethrow or report the error to global error handlers.

        return;

      }

    }

    void Bar() {

      v8::TryCatch try_catch2(isolate);

      // Call V8 APIs.  V8 APIs fail because the isolate is terminating.

      if (try_catch2.HasCaught()) {  // => false because no new exception is thrown inside |try_catch2| scope.

        // Rethrow or report the error to global error handlers.

        return;

      }

      // Blink reaches here.  :(

    }

A proposed solution looks like the following.

    v8::TryCatch try_catch(isolate);

    // ...

    if (try_catch.HasCaughtOrTerminated()) {

      v8::Local<v8::Exception> exception = try_catch.CaughtExceptionOrTerminationException();

      // Do a job with |exception|.

      return;

    }

where HasCaughtOrTerminated returns true when the isolate is terminating even if no exception is thrown inside the TryCatch scope, and CaughtExceptionOrTerminationException returns a thrown exception if an exception is thrown inside the TryCatch scope or the termination exception if the isolate is terminating.

Cheers,

Yuki Shiino

2018年11月8日(木) 17:38 Kentaro Hara <har...@chromium.org>:

[Kentaro Hara]

On Thu, Nov 8, 2018 at 1:10 AM Yuki Shiino <yukis...@chromium.org> wrote:

+adamk, cbruni to get more attention from V8 team.  This needs V8 team's support.

Actually, I intended haraken's (a3) at my proposal (a), but I'm afraid that changing an existing API HasCaught() would break backward compatibility.  So, I'm also proposing to add a new V8 API like v8::TryCatch::HasPendingException or v8::TryCatch::HasCaughtOrTerminated or whatever we call it.

By the way, the example code is just an example.  Usually we don't have two TryCatch blocks next to each other, and Blink is rethrowing the exception in most cases.  I just forgot to write rethrow in the example.  That's not a point.  Let me revise the example.

Main thread:

    v8::Isolate* worker_isolate = ...;

    worker_isolate->TerminateExecution();  // Terminates the worker isolate.

Worker thread (worker isolate):

    Foo();  // The v8::Isolate terminates during execution of Foo.

    Bar();

where Foo and Bar are the followings.

    void Foo() {

      v8::TryCatch try_catch1(isolate);

      // Call V8 APIs.  The v8::Isolate gets terminated at this point.

      if (try_catch1.HasCaught()) {  // => true due to the termination exception.

        // Rethrow or report the error to global error handlers.

        return;

      }

    }

    void Bar() {

      v8::TryCatch try_catch2(isolate);

      // Call V8 APIs.  V8 APIs fail because the isolate is terminating.

      if (try_catch2.HasCaught()) {  // => false because no new exception is thrown inside |try_catch2| scope.

        // Rethrow or report the error to global error handlers.

        return;

      }

      // Blink reaches here.  :(

    }

Hmm, I'm a bit confused.

If Foo() rethrows the exception, why does the worker thread continue execution and call Bar()? That will cause a problem even when the worker is not terminated (because the worker continues execution ignoring the thrown exception)...?

Also I might want to understand how widely the problem is happening. First of all, it is not realistic to handle worker's sudden termination in 100% cases (unless we add checks to all V8 API calls in the Blink code base). So the best thing we can do is to insert the termination check to places that may call scripts (e.g., V8ScriptRunner, EventListener, Promise resolve / reject etc) and reduce the crash rate. We could introduce HasCaughtOrTerminated() if it helps to reduce the crash rate, but I want to make sure that assumption is correct. My intuition is that it will be more useful to identify places that may call scripts often and insert the termination checks if missing.

[Adam Klein]

Adding a couple more V8 folks who may have thoughts.

I'd similarly like to understand how much this happens in practice.

 

A proposed solution looks like the following.

    v8::TryCatch try_catch(isolate);

    // ...

    if (try_catch.HasCaughtOrTerminated()) {

      v8::Local<v8::Exception> exception = try_catch.CaughtExceptionOrTerminationException();

      // Do a job with |exception|.

      return;

    }

where HasCaughtOrTerminated returns true when the isolate is terminating even if no exception is thrown inside the TryCatch scope, and CaughtExceptionOrTerminationException returns a thrown exception if an exception is thrown inside the TryCatch scope or the termination exception if the isolate is terminating.

As to the problem itself, I have a clarifying questions: doesn't Blink already have some choke point to determine whether it's "ok to call script now"? If so that'd be a more natural place to add this handling than TryCatch.

- Adam

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jx51JXHEt0H9z%3DzT%3D0GnhDf79QjhGtVxRc%3Dy-RXjHXUyw%40mail.gmail.com.

[Yutaka Hirano]

> As to the problem itself, I have a clarifying questions: doesn't Blink already have some choke point to determine whether it's "ok to call script now"? If so that'd be a more natural place to add this handling than TryCatch.

No, there isn't such a point. To make matters worse, some call-sites don't check errors because they don't care.

Sorry for the ignorance but I would like to know V8's policy on termination.

1. What happens if the isolate is forcibly terminated (from another thread) while running a script? Will an exception be thrown? Is a v8::TryCatch catches the exception?

2. What happens if we try to run a script when the isolate has already been terminated?

3. What happens if v8 calls blink code, the isolate is forcibly terminated and then the control is back to v8?

[Adam Klein]

On Thu, Nov 8, 2018 at 8:21 PM Yutaka Hirano <yhi...@chromium.org> wrote:

> As to the problem itself, I have a clarifying questions: doesn't Blink already have some choke point to determine whether it's "ok to call script now"? If so that'd be a more natural place to add this handling than TryCatch.

No, there isn't such a point. To make matters worse, some call-sites don't check errors because they don't care.

Sorry for the ignorance but I would like to know V8's policy on termination.

1. What happens if the isolate is forcibly terminated (from another thread) while running a script? Will an exception be thrown? Is a v8::TryCatch catches the exception?

2. What happens if we try to run a script when the isolate has already been terminated?

3. What happens if v8 calls blink code, the isolate is forcibly terminated and then the control is back to v8?

I'm not intimately familiar with the details here. Yang or Toon, perhaps you have more insight?

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABihn6GKuTUs5UfpbQ6RwXq%3DWw0dDUJkq2JLzadnesfjLG%3DStg%40mail.gmail.com.

[Yang Guo]

When you terminate execution in V8, we abort execution until the bottom-most call into V8. If you have re-entries into V8, V8 always returns empty results until the bottom-most call into V8. On the Blink side on the stack of the re-entries, you can try to call into V8 before returning, but that will simply return empty results. v8::TryCatch scopes along the way will return true for v8::TryCatch::HasCaught and v8::TryCatch::HasTerminated. Isolate::IsExecutionTerminating returns true.

As soon as we reach the bottom-most call, we return with an empty value as well. The v8::TryCatch scope around that will return true for v8::TryCatch::HasCaught and v8::TryCatch::HasTerminated, but Isolate::IsExecutionTerminating will return false (even if you are still inside this outer-most v8::TryCatch scope), because you can safely call into V8 again, from here on. I actually find this a bit counter-intuitive, and it might be better to have Isolate::IsExecutionTerminating return true, until we leave the outer-most v8::TryCatch. Though implementing that seems a bit annoying.

So what you are observing is that you have a non-reentry call to execute the worker. That terminates, and you then have another non-reentry call. That is working as intended though. Once you have left V8 through termination across all re-entries, the isolate is good to be re-used again. I think the correct way to fix your issue is to, at non-reentry calls, always check for v8::TryCatch::HasTerminated, and use that to guide the following control flow.

To answer your questions:

1. What happens if the isolate is forcibly terminated (from another thread) while running a script? Will an exception be thrown? Is a v8::TryCatch catches the exception?

Internally we throw a special exception that cannot be caught by javascript. This exception causes execution to abort until we arrive at the first (non-reentry) call into V8.

2. What happens if we try to run a script when the isolate has already been terminated?

If you completely exited V8 back to the first non-reentry call, you can safely use the isolate again.

3. What happens if v8 calls blink code, the isolate is forcibly terminated and then the control is back to v8?

The termination exception is propagated back to V8, causes the current execution to abort, so that we return an empty value to blink as soon as possible. If this blink frame is called from V8, then calling into V8 will only result in empty values.

Cheers,

Yang

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAEvLGcKKKdZ4-Svz7NmXdzSaL9ryQz4cPEqhTfmWEURzwTdMqQ%40mail.gmail.com.

[Yang Guo]

I filed a bug for the slightly counter-intuitive behavior I mentioned: https://bugs.chromium.org/p/v8/issues/detail?id=8455

Cheers,

Yang

[Yang Guo]

Hi,

I found that at the only place Isolate::TerminateExecution is called from blink, V8 is not even running. That would mean that we don't have to worry about any of this?

Cheers,

Yang

[Kentaro Hara]

I found that at the only place Isolate::TerminateExecution is called from blink, V8 is not even running. That would mean that we don't have to worry about any of this?

At that place the main thread (which is not running V8) is calling TerminateExecution to terminate a running worker thread. The concern is on the worker thread.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAFSTc_iBZUPpYMuUk3ouOe7cLBDizJFUjmcWuaAeVZfMBn4QqQ%40mail.gmail.com.

[Yutaka Hirano]

Hi Yang,

Thank you for the information!

Sorry for the late response. I will send a reply next week.

Thanks,

--
--
v8-users mailing list
v8-u...@googlegroups.com
http://groups.google.com/group/v8-users
---
You received this message because you are subscribed to the Google Groups "v8-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-users+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Yutaka Hirano]

Hi,

I found I don't understand the direction. If there are only two levels, say blink-calls-v8(A)-calls-blink-calls-v8(B), which is the bottom-most v8 call, A or B?

Thanks,

[Yang Guo]

Sorry. I should have been more explicit here. My image of the stack is growing bottom up. So A is the bottom-most V8 call.

Yang

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABihn6H0_3BFq5S2wv5Y8CDyW%3Dv-HjwskH1Wds6PmTWX5akE9g%40mail.gmail.com.

[Yutaka Hirano]

Hi,

Let's consider the following sequence.

-----------------------------> time

main     -----T------------

worker   --*E1**--*E2***---

T: Call TerminateExecution with the worker isolate (on the main thread)

E1: bottom-most script evaluation (* means running)

E2: bottom-most script evaluation (* means running)

In this case, E1 is terminated due to TerminateExecution, but E2 runs normally (i.e., may return a non-empty value) because "the isolate is good to be re-used again", right?

Another question: If E1 starts running after TerminateExecution is called, what happens?

main     T-----------------

worker   --*E1**-----------

T: Call TerminateExecution with the worker isolate (on the main thread)

E1: bottom-most script evaluation (* means running)

Will E1 be aborted due to a past TerminateExecution call, or will it run because "the isolate is good to be re-used again"?

Thanks,

[Yutaka Hirano]

Thanks for the reply.

As Kentaro said, Blink uses TerminateExecution only when it tries to terminate a worker thread forcibly. The isolation will soon be disposed, and the "recovering" functionality is actually harmful for us.

main     -----T------------

worker   --*E1**--*E2******

T: Call TerminateExecution with the worker isolate (on the main thread)

E1: bottom-most script evaluation (* means running)

E2: bottom-most script evaluation (* means running)

In this case, the main thread wants to terminate the worker thread, but it calls TerminateExecution only once so it E2 runs potentially forever.

If there is an option that ensures that future v8 calls will fail whenever possible and the v8::TryCatch scope around that will return true for v8::TryCatch::HasCaught, then we'll be happy. Is it possible?

[Kentaro Hara]

Would it be hard to make Blink immediately terminate the worker thread after E1 is forcibly terminated?

Blink is already doing that for common script execution paths e.g., event listeners, call functions etc.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABihn6F4PfQJML_kNgt5Et2nhfQfVBuXYqaNyHyORAGtbVbEjg%40mail.gmail.com.

[Yang Guo]

I could change V8 to not clear the termination exception so that we always stay in the terminated mode and not recover. However, from experience I expect tons of tests to fail if I implemented this change.

What you could do is add a check after E1 for v8::TryCatch::HasTerminated(), and schedule another Isolate::TerminateExecution() if true. You would need to do this for E2 as well, if you expect an E3, and so forth.

Cheers,

Yang

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jz-oO0wE0ZGzdZ8%3DuXdZdj7UvR_KKG_12%2B_-67-LA_2Cw%40mail.gmail.com.

[Yutaka Hirano]

On Thu, Nov 22, 2018 at 8:53 AM Kentaro Hara <har...@chromium.org> wrote:

Would it be hard to make Blink immediately terminate the worker thread after E1 is forcibly terminated?

Blink is already doing that for common script execution paths e.g., event listeners, call functions etc.

Script execution is scattered in the code base so it's hard to guarantee that. Most of them are short-running, so the problem should be rare, but it will confuse us when the problem happens.

On Thu, Nov 22, 2018 at 5:25 PM Yang Guo <yan...@chromium.org> wrote:

I could change V8 to not clear the termination exception so that we always stay in the terminated mode and not recover. However, from experience I expect tons of tests to fail if I implemented this change.

I was thinking of introducing a new option, but yes, changing the default behavior may impact tests and users other than blink.

 

What you could do is add a check after E1 for v8::TryCatch::HasTerminated(), and schedule another Isolate::TerminateExecution() if true. You would need to do this for E2 as well, if you expect an E3, and so forth.

That might be possible by migrating ExceptionState.RethrowV8Exception(v8::Local<v8::Value>) to ExceptionState.RethrowV8Exception(v8::TryCatch&). yukishiino@, how hard would it be?

[Yuki Shiino]

What you could do is add a check after E1 for v8::TryCatch::HasTerminated(), and schedule another Isolate::TerminateExecution() if true. You would need to do this for E2 as well, if you expect an E3, and so forth.

That might be possible by migrating ExceptionState.RethrowV8Exception(v8::Local<v8::Value>) to ExceptionState.RethrowV8Exception(v8::TryCatch&). yukishiino@, how hard would it be?

Bindings is part of Blink.  "after E1" means "after Blink callback exits back to V8", I think?  Then, there seems no chance for ExceptionState or any bindings code to do it.

Or, is it okay to call v8::Isolate::TerminateExecution() before Blink exits back to V8?

2018年11月22日(木) 17:54 Yutaka Hirano <yhi...@chromium.org>:

[Yuki Shiino]

Hi Yang,

 

I could change V8 to not clear the termination exception so that we always stay in the terminated mode and not recover. However, from experience I expect tons of tests to fail if I implemented this change.

By the way, what are "tons of tests"?  Are they V8 tests? or Blink tests (web tests / layout tests)? or other embedder's tests?

If they're not Blink tests, can we introduce a new API like Isolate::TerminateExecutionAndKeepExecutionTerminated?

2018年11月22日(木) 18:36 Yuki Shiino <yukis...@chromium.org>:

[Yang Guo]

I'm assuming tests with the proxy resolver and inspector related tests. Both assume that we can resume after termination.

Adding a new API like you suggest indeed might make sense. We would set a flag on the isolate which prevents us from clearing the termination exception. Any opinions on this?

Cheers,

Yang

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAN0uC_Q%3Da8RqGpskEyJP-8voXk%3Dq_hPJxvyaxAnowFo1AC2LYw%40mail.gmail.com.