Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1036803002:
binidngs: Make callInternalFunction return MaybeLocal (Closed)
|
Created:
5 years, 9 months ago by bashi Modified:
5 years, 8 months ago CC:
aandrey+blink_chromium.org, apavlov+blink_chromium.org, arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, caseq+blink_chromium.org, devtools-reviews_chromium.org, eustas+blink_chromium.org, kozyatinskiy+blink_chromium.org, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, vivekg_samsung, vivekg, yurys+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Descriptionbinidngs: Make callInternalFunction return MaybeLocal
Replacing old v8::Function::Call() with new one.
BUG=462402
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192991
Patch Set 1 #
Total comments: 15
Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Messages
Total messages: 57 (9 generated)
bashi@chromium.org changed reviewers: + haraken@chromium.org, yukishiino@chromium.org, yurys@chromium.org
PTAL?
https://codereview.chromium.org/1036803002/diff/1/Source/bindings/core/v8/Scr... File Source/bindings/core/v8/ScriptDebugServer.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/bindings/core/v8/Scr... Source/bindings/core/v8/ScriptDebugServer.cpp:71: return V8ScriptRunner::callInternalFunction(function, debuggerScript, argc, argv, m_isolate).ToLocalChecked(); How is it guaranteed that this callInternalFunction does not erturn an empty handle? https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:84: v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked(); Ditto. I guess this callInternalFunction can return an empty handle. https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:180: wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result); Can we replace the v8::String::NewFromUtf8 with v8AtomicString in follow-up CLs? Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except the ones in devtools) with v8AtomicString, right?
https://codereview.chromium.org/1036803002/diff/1/Source/bindings/core/v8/Scr... File Source/bindings/core/v8/ScriptRegexp.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/bindings/core/v8/Scr... Source/bindings/core/v8/ScriptRegexp.cpp:82: if (!V8ScriptRunner::callInternalFunction(exec, regex, WTF_ARRAY_LENGTH(argv), argv, isolate).ToLocal(&returnValue)) I don't quite understand what's the advantage of returning MaybeLocal(which will in most cases be followed by ToLocal) compared to calling IsEmpty() on the result handle, can you please explain? Given that users can end up with an empty Local handle after calling ToLocal I don't see a big improvement over inserting IsEmpty() checks at all places where we get Local handle. Is it just to help find all places where Local handle is used in potentially exception unsafe manner?
https://codereview.chromium.org/1036803002/diff/1/Source/bindings/core/v8/Scr... File Source/bindings/core/v8/ScriptDebugServer.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/bindings/core/v8/Scr... Source/bindings/core/v8/ScriptDebugServer.cpp:71: return V8ScriptRunner::callInternalFunction(function, debuggerScript, argc, argv, m_isolate).ToLocalChecked(); On 2015/03/25 12:05:22, haraken wrote: > > How is it guaranteed that this callInternalFunction does not erturn an empty > handle? In fact it is expected to throw in some cases. See ScriptDebugServer::setScriptSource where the exception is handled explicitly. In most of the cases debugger code must not throw and if it does we have no better option than to crash.
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:180: wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result); On 2015/03/25 12:05:22, haraken wrote: > > Can we replace the v8::String::NewFromUtf8 with v8AtomicString in follow-up CLs? > > Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except the ones > in devtools) with v8AtomicString, right? This is exactly one of the places in devtools code that is going to be moved along with the rest of debugger instrumentation so I'd rather you replaced this calls with ToLocalChecked or something like that. BTW, would it make sense to have something like ToLocalOrCrash?
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:180: wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result); On 2015/03/25 12:29:25, yurys wrote: > On 2015/03/25 12:05:22, haraken wrote: > > > > Can we replace the v8::String::NewFromUtf8 with v8AtomicString in follow-up > CLs? > > > > Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except the > ones > > in devtools) with v8AtomicString, right? > > This is exactly one of the places in devtools code that is going to be moved > along with the rest of debugger instrumentation so I'd rather you replaced this > calls with ToLocalChecked or something like that. BTW, would it make sense to > have something like ToLocalOrCrash? ToLocalChecked crashes when V8_ENABLE_CHECKS is defined at compile time. Or, do you want ToLocalOrCrash to crash regardless of V8_ENABLE_CHECKS?
(BTW, I'm getting to understand that this is a tough project -- it's tough to define a good policy about MaybeLocal that scales up to the entire code base. Of course we won't give up :-)
On 2015/03/25 12:34:01, Yuki wrote: > https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... > File Source/core/inspector/JavaScriptCallFrame.cpp (right): > > https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... > Source/core/inspector/JavaScriptCallFrame.cpp:180: > wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result); > On 2015/03/25 12:29:25, yurys wrote: > > On 2015/03/25 12:05:22, haraken wrote: > > > > > > Can we replace the v8::String::NewFromUtf8 with v8AtomicString in follow-up > > CLs? > > > > > > Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except the > > ones > > > in devtools) with v8AtomicString, right? > > > > This is exactly one of the places in devtools code that is going to be moved > > along with the rest of debugger instrumentation so I'd rather you replaced > this > > calls with ToLocalChecked or something like that. BTW, would it make sense to > > have something like ToLocalOrCrash? > > ToLocalChecked crashes when V8_ENABLE_CHECKS is defined at compile time. > > Or, do you want ToLocalOrCrash to crash regardless of V8_ENABLE_CHECKS? The latter as I mentioned elsewhere I can't make up a rationale for why we would want to continue here if the conversion of char* to v8::String has failed. If that happens the code is already in an invalid state and we'd rather crash here than in some distant place in the code. This might increase number of crash reports but that would point at an actual problem explicitly.
On 2015/03/25 12:47:42, yurys wrote: > On 2015/03/25 12:34:01, Yuki wrote: > > > https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... > > File Source/core/inspector/JavaScriptCallFrame.cpp (right): > > > > > https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... > > Source/core/inspector/JavaScriptCallFrame.cpp:180: > > wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result); > > On 2015/03/25 12:29:25, yurys wrote: > > > On 2015/03/25 12:05:22, haraken wrote: > > > > > > > > Can we replace the v8::String::NewFromUtf8 with v8AtomicString in > follow-up > > > CLs? > > > > > > > > Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except > the > > > ones > > > > in devtools) with v8AtomicString, right? > > > > > > This is exactly one of the places in devtools code that is going to be moved > > > along with the rest of debugger instrumentation so I'd rather you replaced > > this > > > calls with ToLocalChecked or something like that. BTW, would it make sense > to > > > have something like ToLocalOrCrash? > > > > ToLocalChecked crashes when V8_ENABLE_CHECKS is defined at compile time. > > > > Or, do you want ToLocalOrCrash to crash regardless of V8_ENABLE_CHECKS? > > The latter as I mentioned elsewhere I can't make up a rationale for why we would > want to continue here if the conversion of char* to v8::String has failed. If > that happens the code is already in an invalid state and we'd rather crash here > than in some distant place in the code. This might increase number of crash > reports but that would point at an actual problem explicitly. +1
On 2015/03/25 12:47:42, yurys wrote: > On 2015/03/25 12:34:01, Yuki wrote: > > > https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... > > File Source/core/inspector/JavaScriptCallFrame.cpp (right): > > > > > https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... > > Source/core/inspector/JavaScriptCallFrame.cpp:180: > > wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result); > > On 2015/03/25 12:29:25, yurys wrote: > > > On 2015/03/25 12:05:22, haraken wrote: > > > > > > > > Can we replace the v8::String::NewFromUtf8 with v8AtomicString in > follow-up > > > CLs? > > > > > > > > Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except > the > > > ones > > > > in devtools) with v8AtomicString, right? > > > > > > This is exactly one of the places in devtools code that is going to be moved > > > along with the rest of debugger instrumentation so I'd rather you replaced > > this > > > calls with ToLocalChecked or something like that. BTW, would it make sense > to > > > have something like ToLocalOrCrash? > > > > ToLocalChecked crashes when V8_ENABLE_CHECKS is defined at compile time. > > > > Or, do you want ToLocalOrCrash to crash regardless of V8_ENABLE_CHECKS? > > The latter as I mentioned elsewhere I can't make up a rationale for why we would > want to continue here if the conversion of char* to v8::String has failed. If > that happens the code is already in an invalid state and we'd rather crash here > than in some distant place in the code. This might increase number of crash > reports but that would point at an actual problem explicitly. This sounds reasonable. Then I think a better fix would to fix the V8 API (i.e., v8::String::NewFromUtf8) so that it crashes for a too large string and never returns an empty handle. I don't think it's a good idea to consider the condition each V8 API returns a MaybeLocal and determines what action the embedder should take for each V8 API. If the embedder needs to take care of what the API does internally, that sounds like a mis-design of the API. Specifically, I don't think it's a good idea to let the embedder choose if the embedder should use ToLocal or ToLocalChecked. If we really want to keep the current V8 API, we can create a thin helper function in V8Binding.h that does the work (i.e., crashes for a too large string and never returns an empty handle).
bashi@chromium.org changed reviewers: + dcarney@chromium.org, jochen@chromium.org
On 2015/03/25 12:21:48, yurys wrote: > I don't quite understand what's the advantage of returning MaybeLocal(which will > in most cases be followed by ToLocal) compared to calling IsEmpty() on the > result handle, can you please explain? Given that users can end up with an empty > Local handle after calling ToLocal I don't see a big improvement over inserting Added jochen@ and dcarney@. ToLocal() is mandatory, but IsEmpty() isn't. Using Maybe we can detect places we miss empty checks on compile time. Using IsEmpty() we should read code carefully, which seems doesn't work well. > IsEmpty() checks at all places where we get Local handle. Is it just to help > find all places where Local handle is used in potentially exception unsafe > manner? IIUC, it's the motivation of this API change. For workers it has been a big problem.
Updated. https://codereview.chromium.org/1036803002/diff/1/Source/bindings/core/v8/Scr... File Source/bindings/core/v8/ScriptDebugServer.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/bindings/core/v8/Scr... Source/bindings/core/v8/ScriptDebugServer.cpp:71: return V8ScriptRunner::callInternalFunction(function, debuggerScript, argc, argv, m_isolate).ToLocalChecked(); On 2015/03/25 12:25:06, yurys wrote: > On 2015/03/25 12:05:22, haraken wrote: > > > > How is it guaranteed that this callInternalFunction does not erturn an empty > > handle? > > In fact it is expected to throw in some cases. See > ScriptDebugServer::setScriptSource where the exception is handled explicitly. In > most of the cases debugger code must not throw and if it does we have no better > option than to crash. In other places, I changed the return value to MaybeLocal, but I'd prefer using ToLocalChecked() here as yurys@ says there is no better option than crash. https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:84: v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked(); 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? https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:180: wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result); On 2015/03/25 12:34:01, Yuki wrote: > On 2015/03/25 12:29:25, yurys wrote: > > On 2015/03/25 12:05:22, haraken wrote: > > > > > > Can we replace the v8::String::NewFromUtf8 with v8AtomicString in follow-up > > CLs? > > > > > > Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except the > > ones > > > in devtools) with v8AtomicString, right? > > > > This is exactly one of the places in devtools code that is going to be moved > > along with the rest of debugger instrumentation so I'd rather you replaced > this > > calls with ToLocalChecked or something like that. BTW, would it make sense to > > have something like ToLocalOrCrash? > > ToLocalChecked crashes when V8_ENABLE_CHECKS is defined at compile time. > > Or, do you want ToLocalOrCrash to crash regardless of V8_ENABLE_CHECKS? Let's use ToLocalChecked() as this is going to be moved out.
I've been musing more about this effort and I think some points need to be clarified before we move further. I'll post my thoughts to the blink-dev thread to reach broader audience.
yurys@chromium.org changed reviewers: + aandrey@chromium.org
https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:84: v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked(); 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 https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:180: wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result); On 2015/03/27 00:39:50, bashi1 wrote: > On 2015/03/25 12:34:01, Yuki wrote: > > On 2015/03/25 12:29:25, yurys wrote: > > > On 2015/03/25 12:05:22, haraken wrote: > > > > > > > > Can we replace the v8::String::NewFromUtf8 with v8AtomicString in > follow-up > > > CLs? > > > > > > > > Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except > the > > > ones > > > > in devtools) with v8AtomicString, right? > > > > > > This is exactly one of the places in devtools code that is going to be moved > > > along with the rest of debugger instrumentation so I'd rather you replaced > > this > > > calls with ToLocalChecked or something like that. BTW, would it make sense > to > > > have something like ToLocalOrCrash? > > > > ToLocalChecked crashes when V8_ENABLE_CHECKS is defined at compile time. > > > > Or, do you want ToLocalOrCrash to crash regardless of V8_ENABLE_CHECKS? > > Let's use ToLocalChecked() as this is going to be moved out. Current implementation with ToLocal call here looks fine to me, it is equivalent to what it did before. https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... Source/core/inspector/JavaScriptCallFrame.cpp:209: return ScriptValue(scriptState, V8ScriptRunner::callInternalFunction(setVariableValueFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate).ToLocalChecked()); 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.... 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.
jochen@, dcarney@: could you comment on the questions below? On 2015/03/26 02:01:06, bashi1 wrote: > On 2015/03/25 12:21:48, yurys wrote: > > I don't quite understand what's the advantage of returning MaybeLocal(which > will > > in most cases be followed by ToLocal) compared to calling IsEmpty() on the > > result handle, can you please explain? Given that users can end up with an > empty > > Local handle after calling ToLocal I don't see a big improvement over > inserting > > Added jochen@ and dcarney@. > > ToLocal() is mandatory, but IsEmpty() isn't. Using Maybe we can detect places we > miss empty checks on compile time. Using IsEmpty() we should read code > carefully, which seems doesn't work well. >
On 2015/03/27 at 12:08:01, yurys wrote: > jochen@, dcarney@: could you comment on the questions below? > > On 2015/03/26 02:01:06, bashi1 wrote: > > On 2015/03/25 12:21:48, yurys wrote: > > > I don't quite understand what's the advantage of returning MaybeLocal(which > > will > > > in most cases be followed by ToLocal) compared to calling IsEmpty() on the > > > result handle, can you please explain? Given that users can end up with an > > empty > > > Local handle after calling ToLocal I don't see a big improvement over > > inserting > > > > Added jochen@ and dcarney@. > > > > ToLocal() is mandatory, but IsEmpty() isn't. Using Maybe we can detect places we > > miss empty checks on compile time. Using IsEmpty() we should read code > > carefully, which seems doesn't work well. > > The advantage of MaybeLocal is that you can just pass it on if you don't want to deal with exceptions at that point. Only before you actually want to use the result, you're forced to deal with exceptions in some form or another.
On 2015/03/27 12:11:20, jochen wrote: > The advantage of MaybeLocal is that you can just pass it on if you don't want to > deal with exceptions at that point. Only before you actually want to use the > result, you're forced to deal with exceptions in some form or another. Empty Local<> can also be passed on to deal with failure later. I understand that the type makes it clear that the handle can be empty and but there is a simple way to covert it to an empty Local. As long as Local::IsEmpty stays in the API it is still dangerous, is there a plan to address this?
once all callsites are updated, we'll remove v8 API methods that can return empty handles.
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?
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.
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; > My concern is that this usage assumes that you have to first allocate Local<> foo initialized with default constructor Local<>(), i.e. the client has to create an empty Local handle which is unfortunate. Ideally Local should never be empty and I'd rather the default Local constructor was dropped to prohibit creating Local handles completely. In other words the only ways to initialize Local should be operator= and copy constructor so that any declaration of Local must be followed by initialization. Does it makes sense? > 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. I see, thanks for explaining.
On 2015/03/27 at 13:41:46, yurys wrote: > 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; > > > My concern is that this usage assumes that you have to first allocate Local<> foo initialized with default constructor Local<>(), i.e. the client has to create an empty Local handle which is unfortunate. Ideally Local should never be empty and I'd rather the default Local constructor was dropped to prohibit creating Local handles completely. In other words the only ways to initialize Local should be operator= and copy constructor so that any declaration of Local must be followed by initialization. Does it makes sense? I'm not sure I understand your concern? Do you think somebody might create an empty handle and inject it into the system? > > > 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. > I see, thanks for explaining.
On 2015/03/27 13:52:44, jochen wrote: > On 2015/03/27 at 13:41:46, yurys wrote: > > 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; > > > > > My concern is that this usage assumes that you have to first allocate Local<> > foo initialized with default constructor Local<>(), i.e. the client has to > create an empty Local handle which is unfortunate. Ideally Local should never be > empty and I'd rather the default Local constructor was dropped to prohibit > creating Local handles completely. In other words the only ways to initialize > Local should be operator= and copy constructor so that any declaration of Local > must be followed by initialization. Does it makes sense? > > I'm not sure I understand your concern? Do you think somebody might create an > empty handle and inject it into the system? > My point is that clients of the API can still create empty Local handles and end up dereferencing them (due to a programming mistake). Example: Local<String> foo; if (some_condition) maybe_foo.ToLocal(&foo); } foo->Length(); This can be more complicated in real code of cause. My point is that it would be much easier to reason about the code it Local<> could never be empty. And to guarantee this we have to get rid of the default constructor Local() (the only way to create empty handle directly).
On 2015/03/27 at 14:18:58, yurys wrote: > On 2015/03/27 13:52:44, jochen wrote: > > On 2015/03/27 at 13:41:46, yurys wrote: > > > 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; > > > > > > > My concern is that this usage assumes that you have to first allocate Local<> > > foo initialized with default constructor Local<>(), i.e. the client has to > > create an empty Local handle which is unfortunate. Ideally Local should never be > > empty and I'd rather the default Local constructor was dropped to prohibit > > creating Local handles completely. In other words the only ways to initialize > > Local should be operator= and copy constructor so that any declaration of Local > > must be followed by initialization. Does it makes sense? > > > > I'm not sure I understand your concern? Do you think somebody might create an > > empty handle and inject it into the system? > > > > My point is that clients of the API can still create empty Local handles and end up dereferencing them (due to a programming mistake). > > Example: > > Local<String> foo; > if (some_condition) > maybe_foo.ToLocal(&foo); > } > foo->Length(); > > This can be more complicated in real code of cause. My point is that it would be much easier to reason about the code it Local<> could never be empty. And to guarantee this we have to get rid of the default constructor Local() (the only way to create empty handle directly). I see. How would you propose to address this?
On 2015/03/27 14:18:58, yurys wrote:
> On 2015/03/27 13:52:44, jochen wrote:
> > On 2015/03/27 at 13:41:46, yurys wrote:
> > > 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;
> > > >
> > > My concern is that this usage assumes that you have to first allocate
> Local<>
> > foo initialized with default constructor Local<>(), i.e. the client has to
> > create an empty Local handle which is unfortunate. Ideally Local should
never
> be
> > empty and I'd rather the default Local constructor was dropped to prohibit
> > creating Local handles completely. In other words the only ways to
initialize
> > Local should be operator= and copy constructor so that any declaration of
> Local
> > must be followed by initialization. Does it makes sense?
> >
> > I'm not sure I understand your concern? Do you think somebody might create
an
> > empty handle and inject it into the system?
> >
>
> My point is that clients of the API can still create empty Local handles and
end
> up dereferencing them (due to a programming mistake).
>
> Example:
>
> Local<String> foo;
> if (some_condition)
> maybe_foo.ToLocal(&foo);
> }
> foo->Length();
>
> This can be more complicated in real code of cause. My point is that it would
be
> much easier to reason about the code it Local<> could never be empty. And to
> guarantee this we have to get rid of the default constructor Local() (the only
> way to create empty handle directly).
If there were no default constructor the following declaration would become a
compile time error:
Local<String> foo;
and the code above would have to be changed to
if (some_condition)
if (!maybe_foo.IsEmpty()) {
Local<String> foo = maybe_foo.ToLocal();
foo->Length();
}
}
On 2015/03/27 at 14:22:02, yurys wrote:
> On 2015/03/27 14:18:58, yurys wrote:
> > On 2015/03/27 13:52:44, jochen wrote:
> > > On 2015/03/27 at 13:41:46, yurys wrote:
> > > > 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;
> > > > >
> > > > My concern is that this usage assumes that you have to first allocate
> > Local<>
> > > foo initialized with default constructor Local<>(), i.e. the client has to
> > > create an empty Local handle which is unfortunate. Ideally Local should
never
> > be
> > > empty and I'd rather the default Local constructor was dropped to prohibit
> > > creating Local handles completely. In other words the only ways to
initialize
> > > Local should be operator= and copy constructor so that any declaration of
> > Local
> > > must be followed by initialization. Does it makes sense?
> > >
> > > I'm not sure I understand your concern? Do you think somebody might create
an
> > > empty handle and inject it into the system?
> > >
> >
> > My point is that clients of the API can still create empty Local handles and
end
> > up dereferencing them (due to a programming mistake).
> >
> > Example:
> >
> > Local<String> foo;
> > if (some_condition)
> > maybe_foo.ToLocal(&foo);
> > }
> > foo->Length();
> >
> > This can be more complicated in real code of cause. My point is that it
would be
> > much easier to reason about the code it Local<> could never be empty. And to
> > guarantee this we have to get rid of the default constructor Local() (the
only
> > way to create empty handle directly).
>
> If there were no default constructor the following declaration would become a
compile time error:
> Local<String> foo;
>
> and the code above would have to be changed to
>
> if (some_condition)
> if (!maybe_foo.IsEmpty()) {
> Local<String> foo = maybe_foo.ToLocal();
> foo->Length();
> }
> }
But ToLocal() in this example has to CHECK (and so it's slow)
On 2015/03/27 14:21:02, jochen wrote:
> On 2015/03/27 at 14:18:58, yurys wrote:
> > On 2015/03/27 13:52:44, jochen wrote:
> > > On 2015/03/27 at 13:41:46, yurys wrote:
> > > > 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;
> > > > >
> > > > My concern is that this usage assumes that you have to first allocate
> Local<>
> > > foo initialized with default constructor Local<>(), i.e. the client has to
> > > create an empty Local handle which is unfortunate. Ideally Local should
> never be
> > > empty and I'd rather the default Local constructor was dropped to prohibit
> > > creating Local handles completely. In other words the only ways to
> initialize
> > > Local should be operator= and copy constructor so that any declaration of
> Local
> > > must be followed by initialization. Does it makes sense?
> > >
> > > I'm not sure I understand your concern? Do you think somebody might create
> an
> > > empty handle and inject it into the system?
> > >
> >
> > My point is that clients of the API can still create empty Local handles and
> end up dereferencing them (due to a programming mistake).
> >
> > Example:
> >
> > Local<String> foo;
> > if (some_condition)
> > maybe_foo.ToLocal(&foo);
> > }
> > foo->Length();
> >
> > This can be more complicated in real code of cause. My point is that it
would
> be much easier to reason about the code it Local<> could never be empty. And
to
> guarantee this we have to get rid of the default constructor Local() (the only
> way to create empty handle directly).
>
> I see. How would you propose to address this?
1. Mark Local() as V8_DEPRECATE_SOON
2. Fix all places where Local is initialized with default constructor so that
they are initialized using copy constructor with preceding IsEmpty check.
E.g.
Local<String> foo;
foo = bar();
baz(foo);
should become
MaybeHandle<String> foo = bar();
if (foo.IsEmpty()) {
<handle exception>
} else {
baz(foo.ToLocal());
}
The usages can be found automatically by enabling deprecation warning.
3. Make ToLocal() crash if it is called on empty MaybeLocal.
4. Deprecate and remove default constructor Local().
I don't see a neat wat to make compiler check that we didn't miss IsEmpty()
check in this case. ToLocal could accept two callbacks: onsuccess(Local) and
onerror(MaybeLocal) but this is awkward and too verbose.
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.
Patchset #3 (id:40001) has been deleted
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/JavaS... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/1/Source/core/inspector/JavaS... 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/JavaS... 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.... > 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/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/1036803002/diff/60001/Source/bindings/core/v8... 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/J... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/60001/Source/core/inspector/J... 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/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/1036803002/diff/60001/Source/bindings/core/v8... 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. https://codereview.chromium.org/1036803002/diff/60001/Source/core/inspector/J... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/60001/Source/core/inspector/J... 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 previous code does. That said, I already added v8SetReturnValue which takes MaybeLocal, so just changing the return type should work. Done.
lgtm % the comment below https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/J... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/J... 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/diff/80001/Source/core/inspector/J... File Source/core/inspector/JavaScriptCallFrame.cpp (right): https://codereview.chromium.org/1036803002/diff/80001/Source/core/inspector/J... 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.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org Link to the patchset: https://codereview.chromium.org/1036803002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1036803002/100001
LGTM https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... 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/v... File Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... 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/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... 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 crashIfV8IsDead() crashes even when we disable assersions.
https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... 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 ToLocalChecked so that it crashes when an empty handle is returned? (What's a problem of doing that?)
On 2015/04/01 07:25:09, haraken wrote: > https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... > File Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... > 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 > ToLocalChecked so that it crashes when an empty handle is returned? (What's a > problem of doing that?) Other embedder may not want to crash always when ToLocalChecked() fails. Also, if my understanding is correct, ToLocalChecked() failures aren't equivalent to v8::V8::IsDead()
On 2015/04/01 07:45:39, bashi1 wrote: > On 2015/04/01 07:25:09, haraken wrote: > > > https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... > > File Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > > > > https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... > > 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 > > ToLocalChecked so that it crashes when an empty handle is returned? (What's a > > problem of doing that?) > > Other embedder may not want to crash always when ToLocalChecked() fails. Also, > if my understanding is correct, ToLocalChecked() failures aren't equivalent to > v8::V8::IsDead() 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?
On 2015/04/01 07:56:33, haraken wrote: > On 2015/04/01 07:45:39, bashi1 wrote: > > On 2015/04/01 07:25:09, haraken wrote: > > > > > > https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... > > > File Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > > > > > > > > https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... > > > 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 > > > ToLocalChecked so that it crashes when an empty handle is returned? (What's > a > > > problem of doing that?) > > > > Other embedder may not want to crash always when ToLocalChecked() fails. Also, > > if my understanding is correct, ToLocalChecked() failures aren't equivalent to > > v8::V8::IsDead() > > 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? Hmm, we can add a function which always crashes when ToLocal() failed and 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.
On 2015/04/01 08:32:45, bashi1 wrote: > On 2015/04/01 07:56:33, haraken wrote: > > On 2015/04/01 07:45:39, bashi1 wrote: > > > On 2015/04/01 07:25:09, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... > > > > File Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1036803002/diff/100001/Source/bindings/core/v... > > > > 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 > > > > ToLocalChecked so that it crashes when an empty handle is returned? > (What's > > a > > > > problem of doing that?) > > > > > > Other embedder may not want to crash always when ToLocalChecked() fails. > Also, > > > if my understanding is correct, ToLocalChecked() failures aren't equivalent > to > > > v8::V8::IsDead() > > > > 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? > > Hmm, we can add a function which always crashes when ToLocal() failed and > 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. Yes, a separate CL is fine.
> 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
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!
On 2015/04/01 08:46:09, bashi1 wrote: > 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! Thanks
On 2015/04/01 08:51:23, haraken wrote: > On 2015/04/01 08:46:09, bashi1 wrote: > > 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! > > Thanks I pushed the send button prematurely. I wanted to say it would be better to rename ToLocalChecked to ToLocalOrCrash if possible :)
The CQ bit was unchecked by commit-bot@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)
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1036803002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1036803002/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192991 |
