Add OnCompileError handler and v8::CompileError debug event

Add OnCompileError handler and v8::CompileError debug event.
This event is generated when the parser can not generate code.

R=vsevik@chromium.org, yangguo@chromium.org, yurys@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22043

[vsevik, 2014-06-06 16:12:01]

https://chromiumcodereview.appspot.com/264333007/diff/20001/include/v8-debug.h
File include/v8-debug.h (right):

https://chromiumcodereview.appspot.com/264333007/diff/20001/include/v8-debug....
include/v8-debug.h:25: ScriptFailedToParse = 9
SyntaxError

https://chromiumcodereview.appspot.com/264333007/diff/20001/src/debug.cc
File src/debug.cc (right):

https://chromiumcodereview.appspot.com/264333007/diff/20001/src/debug.cc#newc...
src/debug.cc:2705: // Save breakpoints or not, if SyntaxError ocured?
I don't think we should care about breakpoints in a script with syntax error.

https://chromiumcodereview.appspot.com/264333007/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/264333007/diff/20001/src/isolate.cc#ne...
src/isolate.cc:1003: bool Isolate::IsTypeObject(Handle<Object> obj,
Handle<String> key) {
IsObjectOfType

https://chromiumcodereview.appspot.com/264333007/diff/20001/src/isolate.cc#ne...
src/isolate.cc:1003: bool Isolate::IsTypeObject(Handle<Object> obj,
Handle<String> key) {
key -> js_builtin_key

[kozyatinskiy1, 2014-06-09 12:59:33]

On 2014/06/06 16:12:01, vsevik wrote:
> https://chromiumcodereview.appspot.com/264333007/diff/20001/include/v8-debug.h
> File include/v8-debug.h (right):
> 
>
https://chromiumcodereview.appspot.com/264333007/diff/20001/include/v8-debug....
> include/v8-debug.h:25: ScriptFailedToParse = 9
> SyntaxError
Done.
 
> https://chromiumcodereview.appspot.com/264333007/diff/20001/src/debug.cc
> File src/debug.cc (right):
> 
>
https://chromiumcodereview.appspot.com/264333007/diff/20001/src/debug.cc#newc...
> src/debug.cc:2705: // Save breakpoints or not, if SyntaxError ocured?
> I don't think we should care about breakpoints in a script with syntax error.
I think so too. Deleted this code.
 
> https://chromiumcodereview.appspot.com/264333007/diff/20001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/264333007/diff/20001/src/isolate.cc#ne...
> src/isolate.cc:1003: bool Isolate::IsTypeObject(Handle<Object> obj,
> Handle<String> key) {
> IsObjectOfType
Done.

 
>
https://chromiumcodereview.appspot.com/264333007/diff/20001/src/isolate.cc#ne...
> src/isolate.cc:1003: bool Isolate::IsTypeObject(Handle<Object> obj,
> Handle<String> key) {
> key -> js_builtin_key
Renamed to type.

Added test.

[kozyatinskiy1, 2014-06-10 17:07:17]

Moved OnSyntaxError call to Parser.
Removed AfterCompile flags.

[vsevik, 2014-06-10 17:28:05]

Looks good to me.

[vsevik, 2014-06-10 17:28:32]

Looks good to me.

yurys@, yangguo@, Could you please take a look on that?

[vsevik, 2014-06-11 11:47:16]

lgtm

[yurys, 2014-06-11 11:52:14]

lgtm

https://codereview.chromium.org/264333007/diff/150001/src/debug.h
File src/debug.h (left):

https://codereview.chromium.org/264333007/diff/150001/src/debug.h#oldcode368
src/debug.h:368: NO_AFTER_COMPILE_FLAGS,
It might make sense to do this clean-up in a separate change.

[kozyatinskiy1, 2014-06-11 12:02:28]

Yang, could you do OWNER review?

[Yang, 2014-06-11 14:33:37]

https://codereview.chromium.org/264333007/diff/150001/src/debug.cc
File src/debug.cc (left):

https://codereview.chromium.org/264333007/diff/150001/src/debug.cc#oldcode2705
src/debug.cc:2705: if (was_in_scope && (after_compile_flags &
SEND_WHEN_DEBUGGING) == 0) return;
Is it guaranteed that when this method is called from CompileToplevel, we are
not in the debug scope? If not, this will change behavior.

https://codereview.chromium.org/264333007/diff/150001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/264333007/diff/150001/src/parser.cc#newcode3863
src/parser.cc:3863: isolate()->Throw(*result, &location);
Won't Debug::OnException trigger as well? The call to Debug::OnCompileError
doesn't actually contain the syntax error location, right? So you would need the
following Debug::OnException to get the actual syntax error location, right? A
split API like that is kind of weird.

[kozyatinskiy1, 2014-06-11 15:07:22]

On 2014/06/11 14:33:37, Yang wrote:
> https://codereview.chromium.org/264333007/diff/150001/src/debug.cc
> File src/debug.cc (left):
> 
> https://codereview.chromium.org/264333007/diff/150001/src/debug.cc#oldcode2705
> src/debug.cc:2705: if (was_in_scope && (after_compile_flags &
> SEND_WHEN_DEBUGGING) == 0) return;
> Is it guaranteed that when this method is called from CompileToplevel, we are
> not in the debug scope? If not, this will change behavior.
There are next three lines of code (2267-2671) in base version:
if (in_debug_scope() || ignore_events()) return;
HandleScope scope(isolate_);
bool was_in_scope = in_debug_scope();

If in_debug_scope() is true, then we will leave the function and do nothing.
Otherwise, was_in_scope is false, and condition (was_in_scope &&
(after_compile_flags & SEND_WHEN_DEBUGGING) == 0) is false, too.
Variable was_in_scope is always false.

Could behavior was changed earlier patches?

> 
> https://codereview.chromium.org/264333007/diff/150001/src/parser.cc
> File src/parser.cc (right):
> 
>
https://codereview.chromium.org/264333007/diff/150001/src/parser.cc#newcode3863
> src/parser.cc:3863: isolate()->Throw(*result, &location);
> Won't Debug::OnException trigger as well? The call to Debug::OnCompileError
> doesn't actually contain the syntax error location, right? So you would need
the
> following Debug::OnException to get the actual syntax error location, right? A
> split API like that is kind of weird.
Yes, I need the following Debug::OnException. But it is important to distinguish
between an Exception like SyntaxError and compilation errors. Event CompileError
dual event AfterCompile and provides information about the script to debugger.

[Yang, 2014-06-11 15:14:44]

On 2014/06/11 15:07:22, kozyatinskiy wrote:
> On 2014/06/11 14:33:37, Yang wrote:
> > https://codereview.chromium.org/264333007/diff/150001/src/debug.cc
> > File src/debug.cc (left):
> > 
> >
https://codereview.chromium.org/264333007/diff/150001/src/debug.cc#oldcode2705
> > src/debug.cc:2705: if (was_in_scope && (after_compile_flags &
> > SEND_WHEN_DEBUGGING) == 0) return;
> > Is it guaranteed that when this method is called from CompileToplevel, we
are
> > not in the debug scope? If not, this will change behavior.
> There are next three lines of code (2267-2671) in base version:
> if (in_debug_scope() || ignore_events()) return;
> HandleScope scope(isolate_);
> bool was_in_scope = in_debug_scope();
> 
> If in_debug_scope() is true, then we will leave the function and do nothing.
> Otherwise, was_in_scope is false, and condition (was_in_scope &&
> (after_compile_flags & SEND_WHEN_DEBUGGING) == 0) is false, too.
> Variable was_in_scope is always false.
> 
> Could behavior was changed earlier patches?

I see. Makes sense.


> 
> > 
> > https://codereview.chromium.org/264333007/diff/150001/src/parser.cc
> > File src/parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/264333007/diff/150001/src/parser.cc#newcode3863
> > src/parser.cc:3863: isolate()->Throw(*result, &location);
> > Won't Debug::OnException trigger as well? The call to Debug::OnCompileError
> > doesn't actually contain the syntax error location, right? So you would need
> the
> > following Debug::OnException to get the actual syntax error location, right?
A
> > split API like that is kind of weird.
> Yes, I need the following Debug::OnException. But it is important to
distinguish
> between an Exception like SyntaxError and compilation errors. Event
CompileError
> dual event AfterCompile and provides information about the script to debugger.

Now I'm a bit confused. Aren't syntax errors a subset of compilation errors? In
any case, wouldn't you know the current script object from the previous
OnBeforeCompile event? It sounds to me that what we ideally want is to have the
script object piggybacked onto the OnException event.

[vsevik, 2014-06-20 08:36:17]

>
https://codereview.chromium.org/264333007/diff/150001/src/parser.cc#newcode3863
> > > src/parser.cc:3863: isolate()->Throw(*result, &location);
> > > Won't Debug::OnException trigger as well? The call to
Debug::OnCompileError
> > > doesn't actually contain the syntax error location, right? So you would
need
> > the
> > > following Debug::OnException to get the actual syntax error location,
right?
> A
> > > split API like that is kind of weird.
> > Yes, I need the following Debug::OnException. But it is important to
> distinguish
> > between an Exception like SyntaxError and compilation errors. Event
> CompileError
> > dual event AfterCompile and provides information about the script to
debugger.
> 
> Now I'm a bit confused. Aren't syntax errors a subset of compilation errors?
In
> any case, wouldn't you know the current script object from the previous
> OnBeforeCompile event? It sounds to me that what we ideally want is to have
the
> script object piggybacked onto the OnException event.

First of all Debug::OnException might not trigger (it is only used for "pause on
exception" functionality, so in general case it is turned off.
See
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/debug.cc&q=...

Then there could some exotic cases like 
try {
  eval("}")
} catch (e) {}
where syntax errors are not reported to the embedder at all.

So if we want to distinguish between compiled scripts and scripts where
compilation failed we need an explicit signal about compilation failures.

[vsevik, 2014-06-24 14:31:54]

Friendly ping

[Yang, 2014-06-25 07:26:23]

Almost there!

https://codereview.chromium.org/264333007/diff/150001/include/v8-debug.h
File include/v8-debug.h (right):

https://codereview.chromium.org/264333007/diff/150001/include/v8-debug.h#newc...
include/v8-debug.h:24: CompileError = 8
We have the same table (without BrekaForCommand) in debug-debugger.js. I do
think this new one needs to be there as well so that the debug event listener
can distinguish the events. Also, please make CompileError=7 and
BreakForCommand=8 (the latter is not exposed to the debug event listener).

If possible, a test case for a JS debug event listener would be great.

[kozyatinskiy1, 2014-06-25 15:25:08]

On 2014/06/11 11:52:14, yurys wrote:
> https://codereview.chromium.org/264333007/diff/150001/src/debug.h
> File src/debug.h (left):
> 
> https://codereview.chromium.org/264333007/diff/150001/src/debug.h#oldcode368
> src/debug.h:368: NO_AFTER_COMPILE_FLAGS,
> It might make sense to do this clean-up in a separate change.

Extracted to https://codereview.chromium.org/353823004/

On 2014/06/25 07:26:23, Yang wrote:
> https://codereview.chromium.org/264333007/diff/150001/include/v8-debug.h
> File include/v8-debug.h (right):
> 
>
https://codereview.chromium.org/264333007/diff/150001/include/v8-debug.h#newc...
> include/v8-debug.h:24: CompileError = 8
> We have the same table (without BrekaForCommand) in debug-debugger.js. I do
> think this new one needs to be there as well so that the debug event listener
> can distinguish the events. Also, please make CompileError=7 and
> BreakForCommand=8 (the latter is not exposed to the debug event listener).
Done.
 
> If possible, a test case for a JS debug event listener would be great.
Modified existing test: debug-compile-event.js.

[Yang, 2014-06-26 10:25:39]

LGTM with one last comment.

https://codereview.chromium.org/264333007/diff/230001/test/mjsunit/debug-comp...
File test/mjsunit/debug-compile-event.js (right):

https://codereview.chromium.org/264333007/diff/230001/test/mjsunit/debug-comp...
test/mjsunit/debug-compile-event.js:121: assertEquals(before_compile_count,
after_compile_count + compile_error_count);
Can we also assert the value of compile_error_count, just to make sure it does
not arbitrarily change in the future?

[kozyatinskiy1, 2014-06-26 11:16:24]

On 2014/06/26 10:25:39, Yang wrote:
>
https://codereview.chromium.org/264333007/diff/230001/test/mjsunit/debug-comp...
> File test/mjsunit/debug-compile-event.js (right):
> 
>
https://codereview.chromium.org/264333007/diff/230001/test/mjsunit/debug-comp...
> test/mjsunit/debug-compile-event.js:121: assertEquals(before_compile_count,
> after_compile_count + compile_error_count);
> Can we also assert the value of compile_error_count, just to make sure it does
> not arbitrarily change in the future?
Done.

[Yang, 2014-06-26 11:17:12]

On 2014/06/26 11:16:24, kozyatinskiy wrote:
> On 2014/06/26 10:25:39, Yang wrote:
> >
>
https://codereview.chromium.org/264333007/diff/230001/test/mjsunit/debug-comp...
> > File test/mjsunit/debug-compile-event.js (right):
> > 
> >
>
https://codereview.chromium.org/264333007/diff/230001/test/mjsunit/debug-comp...
> > test/mjsunit/debug-compile-event.js:121: assertEquals(before_compile_count,
> > after_compile_count + compile_error_count);
> > Can we also assert the value of compile_error_count, just to make sure it
does
> > not arbitrarily change in the future?
> Done.

lgtm.

[yurys, 2014-06-26 16:04:02]

Committed patchset #8 manually as r22043 (presubmit successful).