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

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

src/bootstrapper.cc

@@ -2211,6 +2211,17 @@ bool Genesis::InstallSpecialObjects(Handle<Context> 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

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<SeededNumberDictionary> slow_element_dictionary =
       SeededNumberDictionary::New(isolate(), 0, TENURED);

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")                                               \

src/isolate.cc

@@ -353,10 +353,23 @@ static bool IsVisibleInStackTrace(StackFrame* raw_frame,
 }
 
 
-Handle<JSArray> Isolate::CaptureSimpleStackTrace(Handle<JSObject> error_object,
-                                                 Handle<Object> caller,
-                                                 int limit) {
+Handle<Object> Isolate::CaptureSimpleStackTrace(Handle<JSObject> error_object,
+                                                Handle<Object> caller) {
+  // Get stack trace limit.
+  Handle<Object> error = Object::GetProperty(
+      this, js_builtins_object(), "$Error").ToHandleChecked();
+  if (!error->IsJSObject()) return factory()->undefined_value();
+
+  Handle<String> stackTraceLimit =
+      factory()->InternalizeUtf8String("stackTraceLimit");
+  ASSERT(!stackTraceLimit.is_null());
+  Handle<Object> stack_trace_limit =
+      JSObject::GetDataProperty(Handle<JSObject>::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<FixedArray> elements =
       factory()->NewFixedArrayWithHoles(initial_size * 4 + 1);
@@ -426,15 +439,25 @@ Handle<JSArray> Isolate::CaptureSimpleStackTrace(Handle<JSObject> error_object,
 void Isolate::CaptureAndSetDetailedStackTrace(Handle<JSObject> error_object) {
   if (capture_stack_trace_for_uncaught_exceptions_) {
     // Capture stack trace for a detailed exception message.
-    Handle<String> key = factory()->hidden_stack_trace_string();
+    Handle<Name> key = factory()->detailed_stack_trace_symbol();
     Handle<JSArray> 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<JSObject> error_object,
+                                            Handle<Object> caller) {
+  // Capture stack trace for simple stack trace string formatting.
+  Handle<Name> key = factory()->stack_trace_symbol();
+  Handle<Object> stack_trace = CaptureSimpleStackTrace(error_object, caller);
+  JSObject::SetProperty(error_object, key, stack_trace, NONE, STRICT).Assert();
+}
+
+
 Handle<JSArray> Isolate::CaptureCurrentStackTrace(
     int frame_limit, StackTrace::StackTraceOptions options) {
   // Ensure no negative values.
@@ -780,25 +803,7 @@ Object* Isolate::StackOverflow() {
   Handle<JSObject> exception = factory()->CopyJSObject(boilerplate);
   DoThrow(*exception, NULL);
 
-  // Get stack trace limit.
-  Handle<Object> error = Object::GetProperty(
-      this, js_builtins_object(), "$Error").ToHandleChecked();
-  if (!error->IsJSObject()) return heap()->exception();
-
-  Handle<String> stackTraceLimit =
-      factory()->InternalizeUtf8String("stackTraceLimit");
-  ASSERT(!stackTraceLimit.is_null());
-  Handle<Object> stack_trace_limit =
-      JSObject::GetDataProperty(Handle<JSObject>::cast(error),
-                                stackTraceLimit);
-  if (!stack_trace_limit->IsNumber()) return heap()->exception();
-  int limit = FastD2IChecked(stack_trace_limit->Number());
-  if (limit < 0) limit = 0;
-  Handle<JSArray> 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<String> 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>(JSArray::cast(stack_property));
+          Handle<Name> 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<Object> stack_trace_property;
+          if (Object::GetProperty(&lookup).ToHandle(&stack_trace_property) &&
+              stack_trace_property->IsJSArray()) {
+            stack_trace_object = Handle<JSArray>::cast(stack_trace_property);
           }
         }
         if (stack_trace_object.is_null()) {

src/isolate.h

@@ -697,11 +697,11 @@ class Isolate {
   Handle<JSArray> CaptureCurrentStackTrace(
       int frame_limit,
       StackTrace::StackTraceOptions options);
-
-  Handle<JSArray> CaptureSimpleStackTrace(Handle<JSObject> error_object,
-                                          Handle<Object> caller,
-                                          int limit);
+  Handle<Object> CaptureSimpleStackTrace(Handle<JSObject> error_object,
+                                         Handle<Object> caller);
   void CaptureAndSetDetailedStackTrace(Handle<JSObject> error_object);
+  void CaptureAndSetSimpleStackTrace(Handle<JSObject> error_object,
+                                     Handle<Object> caller);
 
   // Returns if the top context may access the given global object. If
   // the result is false, the pending exception is guaranteed to be

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;
 }

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<String> key = isolate->factory()->hidden_stack_trace_string();
-  Handle<Object> 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();
 }
 
 

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

test/mjsunit/regress/regress-3404.js

+// 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());

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

test/mjsunit/runtime-gen/getandclearoverflowedstacktrace.js

-// 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);

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;

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

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.