From: yangguo@chromium.org <yangguo@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Thu, 20 Dec 2012 16:25:26 +0000 (+0000)
Subject: Fix several bugs in error stack trace formatting.
X-Git-Tag: upstream/4.7.83~15397
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=a3f16f8e65084be915cd6e26325bbef7a07583e8;p=platform%2Fupstream%2Fv8.git

Fix several bugs in error stack trace formatting.

GetScriptWrapper can be called recursively:
GetScriptWrapper -> GC -> DeferredFormatStackTrace -> GetScriptWrapper

GC-unsafe code in ErrorObjectList::DeferredFormatStackTrace

Enable overwriting Error.prepareStackTrace by itself while not
causing infinity recursion when it triggers an exception.

R=ulan@chromium.org
BUG=

Review URL: https://chromiumcodereview.appspot.com/11649037

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@13256 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---

diff --git a/src/handles.cc b/src/handles.cc
index 3bc1f4bea..855997ec3 100644
--- a/src/handles.cc
+++ b/src/handles.cc
@@ -375,6 +375,15 @@ Handle<JSValue> GetScriptWrapper(Handle<Script> script) {
   Handle<JSFunction> constructor = isolate->script_function();
   Handle<JSValue> result =
       Handle<JSValue>::cast(isolate->factory()->NewJSObject(constructor));
+
+  // The allocation might have triggered a GC, which could have called this
+  // function recursively, and a wrapper has already been created and cached.
+  // In that case, simply return the cached wrapper.
+  if (script->wrapper()->foreign_address() != NULL) {
+    return Handle<JSValue>(
+        reinterpret_cast<JSValue**>(script->wrapper()->foreign_address()));
+  }
+
   result->set_value(*script);
 
   // Create a new weak global handle and use it to cache the wrapper
diff --git a/src/heap.cc b/src/heap.cc
index 665129316..ec98ca526 100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -7293,36 +7293,41 @@ void ErrorObjectList::DeferredFormatStackTrace(Isolate* isolate) {
   int budget = kBudgetPerGC;
   for (int i = 0; i < list_.length(); i++) {
     Object* object = list_[i];
-    // Skip possible holes in the list.
-    if (object->IsTheHole()) continue;
-    if (isolate->heap()->InNewSpace(object) || budget == 0) {
-      list_[write_index++] = object;
-      continue;
+    JSFunction* getter_fun;
+
+    { AssertNoAllocation assert;
+      // Skip possible holes in the list.
+      if (object->IsTheHole()) continue;
+      if (isolate->heap()->InNewSpace(object) || budget == 0) {
+        list_[write_index++] = object;
+        continue;
+      }
+
+      // Check whether the stack property is backed by the original getter.
+      LookupResult lookup(isolate);
+      JSObject::cast(object)->LocalLookupRealNamedProperty(*stack_key, &lookup);
+      if (!lookup.IsFound() || lookup.type() != CALLBACKS) continue;
+      Object* callback = lookup.GetCallbackObject();
+      if (!callback->IsAccessorPair()) continue;
+      Object* getter_obj = AccessorPair::cast(callback)->getter();
+      if (!getter_obj->IsJSFunction()) continue;
+      getter_fun = JSFunction::cast(getter_obj);
+      String* key = isolate->heap()->hidden_stack_trace_symbol();
+      if (key != getter_fun->GetHiddenProperty(key)) continue;
     }
 
-    // Fire the stack property getter, if it is the original marked getter.
-    LookupResult lookup(isolate);
-    JSObject::cast(object)->LocalLookupRealNamedProperty(*stack_key, &lookup);
-    if (!lookup.IsFound() || lookup.type() != CALLBACKS) continue;
-    Object* callback = lookup.GetCallbackObject();
-    if (!callback->IsAccessorPair()) continue;
-    Object* getter_obj = AccessorPair::cast(callback)->getter();
-    if (!getter_obj->IsJSFunction()) continue;
-    JSFunction* getter_fun = JSFunction::cast(getter_obj);
-    String* key = isolate->heap()->hidden_stack_trace_symbol();
-    if (key != getter_fun->GetHiddenProperty(key)) continue;
     budget--;
+    HandleScope scope(isolate);
     bool has_exception = false;
-    Execution::Call(Handle<Object>(getter_fun, isolate),
-                    Handle<Object>(object, isolate),
-                    0,
-                    NULL,
-                    &has_exception);
+    Handle<Object> object_handle(object, isolate);
+    Handle<Object> getter_handle(getter_fun, isolate);
+    Execution::Call(getter_handle, object_handle, 0, NULL, &has_exception);
     if (has_exception) {
       // Hit an exception (most likely a stack overflow).
       // Wrap up this pass and retry after another GC.
       isolate->clear_pending_exception();
-      list_[write_index++] = object;
+      // We use the handle since calling the getter might have caused a GC.
+      list_[write_index++] = *object_handle;
       budget = 0;
     }
   }
diff --git a/src/messages.js b/src/messages.js
index 0bfaf6c61..24cd66d1c 100644
--- a/src/messages.js
+++ b/src/messages.js
@@ -1082,6 +1082,10 @@ function GetTypeName(obj, requireConstructor) {
 }
 
 
+// Flag to prevent recursive call of Error.prepareStackTrace.
+var formatting_custom_stack_trace = false;
+
+
 function captureStackTrace(obj, cons_opt) {
   var stackTraceLimit = $Error.stackTraceLimit;
   if (!stackTraceLimit || !IS_NUMBER(stackTraceLimit)) return;
@@ -1093,14 +1097,17 @@ function captureStackTrace(obj, cons_opt) {
                                  stackTraceLimit);
 
   // Don't be lazy if the error stack formatting is custom (observable).
-  if (IS_FUNCTION($Error.prepareStackTrace)) {
-    var custom_stacktrace_fun = $Error.prepareStackTrace;
-    // Use default error formatting for the case that custom formatting throws.
-    $Error.prepareStackTrace = null;
+  if (IS_FUNCTION($Error.prepareStackTrace) && !formatting_custom_stack_trace) {
     var array = [];
     %MoveArrayContents(GetStackFrames(stack), array);
-    obj.stack = custom_stacktrace_fun(obj, array);
-    $Error.prepareStackTrace = custom_stacktrace_fun;
+    formatting_custom_stack_trace = true;
+    try {
+      obj.stack = $Error.prepareStackTrace(obj, array);
+    } catch (e) {
+      throw e;  // The custom formatting function threw.  Rethrow.
+    } finally {
+      formatting_custom_stack_trace = false;
+    }
     return;
   }
 
diff --git a/test/mjsunit/stack-traces.js b/test/mjsunit/stack-traces.js
index 71d64191c..b5d58fa07 100644
--- a/test/mjsunit/stack-traces.js
+++ b/test/mjsunit/stack-traces.js
@@ -310,9 +310,9 @@ assertTrue(fired);
 error.stack;
 assertTrue(fired);
 
-//Check that throwing exception in a custom stack trace formatting function
-//does not lead to recursion.
-Error.prepareStackTrace = function() { throw new Error("abc"); }
+// Check that throwing exception in a custom stack trace formatting function
+// does not lead to recursion.
+Error.prepareStackTrace = function() { throw new Error("abc"); };
 var message;
 try {
   throw new Error();
@@ -321,3 +321,9 @@ try {
 }
 
 assertEquals("abc", message);
+
+// Test that modifying Error.prepareStackTrace by itself works.
+Error.prepareStackTrace = function() { Error.prepareStackTrace = "custom"; };
+new Error();
+
+assertEquals("custom", Error.prepareStackTrace);