Cleaning up v8::MaybeLocal handles

[Kentaro Hara]

Hi

TL;DR: I'm considering a way to deprecate v8::MaybeLocal handles from the binding layer.

If a worker thread is forcibly killed by the main thread while running some V8 APIs, the V8 APIs may crash. To address the problem, we introduced v8::MaybeLocal handles 1.5 years ago and tried to rewrite all call sites to handle a case where v8::MaybeLocal handles return empty handles:

v8::Local<v8::Value> result;

if (!v8Call(object->Get(context, result))

  return nullptr;  // Handle a case where the V8 API returns an empty handle.

...

However, the programming pattern has introduced a lot of complexity to the code base as follows:

- Most call sites don't know what to do when the V8 API fails. For example, most call sites assume that object->Get(), ToBoolean(), toV8() etc always succeeds.

- Blink is huge, and it's not realistic to propagate the failure up to all the call sites.

- It's not clear what context Blink should pass in the V8 API.

- It's nasty to write multiple lines just to call one V8 API.

As a result, developers (e.g., DevTools) started violating the programming pattern or using v8CallOrCrash (which just crashes when the V8 API returns an empty v8::MaybeLocal handle). It's hard to say that v8::MaybeLocal handles are working.

So, I'm thinking about a way to deprecate v8::MaybeLocal handles. The main reason we needed the v8::MaybeLocal handles is that workers can be forcibly killed by the main thread. However, nhiroki@ recently removed the force-kill code from common paths of worker shutdown. This dramatically decreased the probability of the worker force-kill. It looks like that we no longer need the v8::MaybeLocal handles (I'm collecting UMAs to measure how rare it happens).

The next step is to remove the v8::MaybeLocal handles from the code base. I think there are two options here:

a) Just revert the changes we did 1.5 years ago. In other words, replace the current v8::MaybeLocal-version APIs with not-v8::MaybeLocal-version APIs.

b) Keep using the v8::MaybeLocal-version APIs, but rewrite all call sites so that Blink immediately crashes when V8 APIs return empty v8::MaybeLocal handles (we can do this using some macro).

I'd prefer a), but if the V8 team wants to keep the v8::MaybeLocal APIs for some reason, I'm okay with b). However, in that case, I'd like to remove the context parameter from the v8::MaybeLocal-version APIs because it's not clear in many cases what context we should use and also the context is not used in most of the V8 APIs (e.g., ToNumber, ToString, Get, Set etc don't need the context).

Any thoughts?

--

Kentaro Hara, Tokyo, Japan

[Jochen Eisinger]

On Mon, Jul 4, 2016 at 7:21 AM Kentaro Hara <har...@chromium.org> wrote:

Hi

TL;DR: I'm considering a way to deprecate v8::MaybeLocal handles from the binding layer.

If a worker thread is forcibly killed by the main thread while running some V8 APIs, the V8 APIs may crash. To address the problem, we introduced v8::MaybeLocal handles 1.5 years ago and tried to rewrite all call sites to handle a case where v8::MaybeLocal handles return empty handles:

v8::Local<v8::Value> result;

if (!v8Call(object->Get(context, result))

  return nullptr;  // Handle a case where the V8 API returns an empty handle.

note that this is the wrong pattern. The place should either have a TryCatch and deal with exceptions, or return an empty MaybeHandle.

 

...

However, the programming pattern has introduced a lot of complexity to the code base as follows:

- Most call sites don't know what to do when the V8 API fails. For example, most call sites assume that object->Get(), ToBoolean(), toV8() etc always succeeds.

That's generally not true, however.

 

- Blink is huge, and it's not realistic to propagate the failure up to all the call sites.

Although that'd be the right thing to do :-/

- It's not clear what context Blink should pass in the V8 API.

That'd be another bug in Blink then. The context is the context you want the exception to be created in.

 

- It's nasty to write multiple lines just to call one V8 API.

As a result, developers (e.g., DevTools) started violating the programming pattern or using v8CallOrCrash (which just crashes when the V8 API returns an empty v8::MaybeLocal handle). It's hard to say that v8::MaybeLocal handles are working.

Devtools should only do that for invoking methods in the debug context, where they know that if the script throws an exception, it's because of a fatal error they can't recover from.

That's not true in the case of blink, where we don't have much control over the script being executed. 

 

So, I'm thinking about a way to deprecate v8::MaybeLocal handles. The main reason we needed the v8::MaybeLocal handles is that workers can be forcibly killed by the main thread. However, nhiroki@ recently removed the force-kill code from common paths of worker shutdown. This dramatically decreased the probability of the worker force-kill. It looks like that we no longer need the v8::MaybeLocal handles (I'm collecting UMAs to measure how rare it happens).

The next step is to remove the v8::MaybeLocal handles from the code base. I think there are two options here:

a) Just revert the changes we did 1.5 years ago. In other words, replace the current v8::MaybeLocal-version APIs with not-v8::MaybeLocal-version APIs. 

