From e1d80e28580bdc8f70046b407761df2c6630b742 Mon Sep 17 00:00:00 2001 From: "yangguo@chromium.org" Date: Mon, 30 Jun 2014 11:48:20 +0000 Subject: [PATCH] Fix stack trace accessor behavior. R=verwaest@chromium.org BUG=v8:3404 LOG=N Review URL: https://codereview.chromium.org/343563009 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@22089 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- include/v8.h | 2 +- src/bootstrapper.cc | 11 ++ src/heap.cc | 2 + src/heap.h | 3 +- src/isolate.cc | 70 +++++++------ src/isolate.h | 8 +- src/messages.js | 111 ++++++++------------- src/runtime.cc | 21 +--- src/runtime.h | 3 +- test/mjsunit/regress/regress-3404.js | 27 +++++ test/mjsunit/runtime-gen/collectstacktrace.js | 3 +- .../runtime-gen/getandclearoverflowedstacktrace.js | 5 - test/mjsunit/stack-traces-overflow.js | 6 +- test/mjsunit/stack-traces.js | 20 ++++ tools/generate-runtime-tests.py | 6 +- 15 files changed, 158 insertions(+), 140 deletions(-) create mode 100644 test/mjsunit/regress/regress-3404.js delete mode 100644 test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js diff --git a/include/v8.h b/include/v8.h index 7d27029..cbbc0ff 100644 --- a/include/v8.h +++ b/include/v8.h @@ -5586,7 +5586,7 @@ class Internals { static const int kNullValueRootIndex = 7; static const int kTrueValueRootIndex = 8; static const int kFalseValueRootIndex = 9; - static const int kEmptyStringRootIndex = 160; + static const int kEmptyStringRootIndex = 162; // The external allocation limit should be below 256 MB on all architectures // to avoid that resource-constrained embedders run low on memory. diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 8909e25..eb9d311 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -2211,6 +2211,17 @@ bool Genesis::InstallSpecialObjects(Handle native_context) { false); } + // Expose the stack trace symbol to native JS. + RETURN_ON_EXCEPTION_VALUE( + isolate, + JSObject::SetOwnPropertyIgnoreAttributes( + handle(native_context->builtins(), isolate), + factory->InternalizeOneByteString( + STATIC_ASCII_VECTOR("stack_trace_symbol")), + factory->stack_trace_symbol(), + NONE), + false); + // Expose the debug global object in global if a name for it is specified. if (FLAG_expose_debug_as != NULL && strlen(FLAG_expose_debug_as) != 0) { // If loading fails we just bail out without installing the diff --git a/src/heap.cc b/src/heap.cc index c4cf04a..0a87eff 100644 --- a/src/heap.cc +++ b/src/heap.cc @@ -2917,6 +2917,8 @@ void Heap::CreateInitialObjects() { set_uninitialized_symbol(*factory->NewPrivateSymbol()); set_megamorphic_symbol(*factory->NewPrivateSymbol()); set_observed_symbol(*factory->NewPrivateSymbol()); + set_stack_trace_symbol(*factory->NewPrivateSymbol()); + set_detailed_stack_trace_symbol(*factory->NewPrivateSymbol()); Handle slow_element_dictionary = SeededNumberDictionary::New(isolate(), 0, TENURED); diff --git a/src/heap.h b/src/heap.h index 85af3c9..1300412 100644 --- a/src/heap.h +++ b/src/heap.h @@ -193,6 +193,8 @@ namespace internal { V(Symbol, observed_symbol, ObservedSymbol) \ V(Symbol, uninitialized_symbol, UninitializedSymbol) \ V(Symbol, megamorphic_symbol, MegamorphicSymbol) \ + V(Symbol, stack_trace_symbol, StackTraceSymbol) \ + V(Symbol, detailed_stack_trace_symbol, DetailedStackTraceSymbol) \ V(FixedArray, materialized_objects, MaterializedObjects) \ V(FixedArray, allocation_sites_scratchpad, AllocationSitesScratchpad) \ V(FixedArray, microtask_queue, MicrotaskQueue) @@ -340,7 +342,6 @@ namespace internal { V(strict_compare_ic_string, "===") \ V(infinity_string, "Infinity") \ V(minus_infinity_string, "-Infinity") \ - V(hidden_stack_trace_string, "v8::hidden_stack_trace") \ V(query_colon_string, "(?:)") \ V(Generator_string, "Generator") \ V(throw_string, "throw") \ diff --git a/src/isolate.cc b/src/isolate.cc index 9bffca2..bb55119 100644 --- a/src/isolate.cc +++ b/src/isolate.cc @@ -353,10 +353,23 @@ static bool IsVisibleInStackTrace(StackFrame* raw_frame, } -Handle Isolate::CaptureSimpleStackTrace(Handle error_object, - Handle caller, - int limit) { +Handle Isolate::CaptureSimpleStackTrace(Handle error_object, + Handle caller) { + // Get stack trace limit. + Handle error = Object::GetProperty( + this, js_builtins_object(), "$Error").ToHandleChecked(); + if (!error->IsJSObject()) return factory()->undefined_value(); + + Handle stackTraceLimit = + factory()->InternalizeUtf8String("stackTraceLimit"); + ASSERT(!stackTraceLimit.is_null()); + Handle stack_trace_limit = + JSObject::GetDataProperty(Handle::cast(error), + stackTraceLimit); + if (!stack_trace_limit->IsNumber()) return factory()->undefined_value(); + int limit = FastD2IChecked(stack_trace_limit->Number()); limit = Max(limit, 0); // Ensure that limit is not negative. + int initial_size = Min(limit, 10); Handle elements = factory()->NewFixedArrayWithHoles(initial_size * 4 + 1); @@ -426,15 +439,25 @@ Handle Isolate::CaptureSimpleStackTrace(Handle error_object, void Isolate::CaptureAndSetDetailedStackTrace(Handle error_object) { if (capture_stack_trace_for_uncaught_exceptions_) { // Capture stack trace for a detailed exception message. - Handle key = factory()->hidden_stack_trace_string(); + Handle key = factory()->detailed_stack_trace_symbol(); Handle stack_trace = CaptureCurrentStackTrace( stack_trace_for_uncaught_exceptions_frame_limit_, stack_trace_for_uncaught_exceptions_options_); - JSObject::SetHiddenProperty(error_object, key, stack_trace); + JSObject::SetProperty( + error_object, key, stack_trace, NONE, STRICT).Assert(); } } +void Isolate::CaptureAndSetSimpleStackTrace(Handle error_object, + Handle caller) { + // Capture stack trace for simple stack trace string formatting. + Handle key = factory()->stack_trace_symbol(); + Handle stack_trace = CaptureSimpleStackTrace(error_object, caller); + JSObject::SetProperty(error_object, key, stack_trace, NONE, STRICT).Assert(); +} + + Handle Isolate::CaptureCurrentStackTrace( int frame_limit, StackTrace::StackTraceOptions options) { // Ensure no negative values. @@ -780,25 +803,7 @@ Object* Isolate::StackOverflow() { Handle exception = factory()->CopyJSObject(boilerplate); DoThrow(*exception, NULL); - // Get stack trace limit. - Handle error = Object::GetProperty( - this, js_builtins_object(), "$Error").ToHandleChecked(); - if (!error->IsJSObject()) return heap()->exception(); - - Handle stackTraceLimit = - factory()->InternalizeUtf8String("stackTraceLimit"); - ASSERT(!stackTraceLimit.is_null()); - Handle stack_trace_limit = - JSObject::GetDataProperty(Handle::cast(error), - stackTraceLimit); - if (!stack_trace_limit->IsNumber()) return heap()->exception(); - int limit = FastD2IChecked(stack_trace_limit->Number()); - if (limit < 0) limit = 0; - Handle stack_trace = CaptureSimpleStackTrace( - exception, factory()->undefined_value(), limit); - JSObject::SetHiddenProperty(exception, - factory()->hidden_stack_trace_string(), - stack_trace); + CaptureAndSetSimpleStackTrace(exception, factory()->undefined_value()); return heap()->exception(); } @@ -1054,13 +1059,16 @@ void Isolate::DoThrow(Object* exception, MessageLocation* location) { if (capture_stack_trace_for_uncaught_exceptions_) { if (IsErrorObject(exception_handle)) { // We fetch the stack trace that corresponds to this error object. - Handle key = factory()->hidden_stack_trace_string(); - Object* stack_property = - JSObject::cast(*exception_handle)->GetHiddenProperty(key); - // Property lookup may have failed. In this case it's probably not - // a valid Error object. - if (stack_property->IsJSArray()) { - stack_trace_object = Handle(JSArray::cast(stack_property)); + Handle key = factory()->detailed_stack_trace_symbol(); + // Look up as own property. If the lookup fails, the exception is + // probably not a valid Error object. In that case, we fall through + // and capture the stack trace at this throw site. + LookupIterator lookup( + exception_handle, key, LookupIterator::CHECK_OWN_REAL); + Handle stack_trace_property; + if (Object::GetProperty(&lookup).ToHandle(&stack_trace_property) && + stack_trace_property->IsJSArray()) { + stack_trace_object = Handle::cast(stack_trace_property); } } if (stack_trace_object.is_null()) { diff --git a/src/isolate.h b/src/isolate.h index ed2ed33..06bf459 100644 --- a/src/isolate.h +++ b/src/isolate.h @@ -697,11 +697,11 @@ class Isolate { Handle CaptureCurrentStackTrace( int frame_limit, StackTrace::StackTraceOptions options); - - Handle CaptureSimpleStackTrace(Handle error_object, - Handle caller, - int limit); + Handle CaptureSimpleStackTrace(Handle error_object, + Handle caller); void CaptureAndSetDetailedStackTrace(Handle error_object); + void CaptureAndSetSimpleStackTrace(Handle error_object, + Handle caller); // Returns if the top context may access the given global object. If // the result is false, the pending exception is guaranteed to be diff --git a/src/messages.js b/src/messages.js index 8f83a62..61ba6ce 100644 --- a/src/messages.js +++ b/src/messages.js @@ -1080,7 +1080,8 @@ function GetStackFrames(raw_stack) { var formatting_custom_stack_trace = false; -function FormatStackTrace(obj, error_string, frames) { +function FormatStackTrace(obj, raw_stack) { + var frames = GetStackFrames(raw_stack); if (IS_FUNCTION($Error.prepareStackTrace) && !formatting_custom_stack_trace) { var array = []; %MoveArrayContents(frames, array); @@ -1097,7 +1098,7 @@ function FormatStackTrace(obj, error_string, frames) { } var lines = new InternalArray(); - lines.push(error_string); + lines.push(FormatErrorString(obj)); for (var i = 0; i < frames.length; i++) { var frame = frames[i]; var line; @@ -1132,45 +1133,45 @@ function GetTypeName(receiver, requireConstructor) { } -function captureStackTrace(obj, cons_opt) { - var stackTraceLimit = $Error.stackTraceLimit; - if (!stackTraceLimit || !IS_NUMBER(stackTraceLimit)) return; - if (stackTraceLimit < 0 || stackTraceLimit > 10000) { - stackTraceLimit = 10000; - } - var stack = %CollectStackTrace(obj, - cons_opt ? cons_opt : captureStackTrace, - stackTraceLimit); - - var error_string = FormatErrorString(obj); - - // Set the 'stack' property on the receiver. If the receiver is the same as - // holder of this setter, the accessor pair is turned into a data property. - var setter = function(v) { - // Set data property on the receiver (not necessarily holder). - %DefineDataPropertyUnchecked(this, 'stack', v, NONE); - if (this === obj) { - // Release context values if holder is the same as the receiver. - stack = error_string = UNDEFINED; +var stack_trace_symbol; // Set during bootstrapping. +var formatted_stack_trace_symbol = NEW_PRIVATE("formatted stack trace"); + + +// Format the stack trace if not yet done, and return it. +// Cache the formatted stack trace on the holder. +function StackTraceGetter() { + var formatted_stack_trace = GET_PRIVATE(this, formatted_stack_trace_symbol); + if (IS_UNDEFINED(formatted_stack_trace)) { + var holder = this; + while (!HAS_PRIVATE(holder, stack_trace_symbol)) { + holder = %GetPrototype(holder); + if (!holder) return UNDEFINED; } - }; + var stack_trace = GET_PRIVATE(holder, stack_trace_symbol); + if (IS_UNDEFINED(stack_trace)) return UNDEFINED; + formatted_stack_trace = FormatStackTrace(holder, stack_trace); + SET_PRIVATE(holder, stack_trace_symbol, UNDEFINED); + SET_PRIVATE(holder, formatted_stack_trace_symbol, formatted_stack_trace); + } + return formatted_stack_trace; +}; - // The holder of this getter ('obj') may not be the receiver ('this'). - // When this getter is called the first time, we use the context values to - // format a stack trace string and turn this accessor pair into a data - // property (on the holder). - var getter = function() { - // Stack is still a raw array awaiting to be formatted. - var result = FormatStackTrace(obj, error_string, GetStackFrames(stack)); - // Replace this accessor to return result directly. - %DefineAccessorPropertyUnchecked( - obj, 'stack', function() { return result }, setter, DONT_ENUM); - // Release context values. - stack = error_string = UNDEFINED; - return result; - }; - %DefineAccessorPropertyUnchecked(obj, 'stack', getter, setter, DONT_ENUM); +// If the receiver equals the holder, set the formatted stack trace that the +// getter returns. +function StackTraceSetter(v) { + if (HAS_PRIVATE(this, stack_trace_symbol)) { + SET_PRIVATE(this, stack_trace_symbol, UNDEFINED); + SET_PRIVATE(this, formatted_stack_trace_symbol, v); + } +}; + + +function captureStackTrace(obj, cons_opt) { + // Define accessors first, as this may fail and throw. + ObjectDefineProperty(obj, 'stack', { get: StackTraceGetter, + set: StackTraceSetter}); + %CollectStackTrace(obj, cons_opt ? cons_opt : captureStackTrace); } @@ -1306,40 +1307,8 @@ InstallFunctions($Error.prototype, DONT_ENUM, ['toString', ErrorToString]); function SetUpStackOverflowBoilerplate() { var boilerplate = MakeRangeError('stack_overflow', []); - var error_string = boilerplate.name + ": " + boilerplate.message; - - // Set the 'stack' property on the receiver. If the receiver is the same as - // holder of this setter, the accessor pair is turned into a data property. - var setter = function(v) { - %DefineDataPropertyUnchecked(this, 'stack', v, NONE); - // Tentatively clear the hidden property. If the receiver is the same as - // holder, we release the raw stack trace this way. - %GetAndClearOverflowedStackTrace(this); - }; - - // The raw stack trace is stored as a hidden property on the holder of this - // getter, which may not be the same as the receiver. Find the holder to - // retrieve the raw stack trace and then turn this accessor pair into a - // data property. - var getter = function() { - var holder = this; - while (!IS_ERROR(holder)) { - holder = %GetPrototype(holder); - if (IS_NULL(holder)) return MakeSyntaxError('illegal_access', []); - } - var stack = %GetAndClearOverflowedStackTrace(holder); - // We may not have captured any stack trace. - if (IS_UNDEFINED(stack)) return stack; - - var result = FormatStackTrace(holder, error_string, GetStackFrames(stack)); - // Replace this accessor to return result directly. - %DefineAccessorPropertyUnchecked( - holder, 'stack', function() { return result }, setter, DONT_ENUM); - return result; - }; - %DefineAccessorPropertyUnchecked( - boilerplate, 'stack', getter, setter, DONT_ENUM); + boilerplate, 'stack', StackTraceGetter, StackTraceSetter, DONT_ENUM); return boilerplate; } diff --git a/src/runtime.cc b/src/runtime.cc index f341d14..0a2f459 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -14428,30 +14428,15 @@ RUNTIME_FUNCTION(Runtime_GetScript) { // native code offset. RUNTIME_FUNCTION(Runtime_CollectStackTrace) { HandleScope scope(isolate); - ASSERT(args.length() == 3); + ASSERT(args.length() == 2); CONVERT_ARG_HANDLE_CHECKED(JSObject, error_object, 0); CONVERT_ARG_HANDLE_CHECKED(Object, caller, 1); - CONVERT_NUMBER_CHECKED(int32_t, limit, Int32, args[2]); // Optionally capture a more detailed stack trace for the message. isolate->CaptureAndSetDetailedStackTrace(error_object); // Capture a simple stack trace for the stack property. - return *isolate->CaptureSimpleStackTrace(error_object, caller, limit); -} - - -// Retrieve the stack trace. This is the raw stack trace that yet has to -// be formatted. Since we only need this once, clear it afterwards. -RUNTIME_FUNCTION(Runtime_GetAndClearOverflowedStackTrace) { - HandleScope scope(isolate); - ASSERT(args.length() == 1); - CONVERT_ARG_HANDLE_CHECKED(JSObject, error_object, 0); - Handle key = isolate->factory()->hidden_stack_trace_string(); - Handle result(error_object->GetHiddenProperty(key), isolate); - if (result->IsTheHole()) return isolate->heap()->undefined_value(); - RUNTIME_ASSERT(result->IsJSArray() || result->IsUndefined()); - JSObject::DeleteHiddenProperty(error_object, key); - return *result; + isolate->CaptureAndSetSimpleStackTrace(error_object, caller); + return isolate->heap()->undefined_value(); } diff --git a/src/runtime.h b/src/runtime.h index ebc8598..a93ca98 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -196,8 +196,7 @@ namespace internal { F(FunctionIsAPIFunction, 1, 1) \ F(FunctionIsBuiltin, 1, 1) \ F(GetScript, 1, 1) \ - F(CollectStackTrace, 3, 1) \ - F(GetAndClearOverflowedStackTrace, 1, 1) \ + F(CollectStackTrace, 2, 1) \ F(GetV8Version, 0, 1) \ \ F(SetCode, 2, 1) \ diff --git a/test/mjsunit/regress/regress-3404.js b/test/mjsunit/regress/regress-3404.js new file mode 100644 index 0000000..c4d280e --- /dev/null +++ b/test/mjsunit/regress/regress-3404.js @@ -0,0 +1,27 @@ +// Copyright 2014 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +function testError(error) { + // Reconfigure e.stack to be non-configurable + var desc1 = Object.getOwnPropertyDescriptor(error, "stack"); + Object.defineProperty(error, "stack", + {get: desc1.get, set: desc1.set, configurable: false}); + + var desc2 = Object.getOwnPropertyDescriptor(error, "stack"); + assertFalse(desc2.configurable); + assertEquals(desc1.get, desc2.get); + assertEquals(desc2.get, desc2.get); +} + +function stackOverflow() { + function f() { f(); } + try { f() } catch (e) { return e; } +} + +function referenceError() { + try { g() } catch (e) { return e; } +} + +testError(referenceError()); +testError(stackOverflow()); diff --git a/test/mjsunit/runtime-gen/collectstacktrace.js b/test/mjsunit/runtime-gen/collectstacktrace.js index 0272863..58bf9f7 100644 --- a/test/mjsunit/runtime-gen/collectstacktrace.js +++ b/test/mjsunit/runtime-gen/collectstacktrace.js @@ -3,5 +3,4 @@ // Flags: --allow-natives-syntax --harmony var _error_object = new Object(); var _caller = new Object(); -var _limit = 32; -%CollectStackTrace(_error_object, _caller, _limit); +%CollectStackTrace(_error_object, _caller); diff --git a/test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js b/test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js deleted file mode 100644 index 8abf790..0000000 --- a/test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js +++ /dev/null @@ -1,5 +0,0 @@ -// Copyright 2014 the V8 project authors. All rights reserved. -// AUTO-GENERATED BY tools/generate-runtime-tests.py, DO NOT MODIFY -// Flags: --allow-natives-syntax --harmony -var _error_object = new Object(); -%GetAndClearOverflowedStackTrace(_error_object); diff --git a/test/mjsunit/stack-traces-overflow.js b/test/mjsunit/stack-traces-overflow.js index 1c67680..fab49d3 100644 --- a/test/mjsunit/stack-traces-overflow.js +++ b/test/mjsunit/stack-traces-overflow.js @@ -61,8 +61,8 @@ try { function testErrorPrototype(prototype) { var object = {}; object.__proto__ = prototype; - object.stack = "123"; - assertEquals("123", object.stack); + object.stack = "123"; // Overwriting stack property fails. + assertEquals(prototype.stack, object.stack); assertTrue("123" != prototype.stack); } @@ -126,6 +126,8 @@ try { rec1(0); } catch (e) { assertEquals(undefined, e.stack); + e.stack = "abc"; + assertEquals("abc", e.stack); } Error.stackTraceLimit = 3; diff --git a/test/mjsunit/stack-traces.js b/test/mjsunit/stack-traces.js index 46a16eb..f80a627 100644 --- a/test/mjsunit/stack-traces.js +++ b/test/mjsunit/stack-traces.js @@ -331,3 +331,23 @@ Error.prepareStackTrace = function() { Error.prepareStackTrace = "custom"; }; new Error().stack; assertEquals("custom", Error.prepareStackTrace); + +// Check that the formatted stack trace can be set to undefined. +error = new Error(); +error.stack = undefined; +assertEquals(undefined, error.stack); + +// Check that the stack trace accessors are not forcibly set. +var my_error = {}; +Object.freeze(my_error); +assertThrows(function() { Error.captureStackTrace(my_error); }); + +my_error = {}; +Object.preventExtensions(my_error); +assertThrows(function() { Error.captureStackTrace(my_error); }); + +var fake_error = {}; +my_error = new Error(); +var stolen_getter = Object.getOwnPropertyDescriptor(my_error, 'stack').get; +Object.defineProperty(fake_error, 'stack', { get: stolen_getter }); +assertEquals(undefined, fake_error.stack); diff --git a/tools/generate-runtime-tests.py b/tools/generate-runtime-tests.py index f018cf0..7a9cd82 100755 --- a/tools/generate-runtime-tests.py +++ b/tools/generate-runtime-tests.py @@ -47,11 +47,11 @@ EXPAND_MACROS = [ # that the parser doesn't bit-rot. Change the values as needed when you add, # remove or change runtime functions, but make sure we don't lose our ability # to parse them! -EXPECTED_FUNCTION_COUNT = 416 -EXPECTED_FUZZABLE_COUNT = 331 +EXPECTED_FUNCTION_COUNT = 415 +EXPECTED_FUZZABLE_COUNT = 330 EXPECTED_CCTEST_COUNT = 6 EXPECTED_UNKNOWN_COUNT = 4 -EXPECTED_BUILTINS_COUNT = 809 +EXPECTED_BUILTINS_COUNT = 811 # Don't call these at all. -- 2.7.4