Issue 2429, core implementation and the protocol change

Issue 2429, core implementation and the protocol change

Committed: http://code.google.com/p/v8/source/detail?r=13123

[Peter Rybin, 2012-11-27 17:58:06]

Hi Yang

I'm adding an optional "error_details" field to the protocol.
LiveEdit compiler failure uses it to report compilation error details.

Peter

[Yang, 2012-11-28 15:51:28]

I think we can solve the problem with trying to get line numbers more elegantly,
without going to V8::TryCatch. I think I didn't properly understood your use
case when you asked me one or two weeks ago about this.

If you take a look at Runtime_CompileString... it implements javascript's eval
and basically has to solve the same problem. Running the script
try {
  eval("syntax_error");
} catch (e) {
  print(e.stack);
}

we can still get the line numbers of the syntax error.

If I'm not mistaken, the GatherCompileInfo javascript function calls into
Runtime_LiveEditGatherCompileInfo, eventually calling
LiveEdit::GatherCompileInfo.

What you could do here is just leaving this as is, and at the call site of
GatherCompileInfo catch the potential syntax error, and read the line numbers
from the stack trace.

To get the line numbers as integers without having to tediously parse the
'stack' property, you could define your own formatting function. See
http://code.google.com/p/v8/wiki/JavaScriptStackTraceApi

Short story is: if you override Error.prepareStackTrace with your own function,
you can define what ends up in the 'stack' property of the error object. You
could have it return an object that contains the line numbers you need. I admit
that there is probably no easy way to find out end position of the syntax error
though, only the start position.

Would that be something you could work with?

Also, a test would be really nice :)

https://codereview.chromium.org/11421100/diff/2001/src/liveedit-debugger.js
File src/liveedit-debugger.js (right):

https://codereview.chromium.org/11421100/diff/2001/src/liveedit-debugger.js#n...
src/liveedit-debugger.js:86: failure.details = details;
I'd put this after the following if, even though this doesn't really make a
difference.

[Peter Rybin, 2012-11-28 20:12:30]

I must be missing something, because I couldn't reproduce your technique.

I tried:

  var stack_text;
  try {
    eval(")");
  } catch (e) {
    stack_text = e.stack;
  }

The string in eval clearly has syntax error. In stack_text I receive
"SyntaxError: Unexpected token )", but than the stack text goes that describes a
stack up to the point where 'eval' is called.

Similarly, Chrome DevTools console shows:
(function A(){try{eval("]");}catch(e){return e.stack;}})()

"SyntaxError: Unexpected token ]
    at A (eval at <anonymous> (eval at evaluate (unknown source)),
<anonymous>:2:19)
    at eval (eval at <anonymous> (eval at evaluate (unknown source)),
<anonymous>:2:57)
    at eval (eval at evaluate (unknown source), <anonymous>:1:40)
    at FrameMirror.evaluate (native)
    at Object.evaluate (<anonymous>:273:28)
    at Object.InjectedScript._evaluateOn (<anonymous>:515:39)
    at Object.InjectedScript._evaluateAndWrap (<anonymous>:474:52)
    at Object.InjectedScript.evaluateOnCallFrame (<anonymous>:558:21)
    at hsv2rgb