b) Keep using the v8::MaybeLocal-version APIs, but rewrite all call sites so that Blink immediately crashes when V8 APIs return empty v8::MaybeLocal handles (we can do this using some macro).

I'd prefer a), but if the V8 team wants to keep the v8::MaybeLocal APIs for some reason, I'm okay with b). However, in that case, I'd like to remove the context parameter from the v8::MaybeLocal-version APIs because it's not clear in many cases what context we should use and also the context is not used in most of the V8 APIs (e.g., ToNumber, ToString, Get, Set etc don't need the context).

Those methods need to know in which context to throw an exception in case of error.

a) is not really an option - other embedders would tar & feather us.

b) is plainly wrong. Blink has to deal with exceptions, and while the terminate execution exception might be gone now, there are still several callsites that don't handle exceptions, leading to random crashes in blink.

What about option c) to propagate MaybeLocal<> throughout blink, so we finally correctly deal with exceptions?

[Kentaro Hara]

On Mon, Jul 4, 2016 at 3:01 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Mon, Jul 4, 2016 at 7:21 AM Kentaro Hara <har...@chromium.org> wrote:

Hi

TL;DR: I'm considering a way to deprecate v8::MaybeLocal handles from the binding layer.

If a worker thread is forcibly killed by the main thread while running some V8 APIs, the V8 APIs may crash. To address the problem, we introduced v8::MaybeLocal handles 1.5 years ago and tried to rewrite all call sites to handle a case where v8::MaybeLocal handles return empty handles:

v8::Local<v8::Value> result;

if (!v8Call(object->Get(context, result))

  return nullptr;  // Handle a case where the V8 API returns an empty handle.

note that this is the wrong pattern. The place should either have a TryCatch and deal with exceptions, or return an empty MaybeHandle.

That's theoretically correct, but the tricky part is 1) we don't want to minimize # of TryCatches because it's heavy but 2) we don't want to propagate the MaybeHandle to core/ and modules/ because it messes up the code base :/ I'm not sure if we want to take that risk for the theoretical benefit.

 

...

However, the programming pattern has introduced a lot of complexity to the code base as follows:

- Most call sites don't know what to do when the V8 API fails. For example, most call sites assume that object->Get(), ToBoolean(), toV8() etc always succeeds.

That's generally not true, however.

 

- Blink is huge, and it's not realistic to propagate the failure up to all the call sites.

Although that'd be the right thing to do :-/

I don't think it's the right thing to do. The 1.5 years of work proved that the programming pattern is not maintainable.

- It's not clear what context Blink should pass in the V8 API.

That'd be another bug in Blink then. The context is the context you want the exception to be created in.

Yes, and yukishiino@ is now writing a document to use correct contexts everywhere in Blink. To that goal, we want to decrease the number of places we have to pass in contexts.

I don't think ToNumber, ToString, Get etc need to take contexts because what they need is the current context, not the relevant context. V8 can get the current context by isolate->current_context(), so the embedder doesn't need to pass in the context, right?

As far as I look at the implementation of ToNumber, it doesn't look like it's using the passed-in context (except that CallDepthScope *re*enters the current context).

 

- It's nasty to write multiple lines just to call one V8 API.

As a result, developers (e.g., DevTools) started violating the programming pattern or using v8CallOrCrash (which just crashes when the V8 API returns an empty v8::MaybeLocal handle). It's hard to say that v8::MaybeLocal handles are working.

Devtools should only do that for invoking methods in the debug context, where they know that if the script throws an exception, it's because of a fatal error they can't recover from.

That's not true in the case of blink, where we don't have much control over the script being executed. 

 

So, I'm thinking about a way to deprecate v8::MaybeLocal handles. The main reason we needed the v8::MaybeLocal handles is that workers can be forcibly killed by the main thread. However, nhiroki@ recently removed the force-kill code from common paths of worker shutdown. This dramatically decreased the probability of the worker force-kill. It looks like that we no longer need the v8::MaybeLocal handles (I'm collecting UMAs to measure how rare it happens).

