JS errors thrown in private scripts should be reported to stderr in Debug builds

Source/bindings/templates/methods.cpp

 {##############################################################################}
 {% macro generate_method(method, world_suffix) %}
 {% filter conditional(method.conditional_string) %}
 static void {{method.name}}{{method.overload_index}}Method{{world_suffix}}(const v8::FunctionCallbackInfo<v8::Value>& info)
 {
     {# Local variables #}
     {% if method.has_exception_state %}
     ExceptionState exceptionState(ExceptionState::ExecutionContext, "{{method.name}}", "{{interface_name}}", info.Holder(), info.GetIsolate());
     {% endif %}
     {# Overloaded methods have length checked during overload resolution #}
@@ skipping 507 matching lines @@
     v8::Handle<v8::Value> {{argument.handle}} = {{argument.private_script_cpp_value_to_v8_value}};
     {% endfor %}
     {% if method.arguments %}
     v8::Handle<v8::Value> argv[] = { {{method.arguments | join(', ', 'handle')}} };
     {% else %}
     {# Empty array initializers are illegal, and don\'t compile in MSVC. #}
     v8::Handle<v8::Value> *argv = 0;
     {% endif %}
     ExceptionState exceptionState(ExceptionState::ExecutionContext, "{{method.name}}", "{{cpp_class}}", scriptState->context()->Global(), scriptState->isolate());
     v8::TryCatch block;
+#ifndef NDEBUG
+    block.SetVerbose(true);
+#endif
     V8RethrowTryCatchScope rethrow(block);

[Jens Widell, 2014/08/11 09:26:15]

Unrelated: Using a V8RethrowTryCatchScope here is a bit weird, actually, since
any exception initially caught by this TryCatch must explicitly *not* be
rethrown; we either throw another exception, or die. Having it here doesn't
cause any wrong behavior, but perhaps it would be clearer in this particular
case to have an explicit block.ReThrow() once we've thrown the correct
exception?

[haraken, 2014/08/11 09:41:16]

Makes a lot of sense. Will fix in a separate CL shortly.

     {% if method.idl_type == 'void' %}
     PrivateScriptRunner::runDOMMethod(scriptState, "{{cpp_class}}", "{{method.name}}", holder, {{method.arguments | length}}, argv);
     if (block.HasCaught()) {

[Jens Widell, 2014/08/11 09:26:15]

Should we have block.SetVerbose(false) here too? Otherwise, won't the "real"
exception (thrown back to the script) also get reported, so that you get two
reports for every (non-"internal error") exception thrown in a private script?

[haraken, 2014/08/11 09:41:16]

In my understanding, it will happen, so I limited the SetVerbose() to only Debug
builds. If we set SetVerbose(), V8 reports a verbose exception. Then the
exception bubbles up to an external handler and the external handler might
report the exception again.

Actually I don't fully understand when V8 throws the verbose exception. Even if
we set SetVerbose(false) here, isn't it too late? (I mean, isn't the verbose
exception already reported by V8?)

[Jens Widell, 2014/08/11 09:52:47]

I got the impression (having for the first time looked at the involved code in
v8/src/isolate.cc) that V8 looks at the flag on the top-most TryCatch each time
an exception is thrown, and schedules the message right then if the flag is set.

IOW, it looks to me like calling SetVerbose(false) before
PrivateScriptRunner::throwDOMExceptionInPrivateScriptIfNeeded() could affect
behavior, at least.

[haraken, 2014/08/11 11:56:41]

Thanks for looking into the details!

However, I guess we anyway cannot call SetVerbose(false) here. If we call
SetVerbose(false), this CL has no meaning. What this CL wants to do is to dump
all exceptions thrown in private scripts into console. For example, imagine that
user's script has the following code:

try {
  func();  // This calls a private script.
} catch (e) {
}

Then typo that happens in the private script won't be reported anywhere.

Another example is a case where C++ invokes a private script using a
V8X::PrivateScript::xxx() method. In this case, an exception thrown in
V8X::PrivateScript::xxx() is not reported to the console until (I think) the
next Execution::invoke() runs in V8 (i.e., next Script::Run() or
Function::Call() is called in V8).

This is problematic when debugging private scripts, so I want to have V8 always
report such exceptions in Debug builds. (A better idea is welcomed.)

[Jens Widell, 2014/08/11 12:08:28]

I would think it would, but I haven't tested it and could certainly be wrong.
:-)

As long as we have SetVerbose(true) "during" the
PrivateScriptRunner::runDOMMethod() call, I would think any exception thrown by
it is reported to the console, regardless of what the calling user script looks
like.

But if we then clone that exception and throw it still with the TryCatch and
SetVerbose(true) in scope then that will be reported to the console too. That
double reporting is what I suggest we would avoid by calling SetVerbose(false).
I would think the same applies here.

[haraken, 2014/08/11 13:26:20]

Thanks, I understand what you're saying. Given that
throwDOMExceptionInPrivateScriptIfNeeded calls Isolate::ThrowException(), I
agree with you that if we leave the SetVerbose as being true, the
Isolate::ThrowException() will add one more line to the console.

However, strangely, in fact that does not happen. I haven't yet looked into the
details, but I added SetVerbose(false) as you suggested just in case.

         PrivateScriptRunner::throwDOMExceptionInPrivateScriptIfNeeded(scriptState->isolate(), exceptionState, block.Exception());

[Jens Widell, 2014/08/11 09:26:15]

Unrelated: Why aren't we checking the return value of this call?

[haraken, 2014/08/11 09:41:16]

You're right. Will fix in a separate CL shortly.

         return false;
     }
     {% else %}
     v8::Handle<v8::Value> v8Value = PrivateScriptRunner::runDOMMethod(scriptState, "{{cpp_class}}", "{{method.name}}", holder, {{method.arguments | length}}, argv);
     if (block.HasCaught()) {
         if (!PrivateScriptRunner::throwDOMExceptionInPrivateScriptIfNeeded(scriptState->isolate(), exceptionState, block.Exception())) {
             // FIXME: We should support exceptions other than DOM exceptions.
             RELEASE_ASSERT_NOT_REACHED();
         }
         return false;
@@ skipping 49 matching lines @@
 v8::Handle<v8::Object> wrapper = wrap(impl.get(), info.Holder(), info.GetIsolate());
 {% else %}
 {% set constructor_class = v8_class + ('Constructor'
                                        if constructor.is_named_constructor else
                                        '') %}
 v8::Handle<v8::Object> wrapper = info.Holder();
 V8DOMWrapper::associateObjectWithWrapper<{{v8_class}}>(impl.release(), &{{constructor_class}}::wrapperTypeInfo, wrapper, info.GetIsolate(), {{wrapper_configuration}});
 {% endif %}
 v8SetReturnValue(info, wrapper);
 {% endmacro %}