bindings: Use Maybe APIs in V8ScriptRunner (part 1)

Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp

Index: Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp
diff --git a/Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp b/Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp
index c169b37bca1197a1324d0529837def04cd6c4c7a..6ed37726e3fcf20f7ae3961cbc2f80900e4cc877 100644
--- a/Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp
+++ b/Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp
@@ -378,8 +378,8 @@ void V8InjectedScriptHost::evalMethodCustom(const v8::FunctionCallbackInfo<v8::V
 
     ASSERT(isolate->InContext());
     v8::TryCatch tryCatch;
-    v8::Handle<v8::Value> result = V8ScriptRunner::compileAndRunInternalScript(expression, info.GetIsolate());
-    if (tryCatch.HasCaught()) {
+    v8::Local<v8::Value> result;
+    if (!v8Call(V8ScriptRunner::compileAndRunInternalScript(expression, info.GetIsolate()), result, tryCatch)) {
         v8SetReturnValue(info, tryCatch.ReThrow());
         return;
     }
@@ -402,15 +402,20 @@ void V8InjectedScriptHost::evaluateWithExceptionDetailsMethodCustom(const v8::Fu
 
     ASSERT(isolate->InContext());
     v8::TryCatch tryCatch;
-    v8::Handle<v8::Script> script = V8ScriptRunner::compileScript(expression, String(), String(), TextPosition(), isolate);
-    v8::Handle<v8::Value> result = V8ScriptRunner::runCompiledScript(isolate, script, currentExecutionContext(isolate));
+    v8::Local<v8::Script> script;
+    v8::MaybeLocal<v8::Value> result;
+    if (v8Call(V8ScriptRunner::compileScript(expression, String(), String(), TextPosition(), isolate), script, tryCatch))
+        result = V8ScriptRunner::runCompiledScript(isolate, script, currentExecutionContext(isolate));
 
+    if (!tryCatch.CanContinue())

[haraken, 2015/03/18 01:58:51]

Sorry for a repeated question, but when do we need CanContinue and when do we
not? I'm wondering why we have CanContinue here but don't in other places.

[bashi, 2015/03/18 02:33:12]

Other places return immediately on failure, but this isn't the case, and calls
V8 APIs, which may fail. No?

[haraken, 2015/03/18 08:27:37]

Hmm, I'm not sure of how helpful the CanContinue check is. Even if we have the
CanContinue check, we'll crash somewhere unexpected because the binding code is
not written in a can-continue-aware manner. If we don't have the check, we'll
crash in the next V8 API. I'm not sure if the former is improving the situation.

I think this is a separate issue we need to address in the future.

[bashi, 2015/03/20 01:16:07]

OK, removed.

+        return;
     v8::Local<v8::Object> wrappedResult = v8::Object::New(isolate);
     if (tryCatch.HasCaught()) {

[haraken, 2015/03/18 08:27:37]

Also this HasCaught check is a bit too late. The Object::New might cause a GC,
which may set another exception. Basically HasCaught must be called immediately
after the V8 API you called.

[bashi, 2015/03/20 01:16:06]

Changed the current implementation so that we don't continue when APIs fail.

         wrappedResult->Set(v8::String::NewFromUtf8(isolate, "result"), tryCatch.Exception());
         wrappedResult->Set(v8::String::NewFromUtf8(isolate, "exceptionDetails"), JavaScriptCallFrame::createExceptionDetails(isolate, tryCatch.Message()));
     } else {

[haraken, 2015/03/18 01:58:51]

Is it possible to hit this branch?

Given that you added ASSERT(success||tryCatch.HasCaught()) to V8BindingMacros.h,
I think we always hit the above tryCatch.HasCaught() branch.

[bashi, 2015/03/18 02:33:12]

If v8ScriptRunner::runCompiledScript() succeeds, we reach this branch.
If fails, we reach L414.

-        wrappedResult->Set(v8::String::NewFromUtf8(isolate, "result"), result);
+        ASSERT(!result.IsEmpty());
+        wrappedResult->Set(v8::String::NewFromUtf8(isolate, "result"), result.ToLocalChecked());
         wrappedResult->Set(v8::String::NewFromUtf8(isolate, "exceptionDetails"), v8::Undefined(isolate));
     }
     v8SetReturnValue(info, wrappedResult);