Blink-in-JS: Make error messages thrown in private scripts more verbose

Source/bindings/core/v8/PrivateScriptRunner.cpp

Index: Source/bindings/core/v8/PrivateScriptRunner.cpp
diff --git a/Source/bindings/core/v8/PrivateScriptRunner.cpp b/Source/bindings/core/v8/PrivateScriptRunner.cpp
index c78bdb8191d17161974503dc0b4498aabb0f0880..9569478b901384c2fafba0783a99f7bd86098f0a 100644
--- a/Source/bindings/core/v8/PrivateScriptRunner.cpp
+++ b/Source/bindings/core/v8/PrivateScriptRunner.cpp
@@ -18,17 +18,45 @@
 
 namespace blink {
 
-#define LOG_ERROR_ALWAYS(...) WTFReportError(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, __VA_ARGS__)
+static void dumpV8Message(v8::Handle<v8::Message> message)
+{
+    if (message.IsEmpty())
+        return;
+
+    v8::Handle<v8::Value> resourceName = message->GetScriptOrigin().ResourceName();
+    String fileName = "Unknown JavaScript file";
+    if (!resourceName.IsEmpty() && resourceName->IsString())
+        fileName = toCoreString(v8::Handle<v8::String>::Cast(resourceName));
+    int lineNumber = message->GetLineNumber();
+    v8::Handle<v8::String> errorMessage = message->Get();
+    fprintf(stderr, "%s (line %d): %s\n", fileName.utf8().data(), lineNumber, toCoreString(errorMessage).utf8().data());
+}
+
+static void dumpJSError(String exceptionName, v8::Handle<v8::Message> message)
+{
+    // FIXME: Set a ScriptOrigin of the private script and print a more informative message.
+#ifndef NDEBUG
+    fprintf(stderr, "Private script throws an exception: %s\n", exceptionName.utf8().data());
+    dumpV8Message(message);
+#endif
+}
 
-static v8::Handle<v8::Value> compileAndRunInternalScript(v8::Isolate* isolate, String className, const unsigned char* source, size_t size)
+static v8::Handle<v8::Value> compilePrivateScript(v8::Isolate* isolate, String className, const unsigned char* source, size_t size)

[Jens Widell, 2014/08/13 12:08:38]

I think "compileAndRun" was a bit more accurate. Any reason to call this just
"compile" when it also runs?

[haraken, 2014/08/13 12:37:57]

Done.

 {
     v8::TryCatch block;
     String sourceString(reinterpret_cast<const char*>(source), size);
-    v8::Handle<v8::Value> result = V8ScriptRunner::compileAndRunInternalScript(v8String(isolate, sourceString), isolate);
+    String fileName = className + ".js";

[Jens Widell, 2014/08/13 12:08:38]

This isn't necessarily correct for a partial interface, is it? It wouldn't be
entirely helpful if the error message said "Document.js (line 15)" when the
error is actually in DocumentXSLT.js.

[haraken, 2014/08/13 12:37:57]

Good point. I changed the code so that |className| becomes a class name of a
partial interface.

However, the current variable names (e.g., |className| and
|dependencyClassName|) are a bit confusing. I'll rename them as follows in a
follow-up CL.

- |className| means a class name of a dependency interface (e.g., "Document").

- |partialClassName| means a class name of a partial interface (e.g.,
"DocumentXSLT").

[Jens Widell, 2014/08/13 12:56:18]

Sounds like a good plan.

+    v8::Handle<v8::Script> script = V8ScriptRunner::compileScript(v8String(isolate, sourceString), fileName, TextPosition::minimumPosition(), 0, isolate, NotSharableCrossOrigin, V8CacheOptionsOff);
     if (block.HasCaught()) {
-        LOG_ERROR_ALWAYS("Private script error: Compile failed. (Class name = %s)\n", className.utf8().data());
-        if (!block.Message().IsEmpty())
-            LOG_ERROR_ALWAYS("%s\n", toCoreString(block.Message()->Get()).utf8().data());
+        fprintf(stderr, "Private script error: Compile failed. (Class name = %s)\n", className.utf8().data());
+        dumpV8Message(block.Message());
+        RELEASE_ASSERT_NOT_REACHED();
+    }
+
+    v8::Handle<v8::Value> result = V8ScriptRunner::runCompiledInternalScript(script, isolate);
+    if (block.HasCaught()) {
+        fprintf(stderr, "Private script error: installClass() failed. (Class name = %s)\n", className.utf8().data());
+        dumpV8Message(block.Message());
         RELEASE_ASSERT_NOT_REACHED();
     }
     return result;
@@ -38,7 +66,7 @@ static v8::Handle<v8::Value> compileAndRunInternalScript(v8::Isolate* isolate, S
 // are compiled when any of the JS files is requested. Ideally we should avoid compiling
 // unrelated JS files. For example, if a method in XPartial-1.js is requested, we just
 // need to compile X.js and XPartial-1.js, and don't need to compile XPartial-2.js.
-static void compilePrivateScript(v8::Isolate* isolate, String className)
+static void installPrivateScript(v8::Isolate* isolate, String className)
 {
     int compiledScriptCount = 0;
     // |kPrivateScriptSourcesForTesting| is defined in V8PrivateScriptSources.h, which is auto-generated
@@ -46,7 +74,7 @@ static void compilePrivateScript(v8::Isolate* isolate, String className)
 #ifndef NDEBUG
     for (size_t index = 0; index < WTF_ARRAY_LENGTH(kPrivateScriptSourcesForTesting); index++) {
         if (className == kPrivateScriptSourcesForTesting[index].dependencyClassName) {
-            compileAndRunInternalScript(isolate, className, kPrivateScriptSourcesForTesting[index].source, kPrivateScriptSourcesForTesting[index].size);
+            compilePrivateScript(isolate, className, kPrivateScriptSourcesForTesting[index].source, kPrivateScriptSourcesForTesting[index].size);
             compiledScriptCount++;
         }
     }
@@ -56,18 +84,18 @@ static void compilePrivateScript(v8::Isolate* isolate, String className)
     // by make_private_script_source.py.
     for (size_t index = 0; index < WTF_ARRAY_LENGTH(kPrivateScriptSources); index++) {
         if (className == kPrivateScriptSources[index].dependencyClassName) {
-            compileAndRunInternalScript(isolate, className, kPrivateScriptSources[index].source, kPrivateScriptSources[index].size);
+            compilePrivateScript(isolate, className, kPrivateScriptSources[index].source, kPrivateScriptSources[index].size);
             compiledScriptCount++;
         }
     }
 
     if (!compiledScriptCount) {
-        LOG_ERROR_ALWAYS("Private script error: Target source code was not found. (Class name = %s)\n", className.utf8().data());
+        fprintf(stderr, "Private script error: Target source code was not found. (Class name = %s)\n", className.utf8().data());
         RELEASE_ASSERT_NOT_REACHED();
     }
 }
 
-static v8::Handle<v8::Value> compilePrivateScriptRunner(v8::Isolate* isolate)
+static v8::Handle<v8::Value> installPrivateScriptRunner(v8::Isolate* isolate)
 {
     const String className = "PrivateScriptRunner";
     size_t index;
@@ -78,10 +106,10 @@ static v8::Handle<v8::Value> compilePrivateScriptRunner(v8::Isolate* isolate)
             break;
     }
     if (index == WTF_ARRAY_LENGTH(kPrivateScriptSources)) {
-        LOG_ERROR_ALWAYS("Private script error: Target source code was not found. (Class name = %s)\n", className.utf8().data());
+        fprintf(stderr, "Private script error: Target source code was not found. (Class name = %s)\n", className.utf8().data());
         RELEASE_ASSERT_NOT_REACHED();
     }
-    return compileAndRunInternalScript(isolate, className, kPrivateScriptSources[index].source, kPrivateScriptSources[index].size);
+    return compilePrivateScript(isolate, className, kPrivateScriptSources[index].source, kPrivateScriptSources[index].size);
 }
 
 static v8::Handle<v8::Object> classObjectOfPrivateScript(ScriptState* scriptState, String className)
@@ -93,13 +121,13 @@ static v8::Handle<v8::Object> classObjectOfPrivateScript(ScriptState* scriptStat
     if (compiledClass.IsEmpty()) {
         v8::Handle<v8::Value> installedClasses = scriptState->perContextData()->compiledPrivateScript("PrivateScriptRunner");
         if (installedClasses.IsEmpty()) {
-            installedClasses = compilePrivateScriptRunner(isolate);
+            installedClasses = installPrivateScriptRunner(isolate);
             scriptState->perContextData()->setCompiledPrivateScript("PrivateScriptRunner", installedClasses);
         }
         RELEASE_ASSERT(!installedClasses.IsEmpty());
         RELEASE_ASSERT(installedClasses->IsObject());
 
-        compilePrivateScript(isolate, className);
+        installPrivateScript(isolate, className);
         compiledClass = v8::Handle<v8::Object>::Cast(installedClasses)->Get(v8String(isolate, className));
         RELEASE_ASSERT(!compiledClass.IsEmpty());
         RELEASE_ASSERT(compiledClass->IsObject());
@@ -122,9 +150,8 @@ static void initializeHolderIfNeeded(ScriptState* scriptState, v8::Handle<v8::Ob
             v8::TryCatch block;
             V8ScriptRunner::callFunction(v8::Handle<v8::Function>::Cast(initializeFunction), scriptState->executionContext(), holder, 0, 0, isolate);
             if (block.HasCaught()) {
-                LOG_ERROR_ALWAYS("Private script error: Object constructor threw an exception.\n");
-                if (!block.Message().IsEmpty())
-                    LOG_ERROR_ALWAYS("%s\n", toCoreString(block.Message()->Get()).utf8().data());
+                fprintf(stderr, "Private script error: Object constructor threw an exception.\n");
+                dumpV8Message(block.Message());
                 RELEASE_ASSERT_NOT_REACHED();
             }
         }
@@ -163,12 +190,12 @@ v8::Handle<v8::Value> PrivateScriptRunner::runDOMAttributeGetter(ScriptState* sc
     v8::Handle<v8::Object> classObject = classObjectOfPrivateScript(scriptState, className);
     v8::Handle<v8::Value> descriptor = classObject->GetOwnPropertyDescriptor(v8String(scriptState->isolate(), attributeName));
     if (descriptor.IsEmpty() || !descriptor->IsObject()) {
-        LOG_ERROR_ALWAYS("Private script error: Target DOM attribute getter was not found. (Class name = %s, Attribute name = %s)\n", className.utf8().data(), attributeName.utf8().data());
+        fprintf(stderr, "Private script error: Target DOM attribute getter was not found. (Class name = %s, Attribute name = %s)\n", className.utf8().data(), attributeName.utf8().data());
         RELEASE_ASSERT_NOT_REACHED();
     }
     v8::Handle<v8::Value> getter = v8::Handle<v8::Object>::Cast(descriptor)->Get(v8String(scriptState->isolate(), "get"));
     if (getter.IsEmpty() || !getter->IsFunction()) {
-        LOG_ERROR_ALWAYS("Private script error: Target DOM attribute getter was not found. (Class name = %s, Attribute name = %s)\n", className.utf8().data(), attributeName.utf8().data());
+        fprintf(stderr, "Private script error: Target DOM attribute getter was not found. (Class name = %s, Attribute name = %s)\n", className.utf8().data(), attributeName.utf8().data());
         RELEASE_ASSERT_NOT_REACHED();
     }
     initializeHolderIfNeeded(scriptState, classObject, holder);
@@ -180,12 +207,12 @@ void PrivateScriptRunner::runDOMAttributeSetter(ScriptState* scriptState, String
     v8::Handle<v8::Object> classObject = classObjectOfPrivateScript(scriptState, className);
     v8::Handle<v8::Value> descriptor = classObject->GetOwnPropertyDescriptor(v8String(scriptState->isolate(), attributeName));
     if (descriptor.IsEmpty() || !descriptor->IsObject()) {
-        LOG_ERROR_ALWAYS("Private script error: Target DOM attribute setter was not found. (Class name = %s, Attribute name = %s)\n", className.utf8().data(), attributeName.utf8().data());
+        fprintf(stderr, "Private script error: Target DOM attribute setter was not found. (Class name = %s, Attribute name = %s)\n", className.utf8().data(), attributeName.utf8().data());
         RELEASE_ASSERT_NOT_REACHED();
     }
     v8::Handle<v8::Value> setter = v8::Handle<v8::Object>::Cast(descriptor)->Get(v8String(scriptState->isolate(), "set"));
     if (setter.IsEmpty() || !setter->IsFunction()) {
-        LOG_ERROR_ALWAYS("Private script error: Target DOM attribute setter was not found. (Class name = %s, Attribute name = %s)\n", className.utf8().data(), attributeName.utf8().data());
+        fprintf(stderr, "Private script error: Target DOM attribute setter was not found. (Class name = %s, Attribute name = %s)\n", className.utf8().data(), attributeName.utf8().data());
         RELEASE_ASSERT_NOT_REACHED();
     }
     initializeHolderIfNeeded(scriptState, classObject, holder);
@@ -198,23 +225,13 @@ v8::Handle<v8::Value> PrivateScriptRunner::runDOMMethod(ScriptState* scriptState
     v8::Handle<v8::Object> classObject = classObjectOfPrivateScript(scriptState, className);
     v8::Handle<v8::Value> method = classObject->Get(v8String(scriptState->isolate(), methodName));
     if (method.IsEmpty() || !method->IsFunction()) {
-        LOG_ERROR_ALWAYS("Private script error: Target DOM method was not found. (Class name = %s, Method name = %s)\n", className.utf8().data(), methodName.utf8().data());
+        fprintf(stderr, "Private script error: Target DOM method was not found. (Class name = %s, Method name = %s)\n", className.utf8().data(), methodName.utf8().data());
         RELEASE_ASSERT_NOT_REACHED();
     }
     initializeHolderIfNeeded(scriptState, classObject, holder);
     return V8ScriptRunner::callFunction(v8::Handle<v8::Function>::Cast(method), scriptState->executionContext(), holder, argc, argv, scriptState->isolate());
 }
 
-static void dumpJSError(String exceptionName, String message)
-{
-    // FIXME: Set a ScriptOrigin of the private script and print a more informative message.
-#ifndef NDEBUG
-    fprintf(stderr, "Private script throws an exception: %s\n", exceptionName.utf8().data());
-    if (!message.isEmpty())
-        fprintf(stderr, "%s\n", message.utf8().data());
-#endif
-}
-
 bool PrivateScriptRunner::rethrowExceptionInPrivateScript(v8::Isolate* isolate, ExceptionState& exceptionState, v8::TryCatch& block)
 {
     v8::Handle<v8::Value> exception = block.Exception();
@@ -225,59 +242,49 @@ bool PrivateScriptRunner::rethrowExceptionInPrivateScript(v8::Isolate* isolate,
     v8::Handle<v8::Value> name = exceptionObject->Get(v8String(isolate, "name"));
     if (name.IsEmpty() || !name->IsString())
         return false;
+
+    v8::Handle<v8::Message> tryCatchMessage = block.Message();
+    v8::Handle<v8::Value> message = exceptionObject->Get(v8String(isolate, "message"));
+    String messageString;
+    if (!message.IsEmpty() && message->IsString())
+        messageString = toCoreString(v8::Handle<v8::String>::Cast(message));
+
     String exceptionName = toCoreString(v8::Handle<v8::String>::Cast(name));
     if (exceptionName == "DOMExceptionInPrivateScript") {
-        v8::Handle<v8::Value> v8Message = exceptionObject->Get(v8String(isolate, "message"));
-        RELEASE_ASSERT(!v8Message.IsEmpty() && v8Message->IsString());
         v8::Handle<v8::Value> code = exceptionObject->Get(v8String(isolate, "code"));
         RELEASE_ASSERT(!code.IsEmpty() && code->IsInt32());
-        exceptionState.throwDOMException(toInt32(code), toCoreString(v8::Handle<v8::String>::Cast(v8Message)));
+        exceptionState.throwDOMException(toInt32(code), messageString);
         exceptionState.throwIfNeeded();
         return true;
     }
     if (exceptionName == "Error") {
-        v8::Handle<v8::Value> v8Message = exceptionObject->Get(v8String(isolate, "message"));
-        RELEASE_ASSERT(!v8Message.IsEmpty() && v8Message->IsString());
-        String message = toCoreString(v8::Handle<v8::String>::Cast(v8Message));
-        exceptionState.throwDOMException(V8GeneralError, message);
+        exceptionState.throwDOMException(V8GeneralError, messageString);
         exceptionState.throwIfNeeded();
-        dumpJSError(exceptionName, message);
+        dumpJSError(exceptionName, tryCatchMessage);
         return true;
     }
     if (exceptionName == "TypeError") {
-        v8::Handle<v8::Value> v8Message = exceptionObject->Get(v8String(isolate, "message"));
-        RELEASE_ASSERT(!v8Message.IsEmpty() && v8Message->IsString());
-        String message = toCoreString(v8::Handle<v8::String>::Cast(v8Message));
-        exceptionState.throwDOMException(V8TypeError, message);
+        exceptionState.throwDOMException(V8TypeError, messageString);
         exceptionState.throwIfNeeded();
-        dumpJSError(exceptionName, message);
+        dumpJSError(exceptionName, tryCatchMessage);
         return true;
     }
     if (exceptionName == "RangeError") {
-        v8::Handle<v8::Value> v8Message = exceptionObject->Get(v8String(isolate, "message"));
-        RELEASE_ASSERT(!v8Message.IsEmpty() && v8Message->IsString());
-        String message = toCoreString(v8::Handle<v8::String>::Cast(v8Message));
-        exceptionState.throwDOMException(V8RangeError, message);
+        exceptionState.throwDOMException(V8RangeError, messageString);
         exceptionState.throwIfNeeded();
-        dumpJSError(exceptionName, message);
+        dumpJSError(exceptionName, tryCatchMessage);
         return true;
     }
     if (exceptionName == "SyntaxError") {
-        v8::Handle<v8::Value> v8Message = exceptionObject->Get(v8String(isolate, "message"));
-        RELEASE_ASSERT(!v8Message.IsEmpty() && v8Message->IsString());
-        String message = toCoreString(v8::Handle<v8::String>::Cast(v8Message));
-        exceptionState.throwDOMException(V8SyntaxError, message);
+        exceptionState.throwDOMException(V8SyntaxError, messageString);
         exceptionState.throwIfNeeded();
-        dumpJSError(exceptionName, message);
+        dumpJSError(exceptionName, tryCatchMessage);
         return true;
     }
     if (exceptionName == "ReferenceError") {
-        v8::Handle<v8::Value> v8Message = exceptionObject->Get(v8String(isolate, "message"));
-        RELEASE_ASSERT(!v8Message.IsEmpty() && v8Message->IsString());
-        String message = toCoreString(v8::Handle<v8::String>::Cast(v8Message));
-        exceptionState.throwDOMException(V8ReferenceError, message);
+        exceptionState.throwDOMException(V8ReferenceError, messageString);
         exceptionState.throwIfNeeded();
-        dumpJSError(exceptionName, message);
+        dumpJSError(exceptionName, tryCatchMessage);
         return true;
     }
     return false;