> > I found that at the only place Isolate::TerminateExecution is called > <https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/workers/worker_thread.cc?type=cs&sq=package:chromium&g=0&l=415> > 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. On Thu, Nov 15, 2018 at 6:51 PM Yang Guo <yang...@chromium.org> wrote: > Hi, > > I found that at the only place Isolate::TerminateExecution is called > <https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/workers/worker_thread.cc?type=cs&sq=package:chromium&g=0&l=415> > from > blink, V8 is not even running. That would mean that we don't have to worry > about any of this? > > Cheers, > > Yang > > On Wed, Nov 14, 2018 at 10:00 AM Yang Guo <yang...@chromium.org> wrote: > >> I filed a bug for the slightly counter-intuitive behavior I mentioned: >> https://bugs.chromium.org/p/v8/issues/detail?id=8455 >> >> Cheers, >> >> Yang >> >> On Wed, Nov 14, 2018 at 9:01 AM Yang Guo <yang...@chromium.org> wrote: >> >>> 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 >>> >>> >>> >>> On Sat, Nov 10, 2018 at 12:15 AM Adam Klein <ad...@chromium.org> wrote: >>> >>>> On Thu, Nov 8, 2018 at 8:21 PM Yutaka Hirano <yhir...@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? >>>> >>>> >>>>> >>>>> >>>>> On Fri, Nov 9, 2018 at 5:30 AM Adam Klein <ad...@chromium.org> wrote: >>>>> >>>>>> Adding a couple more V8 folks who may have thoughts. >>>>>> >>>>>> On Thu, Nov 8, 2018 at 1:59 AM Kentaro Hara <hara...@chromium.org> >>>>>> wrote: >>>>>> >>>>>>> On Thu, Nov 8, 2018 at 1:10 AM Yuki Shiino <yukishi...@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. >>>>>>> >>>>>> >>>>>> 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 >>>>>> >>>>>> >>>>>>> Cheers, >>>>>>>> Yuki Shiino >>>>>>>> >>>>>>>> >>>>>>>> 2018年11月8日(木) 17:38 Kentaro Hara <hara...@chromium.org>: >>>>>>>> >>>>>>>>> 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? >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Nov 7, 2018 at 9:51 PM Yuki Shiino < >>>>>>>>> yukishi...@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> 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 <hara...@chromium.org>: >>>>>>>>>> >>>>>>>>>>> Sorry, I'm not sure if I understand your example... >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Nov 7, 2018 at 2:21 AM Yutaka Hirano < >>>>>>>>>>> yhir...@chromium.org> wrote: >>>>>>>>>>> >>>>>>>>>>>> +ricea@ >>>>>>>>>>>> >>>>>>>>>>>> On Wed, Nov 7, 2018 at 6:44 PM Yuki Shiino < >>>>>>>>>>>> yukishi...@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-architecture-dev+unsubscr...@chromium.org. >>>>>>>>>>>> To post to this group, send email to >>>>>>>>>>>> platform-architecture-...@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 >>>>>>>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABihn6ER8q9QABC3ROCSkm6V4w%2BDb0xEqRvjgzwBpMG9DmiG1Q%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>>>>>>> . >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Kentaro Hara, Tokyo, Japan >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Kentaro Hara, Tokyo, Japan >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Kentaro Hara, Tokyo, Japan >>>>>>> >>>>>>> -- >>>>>>> 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-architecture-dev+unsubscr...@chromium.org. >>>>>>> To post to this group, send email to >>>>>>> platform-architecture-...@chromium.org. >>>>>>> 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 >>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jx51JXHEt0H9z%3DzT%3D0GnhDf79QjhGtVxRc%3Dy-RXjHXUyw%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>> . >>>>>>> >>>>>> -- >>>>> 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-architecture-dev+unsubscr...@chromium.org. >>>>> To post to this group, send email to >>>>> platform-architecture-...@chromium.org. >>>>> 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 >>>>> <https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABihn6GKuTUs5UfpbQ6RwXq%3DWw0dDUJkq2JLzadnesfjLG%3DStg%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>> . >>>>> >>>> -- >>>> 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-architecture-dev+unsubscr...@chromium.org. >>>> To post to this group, send email to >>>> platform-architecture-...@chromium.org. >>>> 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 >>>> <https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAEvLGcKKKdZ4-Svz7NmXdzSaL9ryQz4cPEqhTfmWEURzwTdMqQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>> . >>>> >>> -- > 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-architecture-dev+unsubscr...@chromium.org. > To post to this group, send email to > platform-architecture-...@chromium.org. > 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 > <https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAFSTc_iBZUPpYMuUk3ouOe7cLBDizJFUjmcWuaAeVZfMBn4QqQ%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > -- Kentaro Hara, Tokyo, Japan -- -- v8-users mailing list v8-users@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+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
