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

src/isolate.cc

Index: src/isolate.cc
diff --git a/src/isolate.cc b/src/isolate.cc
index 893a344b511e22818362a0dd748c8c1dec280948..3914c68d762749a059e93bd63d34819f3d112e41 100644
--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -542,6 +542,18 @@ Handle<String> Isolate::StackTraceString() {
 }
 
 
+void Isolate::CaptureAndSetCurrentStackTraceFor(Handle<Object> error_object) {

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

Object -> JSObject as we are expect it to be so.

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

Done.

+  if (capture_stack_trace_for_uncaught_exceptions_) {
+    // Capture stack trace for a detailed exception message.
+    String* key = *(factory()->LookupAsciiSymbol("stackTrace"));
+    JSArray* stack_trace = *CaptureCurrentStackTrace(
+        stack_trace_for_uncaught_exceptions_frame_limit_,
+        stack_trace_for_uncaught_exceptions_options_);
+    JSObject::cast(*error_object)->SetHiddenProperty(key, stack_trace);

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

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.

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

Done.

+  }
+}
+
+
 Handle<JSArray> Isolate::CaptureCurrentStackTrace(
     int frame_limit, StackTrace::StackTraceOptions options) {
   // Ensure no negative values.
@@ -1037,6 +1049,24 @@ bool Isolate::ShouldReportException(bool* can_be_caught_externally,
 }
 
 
+bool Isolate::IsErrorObject(Handle<Object> obj) {
+  if (!obj->IsJSObject()) return false;
+
+  String* error_key = *(factory()->LookupAsciiSymbol("$Error"));
+  Object* error_constructor =
+      js_builtins_object()->GetPropertyNoExceptionThrown(error_key);
+
+  for (Object* prototype = *obj; !prototype->IsNull();
+       prototype = prototype->GetPrototype()) {
+    if (!prototype->IsJSObject()) return false;
+    if (JSObject::cast(prototype)->map()->constructor() == error_constructor) {
+      return true;
+    }
+  }
+  return false;
+}
+
+
 void Isolate::DoThrow(MaybeObject* exception, MessageLocation* location) {
   ASSERT(!has_pending_exception());
 
@@ -1053,6 +1083,9 @@ void Isolate::DoThrow(MaybeObject* exception, MessageLocation* location) {
   bool should_report_exception =
       ShouldReportException(&can_be_caught_externally, catchable_by_javascript);
   bool report_exception = catchable_by_javascript && should_report_exception;
+  bool try_catch_needs_message =
+      can_be_caught_externally && try_catch_handler()->capture_message_;
+  bool bootstrapping = bootstrapper()->IsActive();
 
 #ifdef ENABLE_DEBUGGER_SUPPORT
   // Notify debugger of exception.
@@ -1061,34 +1094,48 @@ void Isolate::DoThrow(MaybeObject* exception, MessageLocation* location) {
   }
 #endif
 
-  // Generate the message.
+  // Generate the message if required.
   Handle<Object> message_obj;

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

Now can be moved into the if where is needed.

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

Done.

-  MessageLocation potential_computed_location;
-  bool try_catch_needs_message =
-      can_be_caught_externally &&
-      try_catch_handler()->capture_message_;
   if (report_exception || try_catch_needs_message) {
+    MessageLocation potential_computed_location;
     if (location == NULL) {
-      // If no location was specified we use a computed one instead
+      // If no location was specified we use a computed one instead.
       ComputeLocation(&potential_computed_location);
       location = &potential_computed_location;
     }
-    if (!bootstrapper()->IsActive()) {
-      // It's not safe to try to make message objects or collect stack
-      // traces while the bootstrapper is active since the infrastructure
-      // may not have been properly initialized.
+    // It's not safe to try to make message objects or collect stack traces
+    // while the bootstrapper is active since the infrastructure may not have
+    // been properly initialized.
+    if (!bootstrapping) {
       Handle<String> stack_trace;
       if (FLAG_trace_exception) stack_trace = StackTraceString();
       Handle<JSArray> stack_trace_object;
-      if (report_exception && capture_stack_trace_for_uncaught_exceptions_) {
+      if (capture_stack_trace_for_uncaught_exceptions_) {
+        if (IsErrorObject(exception_handle)) {
+          // We fetch the stack trace that corresponds to this error object.
+          String* key = *(factory()->LookupAsciiSymbol("stackTrace"));

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

"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::

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

Done. How about v8::hidden_stack_trace?

+          Object* stack_property =
+              JSObject::cast(*exception_handle)->GetHiddenProperty(key);
+          stack_trace_object = Handle<JSArray>(JSArray::cast(stack_property));

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

consider code like:

throw {__proto__: new Error()};

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

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

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

+        } else {
+          // Not an error object, we capture at throw site.
           stack_trace_object = CaptureCurrentStackTrace(
               stack_trace_for_uncaught_exceptions_frame_limit_,
               stack_trace_for_uncaught_exceptions_options_);
+        }
       }
-      ASSERT(is_object);  // Can't use the handle unless there's a real object.

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

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

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

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

-      message_obj = MessageHandler::MakeMessageObject("uncaught_exception",
-          location, HandleVector<Object>(&exception_handle, 1), stack_trace,
+      message_obj = MessageHandler::MakeMessageObject(
+          "uncaught_exception",
+          location,
+          HandleVector<Object>(&exception_handle, 1),
+          stack_trace,
           stack_trace_object);
+      thread_local_top()->pending_message_obj_ = *message_obj;
+      if (location != NULL) {
+        thread_local_top()->pending_message_script_ = *location->script();
+        thread_local_top()->pending_message_start_pos_ = location->start_pos();
+        thread_local_top()->pending_message_end_pos_ = location->end_pos();
+      }
     } else if (location != NULL && !location->script().is_null()) {
       // We are bootstrapping and caught an error where the location is set
       // and we have a script for the location.
@@ -1104,14 +1151,6 @@ void Isolate::DoThrow(MaybeObject* exception, MessageLocation* location) {
 
   // Save the message for reporting if the the exception remains uncaught.
   thread_local_top()->has_pending_message_ = report_exception;
-  if (!message_obj.is_null()) {
-    thread_local_top()->pending_message_obj_ = *message_obj;
-    if (location != NULL) {
-      thread_local_top()->pending_message_script_ = *location->script();
-      thread_local_top()->pending_message_start_pos_ = location->start_pos();
-      thread_local_top()->pending_message_end_pos_ = location->end_pos();
-    }
-  }
 
   // Do not forget to clean catcher_ if currently thrown exception cannot
   // be caught.  If necessary, ReThrow will update the catcher.