The next step is to remove the v8::MaybeLocal handles from the code base. I think there are two options here:

a) Just revert the changes we did 1.5 years ago. In other words, replace the current v8::MaybeLocal-version APIs with not-v8::MaybeLocal-version APIs. 

b) Keep using the v8::MaybeLocal-version APIs, but rewrite all call sites so that Blink immediately crashes when V8 APIs return empty v8::MaybeLocal handles (we can do this using some macro).

I'd prefer a), but if the V8 team wants to keep the v8::MaybeLocal APIs for some reason, I'm okay with b). However, in that case, I'd like to remove the context parameter from the v8::MaybeLocal-version APIs because it's not clear in many cases what context we should use and also the context is not used in most of the V8 APIs (e.g., ToNumber, ToString, Get, Set etc don't need the context).

Those methods need to know in which context to throw an exception in case of error.

a) is not really an option - other embedders would tar & feather us.

Would it be an option to keep both versions of APIs?

 

b) is plainly wrong. Blink has to deal with exceptions, and while the terminate execution exception might be gone now, there are still several callsites that don't handle exceptions, leading to random crashes in blink.

I mean by b) that Blink handles exceptions only when Blink wants to handle it using TryCatch. Otherwise let the V8 API crash.

Remember that (unless we perfectly add TryCatch or propagate the v8::MaybeLocal handles to all entry points to bindings,) we're anyway crashing at some places or causing undefined behavior. I don't think that's better than crashing the V8 APIs immediately.

 

What about option c) to propagate MaybeLocal<> throughout blink, so we finally correctly deal with exceptions? 

I don't think this is an option. The 1.5 years of work proved that it's not a good direction to go. After fixing the worker termination issue, I don't think that the theoretical benefit of perfectly propagating the exceptions will outweigh the cost.

 

Any thoughts?

--

Kentaro Hara, Tokyo, Japan

[Jochen Eisinger]

Kentaro Hara <har...@chromium.org> schrieb am Mo., 4. Juli 2016, 08:43:

On Mon, Jul 4, 2016 at 3:01 PM, Jochen Eisinger <joc...@chromium.org> wrote:

On Mon, Jul 4, 2016 at 7:21 AM Kentaro Hara <har...@chromium.org> wrote:

Hi

TL;DR: I'm considering a way to deprecate v8::MaybeLocal handles from the binding layer.

If a worker thread is forcibly killed by the main thread while running some V8 APIs, the V8 APIs may crash. To address the problem, we introduced v8::MaybeLocal handles 1.5 years ago and tried to rewrite all call sites to handle a case where v8::MaybeLocal handles return empty handles:

v8::Local<v8::Value> result;

