Issue 1100223003: bindings: Add empty checks for toV8()

bashi

Patchset #1 (id:1) has been deleted

5 years, 8 months ago (2015-04-24 04:49:42 UTC) #1

bashi

bashi@chromium.org changed reviewers: + haraken@chromium.org, yukishiino@chromium.org

5 years, 8 months ago (2015-04-28 04:06:02 UTC) #2

haraken

https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8/ScriptController.cpp File Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8/ScriptController.cpp#newcode394 Source/bindings/core/v8/ScriptController.cpp:394: if (v8plugin.IsEmpty() || !v8plugin->IsObject()) I think we can remove ...

5 years, 7 months ago (2015-04-28 04:25:00 UTC) #4

Yuki

https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8/ScriptController.cpp File Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8/ScriptController.cpp#newcode394 Source/bindings/core/v8/ScriptController.cpp:394: if (v8plugin.IsEmpty() || !v8plugin->IsObject()) On 2015/04/28 04:24:59, haraken wrote: ...

5 years, 7 months ago (2015-04-28 04:47:57 UTC) #5

https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:394: if (v8plugin.IsEmpty() ||
!v8plugin->IsObject())
On 2015/04/28 04:24:59, haraken wrote:
> 
> I think we can remove the v8plugin->IsObject() check.

For the code reader, it's not clear that the following call to
v8::Local<v8::Object>::Cast<v8plugin) is safe without IsObject() check.

We knew the implementation details, but those who are not familiar with details
wouldn't think it's clear.  I'd +1 to keep IsObject() check.

https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8CustomElementLifecycleCallbacks.cpp (right):

https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8CustomElementLifecycleCallbacks.cpp:160: return;
On 2015/04/28 04:24:59, haraken wrote:
> 
> Not related to your CL, this code is doing something strange. A common patter
> would be:
> 
>   receiver = DOMDataStore::getWrapper();
>   if (receiver.isEmpty())
>     receiver = wrap();
>   ...

and it should be done inside toV8().

I agree that the current code is faster than a simple call to toV8 because it
already knew the |world| and there is no need to look up the |world|, though.

https://codereview.chromium.org/1100223003/diff/20001/Source/core/dom/DOMData...
File Source/core/dom/DOMDataView.cpp (right):

https://codereview.chromium.org/1100223003/diff/20001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:33: return v8::Handle<v8::Object>();
On 2015/04/28 04:24:59, haraken wrote:
> 
> Remove.

I think it's good to check that v8Buffer is not v8::Object or anything else but
just v8::ArrayBuffer.  And this ASSERT doesn't affect the performance in release
mode, I'd prefer keeping this sort of checks.

bashi

https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8/ScriptController.cpp File Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/1100223003/diff/20001/Source/bindings/core/v8/ScriptController.cpp#newcode394 Source/bindings/core/v8/ScriptController.cpp:394: if (v8plugin.IsEmpty() || !v8plugin->IsObject()) On 2015/04/28 04:47:57, Yuki wrote: ...

5 years, 7 months ago (2015-04-28 05:41:53 UTC) #7

haraken

LGTM except the ASSERT in the inspector-related code. https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp#newcode466 Source/web/WebPluginContainerImpl.cpp:466: ASSERT(!v8value.IsEmpty() ...

5 years, 7 months ago (2015-04-28 05:50:34 UTC) #8

bashi

https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp#newcode466 Source/web/WebPluginContainerImpl.cpp:466: ASSERT(!v8value.IsEmpty() && v8value->IsObject()); On 2015/04/28 05:50:34, haraken wrote: > ...

5 years, 7 months ago (2015-04-28 06:41:15 UTC) #10

Yuki

https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp#newcode466 Source/web/WebPluginContainerImpl.cpp:466: ASSERT(!v8value.IsEmpty() && v8value->IsObject()); On 2015/04/28 06:41:14, bashi1 wrote: > ...

5 years, 7 months ago (2015-04-28 06:43:46 UTC) #11

haraken

https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp#newcode466 Source/web/WebPluginContainerImpl.cpp:466: ASSERT(!v8value.IsEmpty() && v8value->IsObject()); On 2015/04/28 06:43:46, Yuki wrote: > ...

