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> >>> . >>> >> -- -- 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.
