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

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

In order to debug private scripts, JS errors thrown in the private scripts
(e.g., ReferenceError thrown when we make a typo in private scripts)
need to be reported to stderr. This CL makes the change.

The fprintf(stderr, ...) is enabled only in Debug builds.

This CL also renames throwDOMExceptionInPrivateScriptIfNeeded to rethrowExceptionInPrivateScript.

BUG=341031
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180033

[haraken, 2014-08-11 09:05:57]

jl@ or yhirano@: PTAL.

[haraken, 2014-08-11 09:07:07]

(hoshi-san: This CL will fix the issue you're facing. Typo in private scripts
will trigger a console error in Debug builds.)

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

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:531: V8RethrowTryCatchScope
rethrow(block);
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?

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:534: if (block.HasCaught()) {
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?

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:535:
PrivateScriptRunner::throwDOMExceptionInPrivateScriptIfNeeded(scriptState->isolate(),
exceptionState, block.Exception());
Unrelated: Why aren't we checking the return value of this call?

[yhirano, 2014-08-11 09:27:13]

If SetVerbose is enabled only in DEBUG builds, do you have separate expectation
files for one layout test?

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

Thanks for quick review!

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:531: V8RethrowTryCatchScope
rethrow(block);
On 2014/08/11 09:26:15, Jens Lindström wrote:
> 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?

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

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:534: if (block.HasCaught()) {
On 2014/08/11 09:26:15, Jens Lindström wrote:
> 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?

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?)

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:535:
PrivateScriptRunner::throwDOMExceptionInPrivateScriptIfNeeded(scriptState->isolate(),
exceptionState, block.Exception());
On 2014/08/11 09:26:15, Jens Lindström wrote:
> Unrelated: Why aren't we checking the return value of this call?

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

[haraken, 2014-08-11 09:42:18]

On 2014/08/11 09:27:13, yhirano wrote:
> If SetVerbose is enabled only in DEBUG builds, do you have separate
expectation
> files for one layout test?

The private-script-unittest.html is enabled only on Debug builds (because
PrivateScriptTest.js is compiled and integrated into the binary of Debug builds
only).

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

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:534: if (block.HasCaught()) {
On 2014/08/11 09:41:16, haraken wrote:
> On 2014/08/11 09:26:15, Jens Lindström wrote:
> > 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?
> 
> 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?)

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]

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:534: if (block.HasCaught()) {
On 2014/08/11 09:52:47, Jens Lindström wrote:
> On 2014/08/11 09:41:16, haraken wrote:
> > On 2014/08/11 09:26:15, Jens Lindström wrote:
> > > 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?
> > 
> > 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?)
> 
> 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.

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]

LGTM

The SetVerbose(false) issue is purely cosmetic, and only affects debug builds
anyway, and might not exist at all, so no reason for it to block this.

If it does exist and turns out to cause unwanted noise, we can always look into
it later.

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:534: if (block.HasCaught()) {
On 2014/08/11 11:56:41, haraken wrote:
> 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.

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).

> 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).

I would think the same applies here.

> 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.)

[vivekg, 2014-08-11 12:10:33]

lgtm, thanks! This would avoid random crashes due to typo's in the
PrivateScripts and report the verbose error.

[yhirano, 2014-08-11 12:21:16]

lgtm

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

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/460573002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:534: if (block.HasCaught()) {
On 2014/08/11 12:08:28, Jens Lindström wrote:
> On 2014/08/11 11:56:41, haraken wrote:
> > 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.
> 
> 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).
> 
> > 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).
> 
> I would think the same applies here.
> 
> > 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.)

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.

[haraken, 2014-08-11 13:26:52]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-11 13:27:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/460573002/20001

[Jens Widell, 2014-08-11 14:03:59]

On 2014/08/11 13:26:20, haraken wrote:
> 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.

Looking at Isolate::ReportPendingMessages() (and its callers) I suspect in
practice a message is only reported to the console if you call into V8 and the
result is an exception that is caught by your verbose TryCatch.

If you throw an exception using Isolate::ThrowException and it's caught by the
TryCatch, the verbosity flag is used and a message is created and stored in
ThreadLocalTop::pending_message_obj_ as if it was going to be reported, but I
suspect it never will be. The function Isolate::ScheduleThrow() (in
v8/src/isolate.cc) will clear the pending exception (but not the pending
message) in this case, and without a pending exception,
Isolate::ReportPendingMessages() is never called (it begins with an assert that
there is a pending exception.)

[commit-bot: I haz the power, 2014-08-11 14:29:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19292)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22061)

[commit-bot: I haz the power, 2014-08-11 14:55:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-11 14:55:42]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[haraken, 2014-08-12 01:55:36]

Hmm, yhirano@ is right...

With this CL applied, marquee-element.html outputs different test results for
Release builds and Debug builds when exceptions are thrown. This is problematic.

Instead of using SetVerbose(true), probably I can just output the exception to
stderr only in Debug builds. Then it won't affect test results.

[haraken, 2014-08-12 02:38:33]

jl@: Updated the CL and the CL description. Would you take another look again?

I decided to use fprintf(stderr), instead of SetVerbose(true).

https://codereview.chromium.org/460573002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/460573002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/PrivateScriptRunner.cpp:212: fprintf(stderr, "Private
script throws an exception: %s\n", exceptionName.utf8().data());

I used fprintf(stderr) instead of WTF_LOG_ERROR, because WTF_LOG_ERROR prints
__FILE__ and __LINE__ of this code, like this:


ERROR: Private script throws an exception: ReferenceError

../../third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp(212) :
void blink::dumpJSError(WTF::String, WTF::String)
ERROR: bbb is not defined

../../third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp(214) :
void blink::dumpJSError(WTF::String, WTF::String)

[Jens Widell, 2014-08-12 04:38:31]

LGTM

Not: it's a bit messy to review patch-sets that both rebase and adds new
changes. :-)

[haraken, 2014-08-12 05:03:34]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-12 05:03:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/460573002/40001

[haraken, 2014-08-12 05:04:04]

> Not: it's a bit messy to review patch-sets that both rebase and adds new
> changes. :-)

Yeah, I should have uploaded a new issue.

[commit-bot: I haz the power, 2014-08-12 05:17:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22187)

[commit-bot: I haz the power, 2014-08-12 05:53:36]

Change committed as 180033

[Jens Widell, 2014-08-12 06:30:57]

On 2014/08/12 05:04:04, haraken wrote:
> > Not: it's a bit messy to review patch-sets that both rebase and adds new
> > changes. :-)
> 
> Yeah, I should have uploaded a new issue.

Or just upload the rebase and the new changes as two separate patch-sets.