Re: binidngs: Make callInternalFunction return MaybeLocal (issue 1036803002 by bashi@chromium.org)
0 views
Skip to first unread message
yu...@chromium.org
Mar 30, 2015, 6:58:01 AM3/30/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
On 2015/03/27 13:24:15, jochen wrote:
> On 2015/03/27 at 13:20:33, yurys wrote:
> > On 2015/03/27 13:16:44, jochen wrote:
> > > once all callsites are updated, we'll remove v8 API methods that can
return
> > > empty handles.
> >
> > Does it mean that MaybeLocal::ToLocal will be eventually removed in
> favor of
> MaybeLocal::IsEmpty/ToLocalChecked?
> ToLocal() is just a shortcut that allows you to skip the CHECK so it's
> faster.
> You're supposed to use it like this
> if (!maybe_foo.ToLocal(&foo))
> return whatever;
> so in the (likely) case that maybe_foo is not empty, we don't have to do
> the
> CHECK() which ToLocalChecked always does. Given how performance sensitive
> bindings are, I don't think we want to remove it.
Out of curiosity, is there a benchmark showing what the actual slow down is
in
this case? I wouldn't expect it too be big as we already check for null
value in
ToLocal.
https://codereview.chromium.org/1036803002/
> On 2015/03/27 at 13:20:33, yurys wrote:
> > On 2015/03/27 13:16:44, jochen wrote:
> > > once all callsites are updated, we'll remove v8 API methods that can
return
> > > empty handles.
> >
> > Does it mean that MaybeLocal::ToLocal will be eventually removed in
> favor of
> MaybeLocal::IsEmpty/ToLocalChecked?
> ToLocal() is just a shortcut that allows you to skip the CHECK so it's
> faster.
> You're supposed to use it like this
> if (!maybe_foo.ToLocal(&foo))
> return whatever;
> so in the (likely) case that maybe_foo is not empty, we don't have to do
> the
> CHECK() which ToLocalChecked always does. Given how performance sensitive
> bindings are, I don't think we want to remove it.
Out of curiosity, is there a benchmark showing what the actual slow down is
in
this case? I wouldn't expect it too be big as we already check for null
value in
ToLocal.
https://codereview.chromium.org/1036803002/
ba...@chromium.org
Mar 31, 2015, 1:46:08 AM3/31/15
to joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
Updated. PTAL?
In summary,
- I want to make V8ScriptRunner::callInternalFunction() return MaybeLocal
because we use V8ScriptRunner in workers.
- Minimized changes in inspector code given that we are still discussing
how we
should use v8 APIs in inspector.
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaScriptCallFrame.cpp
File Source/core/inspector/JavaScriptCallFrame.cpp (right):
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaScriptCallFrame.cpp#newcode84
Source/core/inspector/JavaScriptCallFrame.cpp:84: v8::Local<v8::Value>
result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0,
m_isolate).ToLocalChecked();
On 2015/03/27 12:07:24, yurys wrote:
> On 2015/03/27 00:39:50, bashi1 wrote:
> > On 2015/03/25 12:05:22, haraken wrote:
> > >
> > > Ditto. I guess this callInternalFunction can return an empty
handle.
> >
> > Yes. If we have better option than crash (e.g. return an empty
string), it
> would
> > be great. yurys@, WDYT?
> @aanrey would know better if his place is allowed to throw when called
e.g. from
> JavaScriptCallFrame::stepInPositions
Changed to return String() since this function seems to have the same
semantics of callV8FunctionReturnInt, which returns 0 on failure.
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaScriptCallFrame.cpp#newcode209
Source/core/inspector/JavaScriptCallFrame.cpp:209: return
ScriptValue(scriptState,
V8ScriptRunner::callInternalFunction(setVariableValueFunction,
callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate).ToLocalChecked());
On 2015/03/27 12:07:24, yurys wrote:
> In this case we may well fail to change local variable and the
exception should
> be propagated to the calling code (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/inspector/InjectedScriptSource.js&q=%5C.setVariableValue%5C(%20f:webkit&sq=package:chromium&type=cs&l=1008).
> So empty handle is valid here.
> FYI: Using ScriptValue in this class (as well as in many other places
in
> inspector code) just hampers reading this code. When it was introduced
we had to
> work both with V8 and JSC and it made perfect sense then. In Blink we
use single
> VM and there are no plans for adding another so using Handles directly
would be
> preferable. I'll try to update this code as it will be necessary for
extracting
> it anyways.
Thanks. Replaced with ToLocal(). In blink, we decided to propagate
MaybeLocal when a method returns a result of v8 Maybe API directly (so
V8ScriptRunner::callInternalFunction returns Maybe). It would make sense
to have this method return MaybeLocal as well, but I didn't do that as
inspector code would have a different policy.
https://codereview.chromium.org/1036803002/
In summary,
- I want to make V8ScriptRunner::callInternalFunction() return MaybeLocal
because we use V8ScriptRunner in workers.
- Minimized changes in inspector code given that we are still discussing
how we
should use v8 APIs in inspector.
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaScriptCallFrame.cpp
File Source/core/inspector/JavaScriptCallFrame.cpp (right):
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaScriptCallFrame.cpp#newcode84
Source/core/inspector/JavaScriptCallFrame.cpp:84: v8::Local<v8::Value>
result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0,
m_isolate).ToLocalChecked();
On 2015/03/27 12:07:24, yurys wrote:
> On 2015/03/27 00:39:50, bashi1 wrote:
> > On 2015/03/25 12:05:22, haraken wrote:
> > >
> > > Ditto. I guess this callInternalFunction can return an empty
handle.
> >
> > Yes. If we have better option than crash (e.g. return an empty
string), it
> would
> > be great. yurys@, WDYT?
> @aanrey would know better if his place is allowed to throw when called
e.g. from
> JavaScriptCallFrame::stepInPositions
Changed to return String() since this function seems to have the same
semantics of callV8FunctionReturnInt, which returns 0 on failure.
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaScriptCallFrame.cpp#newcode209
Source/core/inspector/JavaScriptCallFrame.cpp:209: return
ScriptValue(scriptState,
V8ScriptRunner::callInternalFunction(setVariableValueFunction,
callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate).ToLocalChecked());
On 2015/03/27 12:07:24, yurys wrote:
> In this case we may well fail to change local variable and the
exception should
> be propagated to the calling code (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/inspector/InjectedScriptSource.js&q=%5C.setVariableValue%5C(%20f:webkit&sq=package:chromium&type=cs&l=1008).
> So empty handle is valid here.
> FYI: Using ScriptValue in this class (as well as in many other places
in
> inspector code) just hampers reading this code. When it was introduced
we had to
> work both with V8 and JSC and it made perfect sense then. In Blink we
use single
> VM and there are no plans for adding another so using Handles directly
would be
> preferable. I'll try to update this code as it will be necessary for
extracting
> it anyways.
Thanks. Replaced with ToLocal(). In blink, we decided to propagate
MaybeLocal when a method returns a result of v8 Maybe API directly (so
V8ScriptRunner::callInternalFunction returns Maybe). It would make sense
to have this method return MaybeLocal as well, but I didn't do that as
inspector code would have a different policy.
https://codereview.chromium.org/1036803002/
yu...@chromium.org
Mar 31, 2015, 3:20:20 AM3/31/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
https://codereview.chromium.org/1036803002/diff/60001/Source/bindings/core/v8/V8PerIsolateData.cpp
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):
https://codereview.chromium.org/1036803002/diff/60001/Source/bindings/core/v8/V8PerIsolateData.cpp#newcode246
Source/bindings/core/v8/V8PerIsolateData.cpp:246: if
(!V8ScriptRunner::callInternalFunction(v8::Handle<v8::Function>::Cast(value),
info.This(), 0, 0, info.GetIsolate()).ToLocal(&result))
If we fail to get a valid result there must be a pending exception which
the caller will have to handle. So I wonder what's the reason for
bothering to set any value at all? (I understand that it was written
this way before your change but since you are here it may be a good time
to improve this).
https://codereview.chromium.org/1036803002/diff/60001/Source/core/inspector/JavaScriptCallFrame.cpp
File Source/core/inspector/JavaScriptCallFrame.cpp (right):
https://codereview.chromium.org/1036803002/diff/60001/Source/core/inspector/JavaScriptCallFrame.cpp#newcode210
Source/core/inspector/JavaScriptCallFrame.cpp:210:
ALLOW_UNUSED_LOCAL(success);
This is wrong assumption as setVariableValue may throw and the caller
should deal with the exception. The method is called from
V8JavaScriptCallFrame::setVariableValueMethodCustom which simply
propagates returned value to the caller.
Since we don't want to deal with the exception here I'd probably
recommend changing return type to MaybeLocal but this would in turn
require either dealing explicitly with the exception in
V8JavaScriptCallFrame::setVariableValueMethodCustom or adding overload
for v8SetReturnValue that would accept MaybeLocal and be no-op
(alternatively we could change v8::FunctionCallbackInfo so that it
allowed to specify explicitly that the result is an exception). WDYT?
https://codereview.chromium.org/1036803002/
ba...@chromium.org
Mar 31, 2015, 9:51:28 PM3/31/15
to joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
https://codereview.chromium.org/1036803002/diff/60001/Source/bindings/core/v8/V8PerIsolateData.cpp
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):
https://codereview.chromium.org/1036803002/diff/60001/Source/bindings/core/v8/V8PerIsolateData.cpp#newcode246
Source/bindings/core/v8/V8PerIsolateData.cpp:246: if
(!V8ScriptRunner::callInternalFunction(v8::Handle<v8::Function>::Cast(value),
info.This(), 0, 0, info.GetIsolate()).ToLocal(&result))
On 2015/03/31 07:20:20, yurys_(ooo until Apr 21) wrote:
> If we fail to get a valid result there must be a pending exception
which the
> caller will have to handle. So I wonder what's the reason for
bothering to set
> any value at all? (I understand that it was written this way before
your change
> but since you are here it may be a good time to improve this).
Changed not to set return value on failure.
> If we fail to get a valid result there must be a pending exception
which the
> caller will have to handle. So I wonder what's the reason for
bothering to set
> any value at all? (I understand that it was written this way before
your change
> but since you are here it may be a good time to improve this).
https://codereview.chromium.org/1036803002/diff/60001/Source/core/inspector/JavaScriptCallFrame.cpp
File Source/core/inspector/JavaScriptCallFrame.cpp (right):
https://codereview.chromium.org/1036803002/diff/60001/Source/core/inspector/JavaScriptCallFrame.cpp#newcode210
Source/core/inspector/JavaScriptCallFrame.cpp:210:
ALLOW_UNUSED_LOCAL(success);
On 2015/03/31 07:20:20, yurys_(ooo until Apr 21) wrote:
> This is wrong assumption as setVariableValue may throw and the caller
should
> deal with the exception. The method is called from
> V8JavaScriptCallFrame::setVariableValueMethodCustom which simply
propagates
> returned value to the caller.
> Since we don't want to deal with the exception here I'd probably
recommend
> changing return type to MaybeLocal but this would in turn require
either dealing
> explicitly with the exception in
> V8JavaScriptCallFrame::setVariableValueMethodCustom or adding overload
for
> v8SetReturnValue that would accept MaybeLocal and be no-op
(alternatively we
> could change v8::FunctionCallbackInfo so that it allowed to specify
explicitly
> that the result is an exception). WDYT?
You said an empty handle is valid here, and this is exactly what the
> This is wrong assumption as setVariableValue may throw and the caller
should
> deal with the exception. The method is called from
> V8JavaScriptCallFrame::setVariableValueMethodCustom which simply
propagates
> returned value to the caller.
> Since we don't want to deal with the exception here I'd probably
recommend
> changing return type to MaybeLocal but this would in turn require
either dealing
> explicitly with the exception in
> V8JavaScriptCallFrame::setVariableValueMethodCustom or adding overload
for
> v8SetReturnValue that would accept MaybeLocal and be no-op
(alternatively we
> could change v8::FunctionCallbackInfo so that it allowed to specify
explicitly
> that the result is an exception). WDYT?
previous code does. That said, I already added v8SetReturnValue which
takes MaybeLocal, so just changing the return type should work. Done.
https://codereview.chromium.org/1036803002/
yu...@chromium.org
Apr 1, 2015, 12:11:03 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
lgtm % the comment below
https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/JavaScriptCallFrame.cpp
File Source/core/inspector/JavaScriptCallFrame.cpp (right):
https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/JavaScriptCallFrame.cpp#newcode194
Source/core/inspector/JavaScriptCallFrame.cpp:194: v8::Local<v8::Value>
result = V8ScriptRunner::callInternalFunction(restartFunction,
callFrame, 0, 0, m_isolate).ToLocalChecked();
Restarting frame may throw so we need to change return type to
MaybeLocal here as well.
https://codereview.chromium.org/1036803002/
https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/JavaScriptCallFrame.cpp
File Source/core/inspector/JavaScriptCallFrame.cpp (right):
https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/JavaScriptCallFrame.cpp#newcode194
Source/core/inspector/JavaScriptCallFrame.cpp:194: v8::Local<v8::Value>
result = V8ScriptRunner::callInternalFunction(restartFunction,
callFrame, 0, 0, m_isolate).ToLocalChecked();
Restarting frame may throw so we need to change return type to
MaybeLocal here as well.
https://codereview.chromium.org/1036803002/
ba...@chromium.org
Apr 1, 2015, 2:54:06 AM4/1/15
to joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/JavaScriptCallFrame.cpp
File Source/core/inspector/JavaScriptCallFrame.cpp (right):
https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/JavaScriptCallFrame.cpp#newcode194
Source/core/inspector/JavaScriptCallFrame.cpp:194: v8::Local<v8::Value>
result = V8ScriptRunner::callInternalFunction(restartFunction,
callFrame, 0, 0, m_isolate).ToLocalChecked();
On 2015/04/01 04:11:02, yurys_(ooo until Apr 21) wrote:
> Restarting frame may throw so we need to change return type to
MaybeLocal here
> as well.
Done.
> Restarting frame may throw so we need to change return type to
MaybeLocal here
> as well.
https://codereview.chromium.org/1036803002/
commi...@chromium.org
Apr 1, 2015, 2:55:00 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1036803002/100001
https://codereview.chromium.org/1036803002/
https://chromium-cq-status.appspot.com/patch-status/1036803002/100001
https://codereview.chromium.org/1036803002/
har...@chromium.org
Apr 1, 2015, 3:00:32 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
LGTM
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8PerIsolateData.cpp
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8PerIsolateData.cpp#newcode247
Source/bindings/core/v8/V8PerIsolateData.cpp:247: v8SetReturnValue(info,
result);
Not related to this CL, it seems better to set v8::String::Empty to the
return value if the callInternalFunction fails.
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8ScriptRunner.cpp
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8ScriptRunner.cpp#newcode488
Source/bindings/core/v8/V8ScriptRunner.cpp:488: crashIfV8IsDead();
BTW, do we still need this check? In a case where crashIfV8IsDead
crashes, the result will be an empty handle.
https://codereview.chromium.org/1036803002/
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8PerIsolateData.cpp
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8PerIsolateData.cpp#newcode247
Source/bindings/core/v8/V8PerIsolateData.cpp:247: v8SetReturnValue(info,
result);
Not related to this CL, it seems better to set v8::String::Empty to the
return value if the callInternalFunction fails.
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8ScriptRunner.cpp
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8ScriptRunner.cpp#newcode488
Source/bindings/core/v8/V8ScriptRunner.cpp:488: crashIfV8IsDead();
BTW, do we still need this check? In a case where crashIfV8IsDead
crashes, the result will be an empty handle.
https://codereview.chromium.org/1036803002/
ba...@chromium.org
Apr 1, 2015, 3:21:04 AM4/1/15
to joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8ScriptRunner.cpp
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8ScriptRunner.cpp#newcode488
Source/bindings/core/v8/V8ScriptRunner.cpp:488: crashIfV8IsDead();
On 2015/04/01 07:00:32, haraken wrote:
> BTW, do we still need this check? In a case where crashIfV8IsDead
crashes, the
> result will be an empty handle.
It may be useful given that ToLocalChecked() is DCHECK, while
> BTW, do we still need this check? In a case where crashIfV8IsDead
crashes, the
> result will be an empty handle.
crashIfV8IsDead() crashes even when we disable assersions.
https://codereview.chromium.org/1036803002/
har...@chromium.org
Apr 1, 2015, 3:25:09 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8ScriptRunner.cpp
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v8/V8ScriptRunner.cpp#newcode488
Source/bindings/core/v8/V8ScriptRunner.cpp:488: crashIfV8IsDead();
On 2015/04/01 07:21:04, bashi1 wrote:
> On 2015/04/01 07:00:32, haraken wrote:
> >
> > BTW, do we still need this check? In a case where crashIfV8IsDead
crashes, the
> > result will be an empty handle.
> It may be useful given that ToLocalChecked() is DCHECK, while
crashIfV8IsDead()
> crashes even when we disable assersions.
I'm wondering why we have two kinds of ASSERTs. Maybe should we update
> On 2015/04/01 07:00:32, haraken wrote:
> >
> > BTW, do we still need this check? In a case where crashIfV8IsDead
crashes, the
> > result will be an empty handle.
> It may be useful given that ToLocalChecked() is DCHECK, while
crashIfV8IsDead()
> crashes even when we disable assersions.
ToLocalChecked so that it crashes when an empty handle is returned?
(What's a problem of doing that?)
https://codereview.chromium.org/1036803002/
ba...@chromium.org
Apr 1, 2015, 3:45:39 AM4/1/15
to joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
Also,
if my understanding is correct, ToLocalChecked() failures aren't equivalent
to
v8::V8::IsDead()
https://codereview.chromium.org/1036803002/
har...@chromium.org
Apr 1, 2015, 3:56:33 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
returned, ToLocalChecked() crashes in Debug builds but returns an empty
handle
without crashing in production builds. Is it really something Blink expects
to
happen? What happens if we continue the execution with the empty handle?
https://codereview.chromium.org/1036803002/
ba...@chromium.org
Apr 1, 2015, 4:32:46 AM4/1/15
to joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
replace ToLocalChecked() with it in Blink. I used ToLocalChecked() because
dcarney@ suggested to use it. As for this CL, we don't use ToLocalChecked()
here
and other places which use ToLocalChecked() are going to move out, I'd
prefer
address it in a separate CL.
https://codereview.chromium.org/1036803002/
har...@chromium.org
Apr 1, 2015, 4:34:40 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
dca...@chromium.org
Apr 1, 2015, 4:39:18 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
> I'm getting a bit confused about ToLocalChecked(). If an empty handle is
> returned, ToLocalChecked() crashes in Debug builds but returns an empty
> handle
> without crashing in production builds. Is it really something Blink
> expects to
> happen? What happens if we continue the execution with the empty handle?
https://codereview.chromium.org/1036803002/
ba...@chromium.org
Apr 1, 2015, 4:46:09 AM4/1/15
to joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
On 2015/04/01 08:39:17, dcarney wrote:
> > I'm getting a bit confused about ToLocalChecked(). If an empty handle is
> > returned, ToLocalChecked() crashes in Debug builds but returns an empty
handle
> > without crashing in production builds. Is it really something Blink
> expects
to
> > happen? What happens if we continue the execution with the empty handle?
> it's fine if ToLocalChecked always crashes. i'll switch it
Thanks!
> > I'm getting a bit confused about ToLocalChecked(). If an empty handle is
> > returned, ToLocalChecked() crashes in Debug builds but returns an empty
handle
> > without crashing in production builds. Is it really something Blink
> expects
to
> > happen? What happens if we continue the execution with the empty handle?
> it's fine if ToLocalChecked always crashes. i'll switch it
https://codereview.chromium.org/1036803002/
har...@chromium.org
Apr 1, 2015, 4:51:23 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
har...@chromium.org
Apr 1, 2015, 4:52:26 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
rename ToLocalChecked to ToLocalOrCrash if possible :)
https://codereview.chromium.org/1036803002/
commi...@chromium.org
Apr 1, 2015, 7:45:12 AM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
Try jobs failed on following builders:
mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/49953)
https://codereview.chromium.org/1036803002/
mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/49953)
https://codereview.chromium.org/1036803002/
commi...@chromium.org
Apr 1, 2015, 10:07:10 PM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1036803002/120001
https://codereview.chromium.org/1036803002/
commi...@chromium.org
Apr 1, 2015, 11:56:44 PM4/1/15
to ba...@chromium.org, joc...@chromium.org, aan...@chromium.org, dca...@chromium.org, har...@chromium.org, yukis...@chromium.org, yu...@chromium.org, aandre...@chromium.org, apavlo...@chromium.org, arv+...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, vive...@samsung.com, viv...@chromium.org, yurys...@chromium.org
Committed patchset #6 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192991
https://codereview.chromium.org/1036803002/
https://src.chromium.org/viewvc/blink?view=rev&revision=192991
https://codereview.chromium.org/1036803002/
Reply all
Reply to author
Forward
0 new messages