if (!v8Call(object->Get(context, result))

  return nullptr;  // Handle a case where the V8 API returns an empty handle.

note that this is the wrong pattern. The place should either have a TryCatch and deal with exceptions, or return an empty MaybeHandle.

That's theoretically correct, but the tricky part is 1) we don't want to minimize # of TryCatches because it's heavy but 2) we don't want to propagate the MaybeHandle to core/ and modules/ because it messes up the code base :/ I'm not sure if we want to take that risk for the theoretical benefit.

If we forward exceptions into core, all those methods should deal with an ExceptionState instead of maybehandles

 

...

However, the programming pattern has introduced a lot of complexity to the code base as follows:

- Most call sites don't know what to do when the V8 API fails. For example, most call sites assume that object->Get(), ToBoolean(), toV8() etc always succeeds.

That's generally not true, however.

 

- Blink is huge, and it's not realistic to propagate the failure up to all the call sites.

Although that'd be the right thing to do :-/

I don't think it's the right thing to do. The 1.5 years of work proved that the programming pattern is not maintainable.

We never tried the"right" pattern. A year or so before we did the API change we rewrote v8 internally to use maybe types, and it works just fine

- It's not clear what context Blink should pass in the V8 API.

That'd be another bug in Blink then. The context is the context you want the exception to be created in.

Yes, and yukishiino@ is now writing a document to use correct contexts everywhere in Blink. To that goal, we want to decrease the number of places we have to pass in contexts.

I don't think ToNumber, ToString, Get etc need to take contexts because what they need is the current context, not the relevant context. V8 can get the current context by isolate->current_context(), so the embedder doesn't need to pass in the context, right?

As far as I look at the implementation of ToNumber, it doesn't look like it's using the passed-in context (except that CallDepthScope *re*enters the current context).

 

- It's nasty to write multiple lines just to call one V8 API.

As a result, developers (e.g., DevTools) started violating the programming pattern or using v8CallOrCrash (which just crashes when the V8 API returns an empty v8::MaybeLocal handle). It's hard to say that v8::MaybeLocal handles are working.

Devtools should only do that for invoking methods in the debug context, where they know that if the script throws an exception, it's because of a fatal error they can't recover from.

That's not true in the case of blink, where we don't have much control over the script being executed. 

 

So, I'm thinking about a way to deprecate v8::MaybeLocal handles. The main reason we needed the v8::MaybeLocal handles is that workers can be forcibly killed by the main thread. However, nhiroki@ recently removed the force-kill code from common paths of worker shutdown. This dramatically decreased the probability of the worker force-kill. It looks like that we no longer need the v8::MaybeLocal handles (I'm collecting UMAs to measure how rare it happens).

The next step is to remove the v8::MaybeLocal handles from the code base. I think there are two options here:

a) Just revert the changes we did 1.5 years ago. In other words, replace the current v8::MaybeLocal-version APIs with not-v8::MaybeLocal-version APIs. 

b) Keep using the v8::MaybeLocal-version APIs, but rewrite all call sites so that Blink immediately crashes when V8 APIs return empty v8::MaybeLocal handles (we can do this using some macro).

I'd prefer a), but if the V8 team wants to keep the v8::MaybeLocal APIs for some reason, I'm okay with b). However, in that case, I'd like to remove the context parameter from the v8::MaybeLocal-version APIs because it's not clear in many cases what context we should use and also the context is not used in most of the V8 APIs (e.g., ToNumber, ToString, Get, Set etc don't need the context).

Those methods need to know in which context to throw an exception in case of error.

a) is not really an option - other embedders would tar & feather us.

Would it be an option to keep both versions of APIs?

I would rather not have two versions of all APIs. This also comes with some binary size impact

 

b) is plainly wrong. Blink has to deal with exceptions, and while the terminate execution exception might be gone now, there are still several callsites that don't handle exceptions, leading to random crashes in blink.

I mean by b) that Blink handles exceptions only when Blink wants to handle it using TryCatch. Otherwise let the V8 API crash.

We also have code in blink that converts a maybe handle to an handle inside a trycatch and in case of an exception, rethrows and returns an empty handle. We wouldn't catch this bug when just crashing on the API boundary. Using maybe handles everywhere would make this obvious.

Remember that (unless we perfectly add TryCatch or propagate the v8::MaybeLocal handles to all entry points to bindings,) we're anyway crashing at some places or causing undefined behavior. I don't think that's better than crashing the V8 APIs immediately.

Yes, but why not go for perfect code? Again, we never even tried that

[Kentaro Hara]

In common cases, I think b) will just work. When a V8 API returns an exception, what you want to do would be just to rethrow the exception. That happens by default. Also we already have TryCatch blocks to all common places (V8ScriptRunner, EventListeners etc). So we only need to pay attention when we want to do something uncommon (e.g., Custom elements). I don't think that's common.

 

Remember that (unless we perfectly add TryCatch or propagate the v8::MaybeLocal handles to all entry points to bindings,) we're anyway crashing at some places or causing undefined behavior. I don't think that's better than crashing the V8 APIs immediately.

