binidngs: Make callInternalFunction return MaybeLocal

Source/core/inspector/JavaScriptCallFrame.cpp

 /*
  * Copyright (c) 2010, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 51 matching lines @@
     }
     return m_caller.get();
 }
 
 int JavaScriptCallFrame::callV8FunctionReturnInt(const char* name) const
 {
     v8::HandleScope handleScope(m_isolate);
     v8::Context::Scope contextScope(m_debuggerContext.newLocal(m_isolate));
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, name)));
-    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate);
-    if (result.IsEmpty() || !result->IsInt32())
+    v8::Local<v8::Value> result;
+    if (!V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocal(&result) || !result->IsInt32())
         return 0;
-    return result->Int32Value();
+    return result.As<v8::Int32>()->Value();
 }
 
 String JavaScriptCallFrame::callV8FunctionReturnString(const char* name) const
 {
     v8::HandleScope handleScope(m_isolate);
     v8::Context::Scope contextScope(m_debuggerContext.newLocal(m_isolate));
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, name)));
-    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate);
+    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked();

[haraken, 2015/03/25 12:05:22]

Ditto. I guess this callInternalFunction can return an empty handle.

[bashi, 2015/03/27 00:39:50]

Yes. If we have better option than crash (e.g. return an empty string), it would
be great. yurys@, WDYT?

[yurys, 2015/03/27 12:07:24]

@aanrey would know better if his place is allowed to throw when called e.g. from
JavaScriptCallFrame::stepInPositions

[bashi, 2015/03/31 05:46:08]

Changed to return String() since this function seems to have the same semantics
of callV8FunctionReturnInt, which returns 0 on failure.

     return toCoreStringWithUndefinedOrNullCheck(result);
 }
 
 int JavaScriptCallFrame::sourceID() const
 {
     return callV8FunctionReturnInt("sourceID");
 }
 
 int JavaScriptCallFrame::line() const
 {
@@ skipping 22 matching lines @@
 
 int JavaScriptCallFrame::functionColumn() const
 {
     return callV8FunctionReturnInt("functionColumn");
 }
 
 v8::Local<v8::Value> JavaScriptCallFrame::scopeChain() const
 {
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, "scopeChain")));
-    v8::Local<v8::Array> scopeChain = v8::Local<v8::Array>::Cast(V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate));
+    v8::Local<v8::Array> scopeChain = v8::Local<v8::Array>::Cast(V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked());
     v8::Local<v8::Array> result = v8::Array::New(m_isolate, scopeChain->Length());
     for (uint32_t i = 0; i < scopeChain->Length(); i++)
         result->Set(i, scopeChain->Get(i));
     return result;
 }
 
 int JavaScriptCallFrame::scopeType(int scopeIndex) const
 {
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, "scopeType")));
-    v8::Local<v8::Array> scopeType = v8::Local<v8::Array>::Cast(V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate));
+    v8::Local<v8::Array> scopeType = v8::Local<v8::Array>::Cast(V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked());
     return scopeType->Get(scopeIndex)->Int32Value();
 }
 
 v8::Local<v8::Value> JavaScriptCallFrame::thisObject() const
 {
     return m_callFrame.newLocal(m_isolate)->Get(v8AtomicString(m_isolate, "thisObject"));
 }
 
 String JavaScriptCallFrame::stepInPositions() const
 {
@@ skipping 18 matching lines @@
 ScriptValue JavaScriptCallFrame::evaluateWithExceptionDetails(ScriptState* scriptState, const String& expression, const ScriptValue& scopeExtension)
 {
     ScriptState::Scope scriptScope(scriptState);
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> evalFunction = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, "evaluate")));
     v8::Local<v8::Value> argv[] = {
         v8String(m_debuggerContext.newLocal(m_isolate)->GetIsolate(), expression),
         scopeExtension.isEmpty() ? v8::Handle<v8::Value>::Cast(v8::Undefined(m_isolate)) : scopeExtension.v8Value()
     };
     v8::TryCatch tryCatch;
-    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(evalFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate);
-
     v8::Local<v8::Object> wrappedResult = v8::Object::New(m_isolate);
-    if (tryCatch.HasCaught()) {
+    v8::Local<v8::Value> result;
+    if (V8ScriptRunner::callInternalFunction(evalFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate).ToLocal(&result)) {
+        wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result);

[haraken, 2015/03/25 12:05:22]

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?

[yurys, 2015/03/25 12:29:25]

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?

[Yuki, 2015/03/25 12:34:01]

ToLocalChecked crashes when V8_ENABLE_CHECKS is defined at compile time.

Or, do you want ToLocalOrCrash to crash regardless of V8_ENABLE_CHECKS?

[bashi, 2015/03/27 00:39:50]

Let's use ToLocalChecked() as this is going to be moved out.

[yurys, 2015/03/27 12:07:24]

Current implementation with ToLocal call here looks fine to me, it is equivalent
to what it did before.

+        wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "exceptionDetails"), v8::Undefined(m_isolate));
+    } else {
         wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), tryCatch.Exception());
         wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "exceptionDetails"), createExceptionDetails(m_isolate, tryCatch.Message()));
-    } else {
-        wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result);
-        wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "exceptionDetails"), v8::Undefined(m_isolate));
     }
     return ScriptValue(scriptState, wrappedResult);
 }
 
 v8::Local<v8::Value> JavaScriptCallFrame::restart()
 {
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> restartFunction = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, "restart")));
     v8::Debug::SetLiveEditEnabled(m_isolate, true);
-    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(restartFunction, callFrame, 0, 0, m_isolate);
+    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(restartFunction, callFrame, 0, 0, m_isolate).ToLocalChecked();
     v8::Debug::SetLiveEditEnabled(m_isolate, false);
     return result;
 }
 
 ScriptValue JavaScriptCallFrame::setVariableValue(ScriptState* scriptState, int scopeNumber, const String& variableName, const ScriptValue& newValue)
 {
     ScriptState::Scope scriptScope(scriptState);
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> setVariableValueFunction = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, "setVariableValue")));
     v8::Local<v8::Value> argv[] = {
         v8::Local<v8::Value>(v8::Integer::New(m_isolate, scopeNumber)),
         v8String(m_isolate, variableName),
         newValue.v8Value()
     };
-    return ScriptValue(scriptState, V8ScriptRunner::callInternalFunction(setVariableValueFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate));
+    return ScriptValue(scriptState, V8ScriptRunner::callInternalFunction(setVariableValueFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate).ToLocalChecked());

[yurys, 2015/03/27 12:07:24]

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.

[bashi, 2015/03/31 05:46:08]

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.

 }
 
 v8::Local<v8::Object> JavaScriptCallFrame::createExceptionDetails(v8::Isolate* isolate, v8::Local<v8::Message> message)
 {
     v8::Local<v8::Object> exceptionDetails = v8::Object::New(isolate);
     exceptionDetails->Set(v8::String::NewFromUtf8(isolate, "text"), message->Get());
     exceptionDetails->Set(v8::String::NewFromUtf8(isolate, "url"), message->GetScriptOrigin().ResourceName());
     exceptionDetails->Set(v8::String::NewFromUtf8(isolate, "scriptId"), v8::Integer::New(isolate, message->GetScriptOrigin().ScriptID()->Value()));
     exceptionDetails->Set(v8::String::NewFromUtf8(isolate, "line"), v8::Integer::New(isolate, message->GetLineNumber()));
     exceptionDetails->Set(v8::String::NewFromUtf8(isolate, "column"), v8::Integer::New(isolate, message->GetStartColumn()));
     if (!message->GetStackTrace().IsEmpty())
         exceptionDetails->Set(v8::String::NewFromUtf8(isolate, "stackTrace"), message->GetStackTrace()->AsArray());
     else
         exceptionDetails->Set(v8::String::NewFromUtf8(isolate, "stackTrace"), v8::Undefined(isolate));
     return exceptionDetails;
 }
 
 DEFINE_TRACE(JavaScriptCallFrame)
 {
     visitor->trace(m_caller);
 }
 
 } // namespace blink