(http://img.artlebedev.ru/;-)/build/1347892747/js/artlebedev/colors.js:24:5)
    at SetLogoColor
(http://img.artlebedev.ru/;-)/build/1347892747/js/artlebedev/colors.js:57:53)"

I'm not sure I see how I can get compile error line/column from this.

What also looks suspicious to me is that prepareStackTrace accepts array of
CallSite objects, but the only implementation of CallSite I found is based on
function value (in messages.js), but you cannot build CallSite for compile
error, because you don't have a function.

[Yang, 2012-11-29 16:38:57]

LGTM, but I got some suggestions. And a test case would be very nice.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit-debugger.js
File src/liveedit-debugger.js (right):

https://codereview.chromium.org/11421100/diff/2001/src/liveedit-debugger.js#n...
src/liveedit-debugger.js:86: failure.details = details;
On 2012/11/28 15:51:28, Yang wrote:
> I'd put this after the following if, even though this doesn't really make a
> difference.

I still think this would make sense.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc
File src/liveedit.cc (right):

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc#newcode943
src/liveedit.cc:943: isolate->clear_pending_exception();
If I'm not mistaken, ReportPendingMessages() already clears the pending
exception and the pending message.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc#newcode946
src/liveedit.cc:946: if (!try_catch.Exception().IsEmpty()) {
use try_catch.HasCaught() seems cleaner.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc#newcode951
src/liveedit.cc:951: if (exception->IsJSObject() &&
!try_catch.Message().IsEmpty()) {
It seems that GetLineNumber() and the Get*Column() methods call into javascript
to calculate those from start position and end position.

I suggest attaching only  message->script(), message->start_position() and
message->end_position() to the error object, and use script.locationFromPosition
to compute the rest when the exception is passed into javascript.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc#newcode984
src/liveedit.cc:984: if (rethrow_exception.is_null()) {
Would it be possible to use try_catch.ReThrow()?

[Peter Rybin, 2012-11-29 23:16:05]

I learned this time that ReportPendingMessages does too many additional things
are rewrote it a bit again.

Now I only use TryCatch as a marker message object is requested.
I get location directly from isolate and pass script/start_position/end_position
to JavaScript as you suggested.

I also added a test.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit-debugger.js
File src/liveedit-debugger.js (right):

https://codereview.chromium.org/11421100/diff/2001/src/liveedit-debugger.js#n...
src/liveedit-debugger.js:86: failure.details = details;
On 2012/11/29 16:38:57, Yang wrote:
> On 2012/11/28 15:51:28, Yang wrote:
> > I'd put this after the following if, even though this doesn't really make a
> > difference.
> 
> I still think this would make sense.

You mean this only line?
Done

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc
File src/liveedit.cc (right):

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc#newcode943
src/liveedit.cc:943: isolate->clear_pending_exception();
On 2012/11/29 16:38:57, Yang wrote:
> If I'm not mistaken, ReportPendingMessages() already clears the pending
> exception and the pending message.

It does clear message. I don't see that it clears exception though.
1 line removed.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc#newcode946
src/liveedit.cc:946: if (!try_catch.Exception().IsEmpty()) {
On 2012/11/29 16:38:57, Yang wrote:
> use try_catch.HasCaught() seems cleaner.

Done.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc#newcode951
src/liveedit.cc:951: if (exception->IsJSObject() &&
!try_catch.Message().IsEmpty()) {
On 2012/11/29 16:38:57, Yang wrote:
> It seems that GetLineNumber() and the Get*Column() methods call into
javascript
> to calculate those from start position and end position.
> 
> I suggest attaching only  message->script(), message->start_position() and
> message->end_position() to the error object, and use
script.locationFromPosition
> to compute the rest when the exception is passed into javascript.

Done.

https://codereview.chromium.org/11421100/diff/2001/src/liveedit.cc#newcode984
src/liveedit.cc:984: if (rethrow_exception.is_null()) {
On 2012/11/29 16:38:57, Yang wrote:
> Would it be possible to use try_catch.ReThrow()?

I don't see a nice way to reach TryCatch outside from block scope above.

[Yang, 2012-12-03 17:26:17]

LGTM with one nit.

https://codereview.chromium.org/11421100/diff/9/test/mjsunit/debug-liveedit-c...
File test/mjsunit/debug-liveedit-compile-error.js (right):

https://codereview.chromium.org/11421100/diff/9/test/mjsunit/debug-liveedit-c...
test/mjsunit/debug-liveedit-compile-error.js:48:
Debug.LiveEdit.TestApi.ApplySingleChunkPatch(script, patch_pos,
orig_animal.length, new_animal_patch, change_log);
80 char limit.

[Peter Rybin, 2012-12-03 20:34:38]

https://codereview.chromium.org/11421100/diff/9/test/mjsunit/debug-liveedit-c...
File test/mjsunit/debug-liveedit-compile-error.js (right):

https://codereview.chromium.org/11421100/diff/9/test/mjsunit/debug-liveedit-c...
test/mjsunit/debug-liveedit-compile-error.js:48:
Debug.LiveEdit.TestApi.ApplySingleChunkPatch(script, patch_pos,
orig_animal.length, new_animal_patch, change_log);
On 2012/12/03 17:26:17, Yang wrote:
> 80 char limit.

Done.

[Michael Starzinger, 2012-12-04 09:51:05]

This change is not GC safe.

https://codereview.chromium.org/11421100/diff/3002/src/liveedit.cc
File src/liveedit.cc (right):

https://codereview.chromium.org/11421100/diff/3002/src/liveedit.cc#newcode955
src/liveedit.cc:955: factory->LookupAsciiSymbol("startPosition"),
This pattern is not GC safe. Other handles might have been dereferenced before
the call to LookupAsciiSymbol, which in turn can cause a GC. The factory call
needs to be done separately before a sequence point.

https://codereview.chromium.org/11421100/diff/3002/src/liveedit.cc#newcode959
src/liveedit.cc:959: factory->LookupAsciiSymbol("endPosition"),
Likewise.

https://codereview.chromium.org/11421100/diff/3002/src/liveedit.cc#newcode963
src/liveedit.cc:963: factory->LookupAsciiSymbol("scriptObject"),
Likewise.

[Yang, 2012-12-04 10:04:46]

On 2012/12/04 09:51:05, Michael Starzinger wrote:
> This change is not GC safe.
> 
> https://codereview.chromium.org/11421100/diff/3002/src/liveedit.cc
> File src/liveedit.cc (right):
> 
> https://codereview.chromium.org/11421100/diff/3002/src/liveedit.cc#newcode955
> src/liveedit.cc:955: factory->LookupAsciiSymbol("startPosition"),
> This pattern is not GC safe. Other handles might have been dereferenced before
> the call to LookupAsciiSymbol, which in turn can cause a GC. The factory call
> needs to be done separately before a sequence point.
> 
> https://codereview.chromium.org/11421100/diff/3002/src/liveedit.cc#newcode959
> src/liveedit.cc:959: factory->LookupAsciiSymbol("endPosition"),
> Likewise.
> 
> https://codereview.chromium.org/11421100/diff/3002/src/liveedit.cc#newcode963
> src/liveedit.cc:963: factory->LookupAsciiSymbol("scriptObject"),
> Likewise.

I'll fix this.