Yes, but why not go for perfect code? Again, we never even tried that

bashi@ tried that before and stopped the work at the point where we introduced v8::MaybeLocal handles enough to decrease # of worker-termination crashes (it was two months of work as far as I remember) because 1) we couldn't see the end of the work and 2) were not confident that it's a right way to mess up core/ in the same way. That's the state of the current code base :/

Currently when reviewing patches, I'm letting developers use the following pattern (i.e., just suppress the crash) or just use v8CallOrCrash.

v8::Local<v8::Value> result;
if (!v8Call(object->Get(context, result))

  return nullptr;  // Suppress V8 exception.

Hmm :-/

 

 

What about option c) to propagate MaybeLocal<> throughout blink, so we finally correctly deal with exceptions? 

I don't think this is an option. The 1.5 years of work proved that it's not a good direction to go. After fixing the worker termination issue, I don't think that the theoretical benefit of perfectly propagating the exceptions will outweigh the cost.

 

Any thoughts?

--

Kentaro Hara, Tokyo, Japan

--

Kentaro Hara, Tokyo, Japan

[Yutaka Hirano]

> b) is plainly wrong. Blink has to deal with exceptions, and while the terminate execution exception might be gone now, there are still several callsites that don't handle exceptions, leading to random crashes in blink.

Jochen, can you tell me what errors we may have if we are sure that the script itself cannot throw an exception?

--
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/CALjhuif%3Dr0R_TwXZSFBUXT3ETnGn2DO3ttHo%3DPrmUNjdOzVmxA%40mail.gmail.com.

[Kentaro Hara]

Jochen, can you tell me what errors we may have if we are sure that the script itself cannot throw an exception?

I guess it would be only OOM (which we don't really care; we can just crash under OOM).

I guess Jochen's point is that in common cases you can't be sure that a given script never throws. For example, event object->Get can throw if the getter is overridden. ToString, ToNumber etc can potentially throw as well.

[Kentaro Hara]

On Mon, Jul 4, 2016 at 5:57 PM, Kentaro Hara <har...@chromium.org> wrote:

Jochen, can you tell me what errors we may have if we are sure that the script itself cannot throw an exception?

I guess it would be only OOM (which we don't really care; we can just crash under OOM).

I guess Jochen's point is that in common cases you can't be sure that a given script never throws. For example, event object->Get can throw if the getter is overridden. ToString, ToNumber etc can potentially throw as well.

 

s/event object->Get/even object->Get/

[Yutaka Hirano]

Thank you. I'm providing custom bindings for promise.js (implemented in V8) and ReadableStream.js (implemented in Blink) and both are "trusted" scripts. For such scripts I want to use assertions if an operation says it never throws (e.g., creating a promise never throws).

[Yutaka Hirano]

> (e.g., creating a promise never throws)

Correction: creating a promise resolver never throws.

[Jochen Eisinger]

How can you guarantee that? E.g. when you run out of stack trace?

[Yutaka Hirano]

I am under the impression that Blink doesn't care about C++ stack overflow.

[Jochen Eisinger]

Doesn't care meaning it crashes :)

What if promises are entirely monkey-patched? In the case of devtools, the script is running in a different context, but that isn't true in your case, no?

[Yutaka Hirano]

> What if promises are entirely monkey-patched? In the case of devtools, the script is running in a different context, but that isn't true in your case, no?

It's not a problem for me because we use internal functions exposed to V8/Blink (e.g., PromiseThen in promise.js). They are not modifiable by script authors.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CALjhuifNr%2BAd3tno65ea%3DxNwyjuazpky0%2By-FdbDmEwXkoRw%3Dg%40mail.gmail.com.

[Kentaro Hara]

What happens if promise.js is using window.something but the window.something is overridden by user scripts (note that promise.js runs on the same context as the user scripts)?

[Yutaka Hirano]

My understanding is that such trusted scripts should be aware of if it depends on something modifiable by script authors. I'm talking about operations that never throws exceptions no matter how the environment is modified by script authors. For example, the original Promise.prototype.then in the spec which corresponds to PromiseThen in promise.js doesn't throw an exception if called on a proper Promise object no matter how the environment is modified by script authors.

[Kentaro Hara]

If that is the case, you can use v8CallOrCrash (which means that if the script throws, it's a bug we should fix).

[Kentaro Hara]

I discussed this with jochen@. Here is the summary:

- The largest problem of the current situation is that there is no clear guideline for how to handle MaybeLocal handles returned by V8 APIs. Some code is (correctly) propagating the MaybeLocal handles up to the call sites. Some code is just crashing. Some code is just ignoring. They're mixed and inconsistent. The "correct" solution is to propagate the MaybeLocal handles throughout Blink and deal with the exception when the MaybeLocal handles are actually used. However, it's already proven that it's not realistic to fix the code base that way. Not only will it be a year of work but also unacceptably mess up the code base.

- So we propose the following guideline:

0) Use v8Call macros instead of bare MaybeLocal/Maybe handles.

1) If the V8 API is not expected to throw an exception other than OOM or stack-overflow, you can just use v8CallOrCrash. For example, toV8() can use v8CallOrCrash when instantiating a new object with Function::NewInstance, because it throws only under OOM or stack-overflow. This enables us to remove isEmpty() checks from all the call sites of toV8().

2) Keep an eye on the crash rate on v8CallOrCrash. If it's high, it means that you're using v8CallOrCrash where it shouldn't be used.

