From 5d408ee73da4a7be8f4c3518db58d02b96f2da5a Mon Sep 17 00:00:00 2001 From: "yangguo@chromium.org" Date: Mon, 30 Jun 2014 13:16:42 +0000 Subject: [PATCH] Revert "Fix stack trace accessor behavior." This reverts r22089. TBR=verwaest@chromium.org Review URL: https://codereview.chromium.org/360033002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@22091 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, 140 insertions(+), 158 deletions(-) delete mode 100644 test/mjsunit/regress/regress-3404.js create mode 100644 test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js diff --git a/include/v8.h b/include/v8.h index cbbc0ff..7d27029 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 = 162; + static const int kEmptyStringRootIndex = 160; // 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 eb9d311..8909e25 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -2211,17 +2211,6 @@ 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 0a87eff..c4cf04a 100644 --- a/src/heap.cc +++ b/src/heap.cc @@ -2917,8 +2917,6 @@ 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 1300412..85af3c9 100644 --- a/src/heap.h +++ b/src/heap.h @@ -193,8 +193,6 @@ 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) @@ -342,6 +340,7 @@ 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 bb55119..9bffca2 100644 --- a/src/isolate.cc +++ b/src/isolate.cc @@ -353,23 +353,10 @@ static bool IsVisibleInStackTrace(StackFrame* raw_frame, } -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()); +Handle Isolate::CaptureSimpleStackTrace(Handle error_object, + Handle caller, + int limit) { limit = Max(limit, 0); // Ensure that limit is not negative. - int initial_size = Min(limit, 10); Handle elements = factory()->NewFixedArrayWithHoles(initial_size * 4 + 1); @@ -439,25 +426,15 @@ 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()->detailed_stack_trace_symbol(); + Handle key = factory()->hidden_stack_trace_string(); Handle stack_trace = CaptureCurrentStackTrace( stack_trace_for_uncaught_exceptions_frame_limit_, stack_trace_for_uncaught_exceptions_options_); - JSObject::SetProperty( - error_object, key, stack_trace, NONE, STRICT).Assert(); + JSObject::SetHiddenProperty(error_object, key, stack_trace); } } -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. @@ -803,7 +780,25 @@ Object* Isolate::StackOverflow() { Handle exception = factory()->CopyJSObject(boilerplate); DoThrow(*exception, NULL); - CaptureAndSetSimpleStackTrace(exception, factory()->undefined_value()); + // 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); return heap()->exception(); } @@ -1059,16 +1054,13 @@ 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()->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); + 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)); } } if (stack_trace_object.is_null()) { diff --git a/src/isolate.h b/src/isolate.h index 06bf459..ed2ed33 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); + + Handle CaptureSimpleStackTrace(Handle error_object, + Handle caller, + int limit); 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 61ba6ce..8f83a62 100644 --- a/src/messages.js +++ b/src/messages.js @@ -1080,8 +1080,7 @@ function GetStackFrames(raw_stack) { var formatting_custom_stack_trace = false; -function FormatStackTrace(obj, raw_stack) { - var frames = GetStackFrames(raw_stack); +function FormatStackTrace(obj, error_string, frames) { if (IS_FUNCTION($Error.prepareStackTrace) && !formatting_custom_stack_trace) { var array = []; %MoveArrayContents(frames, array); @@ -1098,7 +1097,7 @@ function FormatStackTrace(obj, raw_stack) { } var lines = new InternalArray(); - lines.push(FormatErrorString(obj)); + lines.push(error_string); for (var i = 0; i < frames.length; i++) { var frame = frames[i]; var line; @@ -1133,45 +1132,45 @@ function GetTypeName(receiver, requireConstructor) { } -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; -}; - - -// 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) { + 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; + } + }; + // 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; + }; -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); + %DefineAccessorPropertyUnchecked(obj, 'stack', getter, setter, DONT_ENUM); } @@ -1307,8 +1306,40 @@ 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', StackTraceGetter, StackTraceSetter, DONT_ENUM); + boilerplate, 'stack', getter, setter, DONT_ENUM); return boilerplate; } diff --git a/src/runtime.cc b/src/runtime.cc index 0a2f459..f341d14 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -14428,15 +14428,30 @@ RUNTIME_FUNCTION(Runtime_GetScript) { // native code offset. RUNTIME_FUNCTION(Runtime_CollectStackTrace) { HandleScope scope(isolate); - ASSERT(args.length() == 2); + ASSERT(args.length() == 3); 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. - isolate->CaptureAndSetSimpleStackTrace(error_object, caller); - return isolate->heap()->undefined_value(); + 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; } diff --git a/src/runtime.h b/src/runtime.h index a93ca98..ebc8598 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -196,7 +196,8 @@ namespace internal { F(FunctionIsAPIFunction, 1, 1) \ F(FunctionIsBuiltin, 1, 1) \ F(GetScript, 1, 1) \ - F(CollectStackTrace, 2, 1) \ + F(CollectStackTrace, 3, 1) \ + F(GetAndClearOverflowedStackTrace, 1, 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 deleted file mode 100644 index c4d280e..0000000 --- a/test/mjsunit/regress/regress-3404.js +++ /dev/null @@ -1,27 +0,0 @@ -// 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 58bf9f7..0272863 100644 --- a/test/mjsunit/runtime-gen/collectstacktrace.js +++ b/test/mjsunit/runtime-gen/collectstacktrace.js @@ -3,4 +3,5 @@ // Flags: --allow-natives-syntax --harmony var _error_object = new Object(); var _caller = new Object(); -%CollectStackTrace(_error_object, _caller); +var _limit = 32; +%CollectStackTrace(_error_object, _caller, _limit); diff --git a/test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js b/test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js new file mode 100644 index 0000000..8abf790 --- /dev/null +++ b/test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js @@ -0,0 +1,5 @@ +// 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 fab49d3..1c67680 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"; // Overwriting stack property fails. - assertEquals(prototype.stack, object.stack); + object.stack = "123"; + assertEquals("123", object.stack); assertTrue("123" != prototype.stack); } @@ -126,8 +126,6 @@ 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 f80a627..46a16eb 100644 --- a/test/mjsunit/stack-traces.js +++ b/test/mjsunit/stack-traces.js @@ -331,23 +331,3 @@ 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 7a9cd82..f018cf0 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 = 415 -EXPECTED_FUZZABLE_COUNT = 330 +EXPECTED_FUNCTION_COUNT = 416 +EXPECTED_FUZZABLE_COUNT = 331 EXPECTED_CCTEST_COUNT = 6 EXPECTED_UNKNOWN_COUNT = 4 -EXPECTED_BUILTINS_COUNT = 811 +EXPECTED_BUILTINS_COUNT = 809 # Don't call these at all. -- 2.7.4