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