3) Otherwise, you should check the emptiness of the MaybeLocal handle returned by the V8 API. For example, in common cases you need to check the MaybeLocal handle returned by Object::Get/Set, because the getter/setter may be overridden by any arbitrary user script that may throw.

Any thoughts?

[Yutaka Hirano]

Great! I'm happy with the proposal.

This is off-topic, but v8Call* sound a bit strange to me, as they don't call anything. I was confused when I saw v8Call first time.

[Kentaro Hara]

This is off-topic, but v8Call* sound a bit strange to me, as they don't call anything. I was confused when I saw v8Call first time.

We could remove the v8Call* macros entirely if that's better.

The reason we introduced the macros was that V8 APIs around Maybe/MaybeLocal handles are confusing. Maybe, MaybeLocal, IsEmpty, IsNothing, IsJust, IsMaybe, FromJust, ToLocal, ToLocalChecked...

[Yuki Shiino]

I'm happy, too, with the approach, but I have a little doubt about whether we really need v8Call, etc.

For MaybeLocal<T>, we can use maybeLocal.ToLocalChecked().

For Maybe<T>, we can use maybe.FromJust().

As we're going to crash by default, I think we wouldn't need v8Call family, maybe.

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

[Kentaro Hara]

Yeah. In practice, we can follow the following rule, right?

a) If you want to crash:

- maybeLocal.ToLocalChecked()

- maybe.FromJust()

b) If you want to handle an exception:

- if (maybeLocal.IsEmpty()) { /* handle the exception */ } else { maybeLocal.ToLocal(); }

- if (maybe.IsNothing()) { /* handle the exception */ } else { maybe.FromJust(); }

Then I agree that the v8Call* macros wouldn't be necessary.

========

Also I hope we can add alias methods to V8 APIs so that we can write the above as follows. It would be much easier to understand.

a) If you want to crash:

- maybeLocal.ToLocal()

- maybe.ToValue()

b) If you want to handle an exception:

- if (maybeLocal.IsEmpty()) { /* handle the exception */ } else { maybeLocal.ToLocal(); }

- if (maybe.IsEmpty()) { /* handle the exception */ } else { maybe.ToValue(); }

[Yuki Shiino]

+1 for the former part.

I'm on the fence for renaming ToLocalChecked() to ToLocal().  ToLocalChecked (or ToLocalOrCrash) is easier for me to understand what can happen.

[Kentaro Hara]

I'm on the fence for renaming ToLocalChecked() to ToLocal().  ToLocalChecked (or ToLocalOrCrash) is easier for me to understand what can happen.

My point is that we should use the same method for a) and b). Currently we use ToLocalChecked for a) and ToLocal for b). That way we can easily catch all crashes to be fixed by looking at the crash reports. I don't see any benefit in using different methods for a) and b) except that we can avoid one if branch for b).

[Kentaro Hara]

Does this look slightly better?

a) If you want to crash:

... = maybeLocal.Get()

