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