5 years, 7 months ago (2015-04-28 06:46:08 UTC) #12

bashi

https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp#newcode466 Source/web/WebPluginContainerImpl.cpp:466: ASSERT(!v8value.IsEmpty() && v8value->IsObject()); On 2015/04/28 06:46:08, haraken wrote: > ...

5 years, 7 months ago (2015-04-28 06:55:50 UTC) #13

haraken

On 2015/04/28 06:55:50, bashi1 wrote: > https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp > File Source/web/WebPluginContainerImpl.cpp (right): > > https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginContainerImpl.cpp#newcode466 > ...

5 years, 7 months ago (2015-04-28 06:57:42 UTC) #14

On 2015/04/28 06:55:50, bashi1 wrote:
>
https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginCon...
> File Source/web/WebPluginContainerImpl.cpp (right):
> 
>
https://codereview.chromium.org/1100223003/diff/20001/Source/web/WebPluginCon...
> Source/web/WebPluginContainerImpl.cpp:466: ASSERT(!v8value.IsEmpty() &&
> v8value->IsObject());
> On 2015/04/28 06:46:08, haraken wrote:
> > On 2015/04/28 06:43:46, Yuki wrote:
> > > On 2015/04/28 06:41:14, bashi1 wrote:
> > > > On 2015/04/28 05:50:34, haraken wrote:
> > > > > On 2015/04/28 05:41:53, bashi1 wrote:
> > > > > > On 2015/04/28 04:24:59, haraken wrote:
> > > > > > > 
> > > > > > > Who guarantees it is not empty?
> > > > > > > 
> > > > > > > Also we can remove the IsObject check.
> > > > > > 
> > > > > > The existing ASSERT() implies this should not be empty, maybe?
> > > > > 
> > > > > I think that is exactly what we're fixing with introducing the Maybe
> > > version.
> > > > We
> > > > > need to assume that any V8 API that returns a Maybe can return an
empty
> > > > handle.
> > > > 
> > > > Yeah, let's use !IsEmpty() assertion to see this assumption is correct.
> I'll
> > > do
> > > > immediate revert if this is a problem.
> > > 
> > > The problem is, ASSERT will be noop in release build, I think.
> > > How can we detect the problem in the real world?
> > 
> > If you argue that we can go with ASSERT here, that means we can go with
> ASSERTs
> > in many other cases. (And the ASSERTs will rarely hit.)
> > 
> > IMHO, we should avoid using ASSERTs (or ToLocalChecked) unless we're pretty
> sure
> > that the API should not return an empty handle. Also we should limit the
> places.
> 
> Ok, changed to return empty handle.
> 
> That's said, I'd prefer using ASSERTs or immediate crashes if possible because
> returning empty handles spreads out the need for empty checks. That's what I'm
> suffering from now :(

I understand your concern, and that's the concern of this project overall...

(In this particular case, the function is already returning an empty handle, so
it won't be a big issue, will it?)

yurys

https://codereview.chromium.org/1100223003/diff/80001/Source/core/dom/DOMDataView.cpp File Source/core/dom/DOMDataView.cpp (right): https://codereview.chromium.org/1100223003/diff/80001/Source/core/dom/DOMDataView.cpp#newcode33 Source/core/dom/DOMDataView.cpp:33: return v8::Handle<v8::Object>(); AFAIU this is where v8::MaybeLocal should be ...

5 years, 7 months ago (2015-04-28 07:05:25 UTC) #15

bashi

https://codereview.chromium.org/1100223003/diff/80001/Source/core/dom/DOMDataView.cpp File Source/core/dom/DOMDataView.cpp (right): https://codereview.chromium.org/1100223003/diff/80001/Source/core/dom/DOMDataView.cpp#newcode33 Source/core/dom/DOMDataView.cpp:33: return v8::Handle<v8::Object>(); On 2015/04/28 07:05:25, yurys wrote: > AFAIU ...

5 years, 7 months ago (2015-04-28 07:09:45 UTC) #16

yurys

On 2015/04/28 07:09:45, bashi1 wrote: > We use MaybeLocal if we can guarantee that the ...

5 years, 7 months ago (2015-04-28 07:41:22 UTC) #17

bashi

On 2015/04/28 07:41:22, yurys wrote: > What's the fundamental difference between exception being thrown and ...

5 years, 7 months ago (2015-04-28 08:20:27 UTC) #18

On 2015/04/28 07:41:22, yurys wrote:
> What's the fundamental difference between exception being thrown and error
state
> in C++ that leads to returning an empty handle?
If the condition is satisfied, We don't need to have TryCatch to detect
exceptions. Also, if we allow a Maybe API return empty handle without exception,
it requires tryCatch.HadException() checks on every failure case.
> I expected that with introduction of MaybeLocal we should be able to limit
> number of places where we have to check for handle emptiness to those where
> MaybeLocal is converted to Local. On the other hand I see that the number of
> places where empty handles are created in C++ increases which means that we
will
> now have to deal with two error states: empty Local handle and empty
MaybeLocal.
> So it is still hard to reason about the code since looking at the usages of
> Local one cannot tell for sure if the handle may be null or not (the review
> comments above prove this). Also on another thread Jochen said that the plan
was
> to eventually remove v8 API methods that can return empty handles
> (https://codereview.chromium.org/1036803002/#msg21) 
> 
> Did you consider using MaybeLocal in all places including C++ code where
> function may fail and return an empty handle?

Yeah, you are right, and we discussed a lot where we should draw the line. It's
not obvious given that:
- there are a lot of functions which could return empty handles
- it requires many engineering resources to replace return types to Maybe, and
it would not be worth doing just for worker exception handling (we are
constantly told that we spend much time for relatively not-so-important tasks)
- if we allow to use Maybe without exceptions, it would introduce another
problem described above

I'd appreciate if you have a great solution for that.

> and removing empty handle
> constructor from the API would be a big win.

I can't find the source of this consensus. Could you point out?

yurys

On 2015/04/28 08:20:27, bashi1 wrote: > On 2015/04/28 07:41:22, yurys wrote: > > What's the ...

5 years, 7 months ago (2015-04-28 09:22:47 UTC) #19

On 2015/04/28 08:20:27, bashi1 wrote:
> On 2015/04/28 07:41:22, yurys wrote:
> > What's the fundamental difference between exception being thrown and error
> state
> > in C++ that leads to returning an empty handle?
> If the condition is satisfied, We don't need to have TryCatch to detect
> exceptions. Also, if we allow a Maybe API return empty handle without
exception,
> it requires tryCatch.HadException() checks on every failure case.

TryCatch allows you to catch thrown exception. You have to use it only when you
need to deal with the exception object or want to stop the exception
propagation. In both cases simply checking MaybeLocal for emptiness is not
enough and you still have to use TryCatch. On the other hand if you don't want
to deal with exception there is no difference between error in C++ and exception
in JS both of which can return empty MaybeLocal so I don't see why it should
require more TryCatch handlers in the code.

> > I expected that with introduction of MaybeLocal we should be able to limit
> > number of places where we have to check for handle emptiness to those where
> > MaybeLocal is converted to Local. On the other hand I see that the number of
> > places where empty handles are created in C++ increases which means that we
> will
> > now have to deal with two error states: empty Local handle and empty
> MaybeLocal.
> > So it is still hard to reason about the code since looking at the usages of
> > Local one cannot tell for sure if the handle may be null or not (the review
> > comments above prove this). Also on another thread Jochen said that the plan
> was
> > to eventually remove v8 API methods that can return empty handles
> > (https://codereview.chromium.org/1036803002/#msg21) 
> > 
> > Did you consider using MaybeLocal in all places including C++ code where
> > function may fail and return an empty handle?
> 
> Yeah, you are right, and we discussed a lot where we should draw the line.

Could you provide link to a thread where this was discussed or probably a design
doc with explanations of why you preferred one approach over another?

> It's not obvious given that:
> - there are a lot of functions which could return empty handles
> - it requires many engineering resources to replace return types to Maybe, and
> it would not be worth doing just for worker exception handling (we are
> constantly told that we spend much time for relatively not-so-important tasks)

As discussed on blink-dev the problem is not limited to workers because
TerminateExecution may be called on the main thread too (in Android WebView).
The work you are doing potentially affects all places that have to deal with JS
and requires a lot of engineering effort. Having all potentially empty handles
turned into MaybeLocal would be a clear final goal to aim at and I can see its
merits. What is the current definition of the boundary where the empty handles
should be dealt with? What is the final state that we want to achieve with the
effort, I mean how would you determine that all dangerous uses of empty handles
have been fixed?

> - if we allow to use Maybe without exceptions, it would introduce another
> problem described above
> 
> I'd appreciate if you have a great solution for that.
> 

Sorry, I don't quite follow you here, which problem are you referring to?

> > and removing empty handle
> > constructor from the API would be a big win.
> 
> I can't find the source of this consensus. Could you point out?
https://codereview.chromium.org/1036803002/#msg21

Constructor of empty Local() is one of the main sources of empty handles.

bashi

On 2015/04/28 09:22:47, yurys wrote: > TryCatch allows you to catch thrown exception. You have ...

5 years, 7 months ago (2015-05-01 02:11:32 UTC) #20

On 2015/04/28 09:22:47, yurys wrote:
> TryCatch allows you to catch thrown exception. You have to use it only when
you
> need to deal with the exception object or want to stop the exception
> propagation. In both cases simply checking MaybeLocal for emptiness is not
> enough and you still have to use TryCatch. On the other hand if you don't want
> to deal with exception there is no difference between error in C++ and
exception
> in JS both of which can return empty MaybeLocal so I don't see why it should
> require more TryCatch handlers in the code.

Right. But it is inconsistent with V8's behavior. We treat V8 Maybe handles as
something closely related to exceptions, while you think Maybe is just a wrapper
of handles. Your view is totally reasonable, but in bindings we need to
determine when we use Maybe (see below).

> > Yeah, you are right, and we discussed a lot where we should draw the line.
> 
> Could you provide link to a thread where this was discussed or probably a
design
> doc with explanations of why you preferred one approach over another?

We discussed offline. I wrote a document[1] but it doesn't have discussion
details. What we have discussed are basically:
- Should we use Maybe everywhere? -> In theory, yes. Not sure in practice. We
can't guarantee that there is no crash due to empty handles unless we use Maybe
everywhere, but we have a lot of functions and callsites which use V8 handles.
As a first step, let's replace V8 API with Maybe version and propagate Maybe if
necessary.
- When we should use Maybe? -> Follow V8 behavior. Use them when we can ensures
that there is an exception on failure.
- But the motivation of this work is to improve better exception handling. We
can't accomplish it when we keep using empty Local handles, no? -> True, but
using Maybe everywhere requires a lot of work. Let's complete API replacement
first. We can do Local -> Maybe conversion as "v2" work if we want.

[1]
https://docs.google.com/document/d/1S1QH0bZGry90wfD11ehBw-a9bgeC2jz_FX513SRVf...

> > I'd appreciate if you have a great solution for that.
> > 
> 
> Sorry, I don't quite follow you here, which problem are you referring to?

Sorry for being unclear. In short, what your suggestion about this work overall?
Use Maybe everywhere? Do you think it is worth doing?

> > I can't find the source of this consensus. Could you point out?
> https://codereview.chromium.org/1036803002/#msg21
> 
> Constructor of empty Local() is one of the main sources of empty handles.

It seems jochen@ didn't reply your last comment. Am I missing something?

yurys

On 2015/05/01 02:11:32, bashi1(ooo til May 12) wrote: > On 2015/04/28 09:22:47, yurys wrote: > ...

5 years, 7 months ago (2015-05-06 11:16:59 UTC) #21

On 2015/05/01 02:11:32, bashi1(ooo til May 12) wrote:
> On 2015/04/28 09:22:47, yurys wrote:
> > TryCatch allows you to catch thrown exception. You have to use it only when
> you
> > need to deal with the exception object or want to stop the exception
> > propagation. In both cases simply checking MaybeLocal for emptiness is not
> > enough and you still have to use TryCatch. On the other hand if you don't
want
> > to deal with exception there is no difference between error in C++ and
> exception
> > in JS both of which can return empty MaybeLocal so I don't see why it should
> > require more TryCatch handlers in the code.
> 
> Right. But it is inconsistent with V8's behavior. We treat V8 Maybe handles as
> something closely related to exceptions, while you think Maybe is just a
wrapper
> of handles. Your view is totally reasonable, but in bindings we need to
> determine when we use Maybe (see below).
> 
> > > Yeah, you are right, and we discussed a lot where we should draw the line.
> > 
> > Could you provide link to a thread where this was discussed or probably a
> design
> > doc with explanations of why you preferred one approach over another?
> 
> We discussed offline. I wrote a document[1] but it doesn't have discussion
> details. What we have discussed are basically:
> - Should we use Maybe everywhere? -> In theory, yes. Not sure in practice. We
> can't guarantee that there is no crash due to empty handles unless we use
Maybe
> everywhere, but we have a lot of functions and callsites which use V8 handles.
> As a first step, let's replace V8 API with Maybe version and propagate Maybe
if
> necessary.
> - When we should use Maybe? -> Follow V8 behavior. Use them when we can
ensures
> that there is an exception on failure.
This is already false as far as I can see, in many places in <v8>/src/api.cc
MaybeLocal<Value>() is returned without throwing an exception (see e.g.
v8::Object::GetRealNamedPropertyInPrototypeChain), am I missing something?

> - But the motivation of this work is to improve better exception handling.
OK, I assumed that the goal was to reduce number of crashes due to empty handles
accesses which of cause in many cases are side effects of JS exceptions but not
always.

> We can't accomplish it when we keep using empty Local handles, no? -> True,
but
> using Maybe everywhere requires a lot of work. Let's complete API replacement
> first. We can do Local -> Maybe conversion as "v2" work if we want.
> 
> [1]
>
https://docs.google.com/document/d/1S1QH0bZGry90wfD11ehBw-a9bgeC2jz_FX513SRVf...
> 
> > > I'd appreciate if you have a great solution for that.
> > > 
> > 
> > Sorry, I don't quite follow you here, which problem are you referring to?
> 
> Sorry for being unclear. In short, what your suggestion about this work
overall?
> Use Maybe everywhere?
Yes. Use it to indicate that a handle can be empty and requires a check before
dereferencing. 

> Do you think it is worth doing?
> 
I don't know. My point is that otherwise we could simply add Local::IsEmpty
checks in the places where they are missing. I don't see why using Maybe would
be fundamentally better.

> > > I can't find the source of this consensus. Could you point out?
> > https://codereview.chromium.org/1036803002/#msg21
> > 
> > Constructor of empty Local() is one of the main sources of empty handles.
> 
> It seems jochen@ didn't reply your last comment. Am I missing something?

bashi

On 2015/05/06 11:16:59, yurys wrote: > On 2015/05/01 02:11:32, bashi1(ooo til May 12) wrote: > ...

5 years, 7 months ago (2015-05-08 03:18:45 UTC) #22

On 2015/05/06 11:16:59, yurys wrote:
> On 2015/05/01 02:11:32, bashi1(ooo til May 12) wrote:
> > On 2015/04/28 09:22:47, yurys wrote:
> > > TryCatch allows you to catch thrown exception. You have to use it only
when
> > you
> > > need to deal with the exception object or want to stop the exception
> > > propagation. In both cases simply checking MaybeLocal for emptiness is not
> > > enough and you still have to use TryCatch. On the other hand if you don't
> want
> > > to deal with exception there is no difference between error in C++ and
> > exception
> > > in JS both of which can return empty MaybeLocal so I don't see why it
should
> > > require more TryCatch handlers in the code.
> > 
> > Right. But it is inconsistent with V8's behavior. We treat V8 Maybe handles
as
> > something closely related to exceptions, while you think Maybe is just a
> wrapper
> > of handles. Your view is totally reasonable, but in bindings we need to
> > determine when we use Maybe (see below).
> > 
> > > > Yeah, you are right, and we discussed a lot where we should draw the
line.
> > > 
> > > Could you provide link to a thread where this was discussed or probably a
> > design
> > > doc with explanations of why you preferred one approach over another?
> > 
> > We discussed offline. I wrote a document[1] but it doesn't have discussion
> > details. What we have discussed are basically:
> > - Should we use Maybe everywhere? -> In theory, yes. Not sure in practice.
We
> > can't guarantee that there is no crash due to empty handles unless we use
> Maybe
> > everywhere, but we have a lot of functions and callsites which use V8
handles.
> > As a first step, let's replace V8 API with Maybe version and propagate Maybe
> if
> > necessary.
> > - When we should use Maybe? -> Follow V8 behavior. Use them when we can
> ensures
> > that there is an exception on failure.
> This is already false as far as I can see, in many places in <v8>/src/api.cc
> MaybeLocal<Value>() is returned without throwing an exception (see e.g.
> v8::Object::GetRealNamedPropertyInPrototypeChain), am I missing something?
There are few exceptions and dcarney@ was working on removing such exceptions,
but I'm not sure the current plan as he left the project.
https://codereview.chromium.org/1003043002/#msg21

> 
> > - But the motivation of this work is to improve better exception handling.
> OK, I assumed that the goal was to reduce number of crashes due to empty
handles
> accesses which of cause in many cases are side effects of JS exceptions but
not
> always.
Yes.
> 
> > We can't accomplish it when we keep using empty Local handles, no? -> True,
> but
> > using Maybe everywhere requires a lot of work. Let's complete API
replacement
> > first. We can do Local -> Maybe conversion as "v2" work if we want.
> > 
> > [1]
> >
>
https://docs.google.com/document/d/1S1QH0bZGry90wfD11ehBw-a9bgeC2jz_FX513SRVf...
> > 
> > > > I'd appreciate if you have a great solution for that.
> > > > 
> > > 
> > > Sorry, I don't quite follow you here, which problem are you referring to?
> > 
> > Sorry for being unclear. In short, what your suggestion about this work
> overall?
> > Use Maybe everywhere?
> Yes. Use it to indicate that a handle can be empty and requires a check before
> dereferencing. 
> 
> > Do you think it is worth doing?
> > 
> I don't know. My point is that otherwise we could simply add Local::IsEmpty
> checks in the places where they are missing. I don't see why using Maybe would
> be fundamentally better.
Yeah, and I'll add IsEmpty() checks as a short-term solution. We can revisit
this when we observe significant crash rate again.
> 
> > > > I can't find the source of this consensus. Could you point out?
> > > https://codereview.chromium.org/1036803002/#msg21
> > > 
> > > Constructor of empty Local() is one of the main sources of empty handles.
> > 
> > It seems jochen@ didn't reply your last comment. Am I missing something?

bashi

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org Link to ...

5 years, 7 months ago (2015-05-12 02:14:25 UTC) #25

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1100223003/120001

5 years, 7 months ago (2015-05-12 02:14:49 UTC) #26

commit-bot: I haz the power

Committed patchset #6 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195221

5 years, 7 months ago (2015-05-12 03:38:23 UTC) #27

yurys

On 2015/05/08 03:18:45, bashi1 wrote: > > This is already false as far as I ...

5 years, 7 months ago (2015-05-20 00:48:44 UTC) #28

bashi

5 years, 7 months ago (2015-05-20 00:55:25 UTC) #29

Message was sent while issue was closed.

On 2015/05/20 00:48:44, yurys wrote:
> On 2015/05/08 03:18:45, bashi1 wrote:
> > > This is already false as far as I can see, in many places in
<v8>/src/api.cc
> > > MaybeLocal<Value>() is returned without throwing an exception (see e.g.
> > > v8::Object::GetRealNamedPropertyInPrototypeChain), am I missing something?
> > There are few exceptions and dcarney@ was working on removing such
exceptions,
> > but I'm not sure the current plan as he left the project.
> > https://codereview.chromium.org/1003043002/#msg21
> > 
> 
> It would make sense to fix those to avoid further confusion. I wonder if
someone
> is going to take over this effort?

+jochen@

I'm also curious on this. jochen@, is anyone working on this?

Issue 1100223003: bindings: Add empty checks for toV8() (Closed)

Description

Patch Set 1 : #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : rebase #

Messages