When rethrowing an exception, print the stack trace of its original site instead of rethrow site.

When rethrowing an exception, print the stack trace of its original site instead of rethrow site.


BUG=60240
Committed: https://code.google.com/p/v8/source/detail?r=10618

[Yang, 2012-02-06 15:19:43]

Second attempt at this.  Behavior is described here:
http://code.google.com/p/chromium/issues/detail?id=60240#c31

In general those changes should not affect performance to begin with, since they
only affect changes for devtools and the Error object.

Two problems I encountered:
- when the error object is constructed, we now have two stack walks. The first
one gathers just a few information into a fixed array for the Error.stack getter
to construct into a string later. The second one gathers more information to be
displayed in devtools. Considering that the latter only kicks in devtools where
we don't care too much about performance, there is no point combining both stack
walks, which would drag down performance of the first stack walk, which is
always being done when an Error object is constructed.
- gathering message location is still only done at the throw site to avoid
confusing and contradicting error messages depending on whether devtools was
open when the error occurred.

[Vyacheslav Egorov (Chromium), 2012-02-06 17:26:25]

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc
File src/isolate.cc (left):

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#oldcode1088
src/isolate.cc:1088: ASSERT(is_object);  // Can't use the handle unless there's
a real object.
I seems that DoThrow currently can't be called with Failure object only with
real Object. 

If this is true: please change DoThrow signature to reflect this and remove
is_object and related checks. 

If this is false: there might be a bug here (ASSERT was not here for nothing).

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode545
src/isolate.cc:545: void
Isolate::CaptureAndSetCurrentStackTraceFor(Handle<Object> error_object) {
Object -> JSObject as we are expect it to be so.

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode552
src/isolate.cc:552: JSObject::cast(*error_object)->SetHiddenProperty(key,
stack_trace);
I think this is point of potential allocation failure which requires handling.

(it seems we forgot to annotate SetHiddenProperty with MUST_USE_RESULT to
capture it, please annotate it).

You can use static method JSObject::SetHiddenProperty instead.

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1098
src/isolate.cc:1098: Handle<Object> message_obj;
Now can be moved into the if where is needed.

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1116
src/isolate.cc:1116: String* key =
*(factory()->LookupAsciiSymbol("stackTrace"));
"stackTrace" duplicated in two places. 

Consider moving somewhere (e.g. to roots as symbol, see identity_hash_symbol for
example).

Also consider prefixing it with v8::

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1119
src/isolate.cc:1119: stack_trace_object =
Handle<JSArray>(JSArray::cast(stack_property));
consider code like:

throw {__proto__: new Error()};

there will be no hidden property stackTrace on the exception itself.

http://codereview.chromium.org/9310122/diff/2002/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9310122/diff/2002/src/runtime.cc#newcode13267
src/runtime.cc:13267: Handle<Object> error_object = args.at<Object>(0);
CONVERT_ARG_CHECKED(JSObject, error_object, 0);

just to be sure that we are getting JSObject.

[Yang, 2012-02-07 08:33:58]

Issues addressed. Another brief look?

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc
File src/isolate.cc (left):

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#oldcode1088
src/isolate.cc:1088: ASSERT(is_object);  // Can't use the handle unless there's
a real object.
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
> I seems that DoThrow currently can't be called with Failure object only with
> real Object. 
> 
> If this is true: please change DoThrow signature to reflect this and remove
> is_object and related checks. 
> 
> If this is false: there might be a bug here (ASSERT was not here for nothing).

Must be some ancient reason. I checked and there is no caller that uses
MaybeObject (anymore).

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode545
src/isolate.cc:545: void
Isolate::CaptureAndSetCurrentStackTraceFor(Handle<Object> error_object) {
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
> Object -> JSObject as we are expect it to be so.

Done.

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode552
src/isolate.cc:552: JSObject::cast(*error_object)->SetHiddenProperty(key,
stack_trace);
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
> I think this is point of potential allocation failure which requires handling.
> 
> (it seems we forgot to annotate SetHiddenProperty with MUST_USE_RESULT to
> capture it, please annotate it).
> 
> You can use static method JSObject::SetHiddenProperty instead.

Done.

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1098
src/isolate.cc:1098: Handle<Object> message_obj;
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
> Now can be moved into the if where is needed.

Done.

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1116
src/isolate.cc:1116: String* key =
*(factory()->LookupAsciiSymbol("stackTrace"));
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
> "stackTrace" duplicated in two places. 
> 
> Consider moving somewhere (e.g. to roots as symbol, see identity_hash_symbol
for
> example).
> 
> Also consider prefixing it with v8::

Done. How about v8::hidden_stack_trace?

http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1119
src/isolate.cc:1119: stack_trace_object =
Handle<JSArray>(JSArray::cast(stack_property));
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
> consider code like:
> 
> throw {__proto__: new Error()};
> 
> there will be no hidden property stackTrace on the exception itself.

Done. In that case we resort to capturing at throw site. A cctest has also been
added.

http://codereview.chromium.org/9310122/diff/2002/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9310122/diff/2002/src/runtime.cc#newcode13267
src/runtime.cc:13267: Handle<Object> error_object = args.at<Object>(0);
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
> CONVERT_ARG_CHECKED(JSObject, error_object, 0);
> 
> just to be sure that we are getting JSObject.

Done.

[Vyacheslav Egorov (Chromium), 2012-02-07 09:17:37]

lgtm

http://codereview.chromium.org/9310122/diff/7001/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9310122/diff/7001/src/isolate.cc#newcode548
src/isolate.cc:548: Handle<String> key(heap()->hidden_stack_trace_symbol());
you can use factory()->hidden_stack_trace_symbol(),
it returns handle.

http://codereview.chromium.org/9310122/diff/7001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/9310122/diff/7001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:13655: // not where it is thrown.
Comment is incorrect. In this test stack trace is captured where it is thrown.

[Yang, 2012-02-07 09:30:55]

On 2012/02/07 09:17:37, Vyacheslav Egorov wrote:
> lgtm
> 
> http://codereview.chromium.org/9310122/diff/7001/src/isolate.cc
> File src/isolate.cc (right):
> 
> http://codereview.chromium.org/9310122/diff/7001/src/isolate.cc#newcode548
> src/isolate.cc:548: Handle<String> key(heap()->hidden_stack_trace_symbol());
> you can use factory()->hidden_stack_trace_symbol(),
> it returns handle.
> 
> http://codereview.chromium.org/9310122/diff/7001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
http://codereview.chromium.org/9310122/diff/7001/test/cctest/test-api.cc#newc...
> test/cctest/test-api.cc:13655: // not where it is thrown.
> Comment is incorrect. In this test stack trace is captured where it is thrown.

Issues addressed. Landing.