... = maybe.Get()

b) If you want to handle an exception:

if (maybeLocal.IsEmpty()) { /* handle the exception */ } else { ... = maybeLocal.Get(); }

if (maybe.IsEmpty()) { /* handle the exception */ } else { ... = maybe.Get(); }

This looks simple enough and then we definitely won't need the v8Call* macros. (You can rename Get to GetOrCrash if you want.)

[Yuki Shiino]

I'm fine with your way, but remember that the actual code looks like

    v8::Local<v8::Object> obj = v8::SomeApi(a, b).Get();

In this case, Get() looks a little weird to me.  Especially, in the following case.

    obj = v8::SetProperty(a, b).Get();

SetSomething().Get() is not intuitive for me.

I understand the actual naming is defails, and your plan itself sounds nice to me.

[Jochen Eisinger]

why not use the ToLocal() method instead of calling IsEmpty()?

Local<Value> result;

if (!SomeApi().ToLocal(&result)) return MaybeLocal<Value>(); // caller has to deal with exception

// Use result

[Kentaro Hara]

On Wed, Jul 27, 2016 at 10:15 AM, Jochen Eisinger <joc...@chromium.org> wrote:

why not use the ToLocal() method instead of calling IsEmpty()?

Local<Value> result;

if (!SomeApi().ToLocal(&result)) return MaybeLocal<Value>(); // caller has to deal with exception

// Use result

Ah, thanks. That sounds nicer.

a) If you want to crash:

maybeLocalAPI().GetResultOrCrash(&result);

maybeAPI().GetResultOrCrash(&result);

b) If you want to handle an exception:

if (!maybeLocalAPI().GetResult(&result)) { /* handle the exception */ }

if (!maybeAPI().GetResult(&result)) { /* handle the exception */ }

?

[Jochen Eisinger]

that looks better. Note, however, that I think it's not viable to change ToLocalChecked/ToLocal to GetResultOrCrash/GetResult without the node community being really really unhappy, so I'd rather stick with the existing names.

[Kentaro Hara]

Would it be an option to add alias methods (GetResultOrCrash/GetResult) instead of renaming?

[Jochen Eisinger]

Sorry for the late reply.

How would we avoid having a mix of the two in the codebase?

[Kentaro Hara]

How would we avoid having a mix of the two in the codebase?

We should just clean up the codebase? (just like we removed the mixture of .As and .Cast)

Note that if we don't add GetResult, we need to introduce FromJustUnchecked().

a) If you want to crash:

maybeLocalAPI().ToLocalChecked(&result);

maybeAPI().FromJust(&result);

b) If you want to handle an exception:

if (!maybeLocalAPI().ToLocal(&result)) { /* handle the exception */ }

if (!maybeAPI().FromJustUnchecked(&result)) { /* handle the exception */ }   <---- we need this one.

Rather than introducing yet another confusing API, I think it would be better to introduce GetResult/GetResultOrCrash consistently. What do you think?

[Jochen Eisinger]

I'd call it .To(): MaybeLocal.ToLocal / Maybe.To

[Kentaro Hara]

So your proposal is like this?

a) If you want to crash:

maybeLocalAPI().ToLocalChecked(&result);

maybeAPI().ToChecked(&result);   <---- Introduce this one. This is an alias of FromJust.

b) If you want to handle an exception:

if (!maybeLocalAPI().ToLocal(&result)) { /* handle the exception */ }

if (!maybeAPI().To(&result)) { /* handle the exception */ }   <---- Introduce this one.

I'm fine with it although it would be slightly nicer if we can use the same name for ToLocal/To.

[Jochen Eisinger]

sgtm

[Kentaro Hara]

I landed the To API (https://codereview.chromium.org/2194793003/).

The conclusion is that we want to use the following pattern throughout the code base (at least in Blink). We no longer need v8Call macros.

a) If you want to crash:

Local<T> result = maybeLocalAPI().ToLocalChecked();

T result = maybeAPI().ToChecked();

b) If you want to handle an exception:

Local<T> result;

if (!maybeLocalAPI().ToLocal(&result)) { /* handle the exception */ }

T result;

if (!maybeAPI().To(&result)) { /* handle the exception */ }