Reviewers: ulan,
Description:
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.
[email protected]
BUG=
Please review this at https://chromiumcodereview.appspot.com/11649037/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/handles.cc
M src/heap.cc
M src/messages.js
M test/mjsunit/stack-traces.js
Index: src/handles.cc
diff --git a/src/handles.cc b/src/handles.cc
index
3bc1f4bea03d831cf802c453eb69c15e6a13f49f..f11d104cd88409f9d5abec3e88972d23f36b9f98
100644
--- a/src/handles.cc
+++ b/src/handles.cc
@@ -364,6 +364,9 @@ static void ClearWrapperCache(Persistent<v8::Value>
handle, void*) {
Handle<JSValue> GetScriptWrapper(Handle<Script> script) {
+ // GC could lead to recursion, which this function cannot deal with
+ // (the cache would not yet be set in a recursive call).
+ AlwaysAllocateScope no_gc;
if (script->wrapper()->foreign_address() != NULL) {
// Return the script wrapper directly from the cache.
return Handle<JSValue>(
Index: src/heap.cc
diff --git a/src/heap.cc b/src/heap.cc
index
66512931698aa582f80a6fd39b60b50ec4095f04..ba2d99408d118b2dba5f00e4dc5e901e8d189038
100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -7293,36 +7293,40 @@ 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;
+ list_[write_index++] = *object_handle;
budget = 0;
}
}
Index: src/messages.js
diff --git a/src/messages.js b/src/messages.js
index
0bfaf6c6118f636e45a029d0e5dabc4ec02c3ae5..24cd66d1c66ce0ebd4a6866d545209622630a1db
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;
}
Index: test/mjsunit/stack-traces.js
diff --git a/test/mjsunit/stack-traces.js b/test/mjsunit/stack-traces.js
index
71d64191c106f979fdf715ba35c36a5c34417d62..b5d58fa0759f3086bce84d139824114d0b80d505
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);
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
