Restore test and behavior prior to deferred stack trace formatting.

R=mstarzinger@chromium.org
TEST=stack-traces-overflow.js

Review URL: https://codereview.chromium.org/19805003

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

src/messages.js

@@ -1145,24 +1145,29 @@ function captureStackTrace(obj, cons_opt) {
   }
 
   var error_string = FormatErrorString(obj);
-  // Note that 'obj' and 'this' maybe different when called on objects that
+  // The holder of this getter ('obj') may not be the receiver ('this').
-  // have the error object on its prototype chain.  The getter replaces itself
+  // When this getter is called the first time, we use the context values to
-  // with a data property as soon as the stack trace has been formatted.
+  // format a stack trace string and turn this accessor pair into a data
-  // The getter must not change the object layout as it may be called after GC.
+  // property (on the holder).
   var getter = function() {
-    if (IS_STRING(stack)) return stack;
     // Stack is still a raw array awaiting to be formatted.
-    stack = FormatStackTrace(error_string, GetStackFrames(stack));
+    var result = FormatStackTrace(error_string, GetStackFrames(stack));
-    // Release context value.
+    // Turn this accessor into a data property.
-    error_string = void 0;
+    %DefineOrRedefineDataProperty(obj, 'stack', result, NONE);
-    return stack;
+    // Release context values.
+    stack = error_string = void 0;
+    return result;
   };
-  %MarkOneShotGetter(getter);
 
-  // The 'stack' property of the receiver is set as data property.  If
+  // Set the 'stack' property on the receiver.  If the receiver is the same as
-  // the receiver is the same as holder, this accessor pair is replaced.
+  // 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).
     %DefineOrRedefineDataProperty(this, 'stack', v, NONE);
+    if (this === obj) {
+      // Release context values if holder is the same as the receiver.
+      stack = error_string = void 0;
+    }
   };
 
   %DefineOrRedefineAccessorProperty(obj, 'stack', getter, setter, DONT_ENUM);
@@ -1300,38 +1305,36 @@ InstallFunctions($Error.prototype, DONT_ENUM, ['toString', ErrorToString]);
 function SetUpStackOverflowBoilerplate() {
   var boilerplate = MakeRangeError('stack_overflow', []);
 
-  // The raw stack trace is stored as hidden property of the copy of this
-  // boilerplate error object.  Note that the receiver 'this' may not be that
-  // error object copy, but can be found on the prototype chain of 'this'.
-  // When the stack trace is formatted, this accessor property is replaced by
-  // a data property.
   var error_string = boilerplate.name + ": " + boilerplate.message;
 
-  // The getter must not change the object layout as it may be called after GC.
+  // The raw stack trace is stored as a hidden property on the holder of this
-  function getter() {
+  // 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 (holder == null) return MakeSyntaxError('illegal_access', []);
+      if (IS_NULL(holder)) return MakeSyntaxError('illegal_access', []);
     }
-    var stack = %GetOverflowedStackTrace(holder);
+    var stack = %GetAndClearOverflowedStackTrace(holder);
-    if (IS_STRING(stack)) return stack;
+    // We may not have captured any stack trace.
-    if (IS_ARRAY(stack)) {
+    if (IS_UNDEFINED(stack)) return stack;
-      var result = FormatStackTrace(error_string, GetStackFrames(stack));
-      %SetOverflowedStackTrace(holder, result);
-      return result;
-    }
-    return void 0;
-  }
-  %MarkOneShotGetter(getter);
 
-  // The 'stack' property of the receiver is set as data property.  If
+    var result = FormatStackTrace(error_string, GetStackFrames(stack));
-  // the receiver is the same as holder, this accessor pair is replaced.
+    // Replace this accessor with a data property.
-  function setter(v) {
+    %DefineOrRedefineDataProperty(holder, 'stack', result, NONE);
+    return result;
+  };
+
+  // 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) {
     %DefineOrRedefineDataProperty(this, 'stack', v, NONE);
-    // Release the stack trace that is stored as hidden property, if exists.
+    // Tentatively clear the hidden property. If the receiver is the same as
-    %SetOverflowedStackTrace(this, void 0);
+    // holder, we release the raw stack trace this way.
-  }
+    %GetAndClearOverflowedStackTrace(this);
+  };
 
   %DefineOrRedefineAccessorProperty(
       boilerplate, 'stack', getter, setter, DONT_ENUM);

src/runtime.cc

@@ -13415,50 +13415,21 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_CollectStackTrace) {
 }
 
 
-// Mark a function to recognize when called after GC to format the stack trace.
+// Retrieve the stack trace.  This is the raw stack trace that yet has to
-RUNTIME_FUNCTION(MaybeObject*, Runtime_MarkOneShotGetter) {
+// be formatted.  Since we only need this once, clear it afterwards.
-  HandleScope scope(isolate);
+RUNTIME_FUNCTION(MaybeObject*, Runtime_GetAndClearOverflowedStackTrace) {
-  ASSERT_EQ(args.length(), 1);
-  CONVERT_ARG_HANDLE_CHECKED(JSFunction, fun, 0);
-  Handle<String> key = isolate->factory()->hidden_stack_trace_string();
-  JSObject::SetHiddenProperty(fun, key, key);
-  return *fun;
-}
-
-
-// Retrieve the stack trace.  This could be the raw stack trace collected
-// on stack overflow or the already formatted stack trace string.
-RUNTIME_FUNCTION(MaybeObject*, Runtime_GetOverflowedStackTrace) {
   HandleScope scope(isolate);
   ASSERT_EQ(args.length(), 1);
   CONVERT_ARG_CHECKED(JSObject, error_object, 0);
   String* key = isolate->heap()->hidden_stack_trace_string();
   Object* result = error_object->GetHiddenProperty(key);
-  if (result->IsTheHole()) result = isolate->heap()->undefined_value();
+  if (result->IsTheHole()) return isolate->heap()->undefined_value();
-  RUNTIME_ASSERT(result->IsJSArray() ||
+  RUNTIME_ASSERT(result->IsJSArray() || result->IsUndefined());
-                 result->IsString() ||
+  error_object->DeleteHiddenProperty(key);
-                 result->IsUndefined());
   return result;
 }
 
 
-// Set or clear the stack trace attached to an stack overflow error object.
-RUNTIME_FUNCTION(MaybeObject*, Runtime_SetOverflowedStackTrace) {
-  HandleScope scope(isolate);
-  ASSERT_EQ(args.length(), 2);
-  CONVERT_ARG_HANDLE_CHECKED(JSObject, error_object, 0);
-  CONVERT_ARG_HANDLE_CHECKED(HeapObject, value, 1);
-  Handle<String> key = isolate->factory()->hidden_stack_trace_string();
-  if (value->IsUndefined()) {
-    error_object->DeleteHiddenProperty(*key);
-  } else {
-    RUNTIME_ASSERT(value->IsString());
-    JSObject::SetHiddenProperty(error_object, key, value);
-  }
-  return *error_object;
-}
-
-
 // Returns V8 version as a string.
 RUNTIME_FUNCTION(MaybeObject*, Runtime_GetV8Version) {
   SealHandleScope shs(isolate);

src/runtime.h

@@ -244,9 +244,7 @@ namespace internal {
   F(FunctionIsBuiltin, 1, 1) \
   F(GetScript, 1, 1) \
   F(CollectStackTrace, 3, 1) \
-  F(MarkOneShotGetter, 1, 1) \
+  F(GetAndClearOverflowedStackTrace, 1, 1) \
-  F(GetOverflowedStackTrace, 1, 1) \
-  F(SetOverflowedStackTrace, 2, 1) \
   F(GetV8Version, 0, 1) \
   \
   F(ClassOf, 1, 1) \

test/cctest/cctest.status

@@ -49,9 +49,6 @@ test-log/EquivalenceOfLoggingAndTraversal: PASS || FAIL
 test-weakmaps/Shrinking: FAIL
 test-weaksets/WeakSet_Shrinking: FAIL
 
-# Deferred stack trace formatting is temporarily disabled.
-test-heap/ReleaseStackTraceData: PASS || FAIL
-
 # Boot up memory use is bloated in debug mode.
 test-mark-compact/BootUpMemoryUse: PASS, PASS || FAIL if $mode == debug
 

test/cctest/test-heap.cc

@@ -2820,20 +2820,26 @@ class SourceResource: public v8::String::ExternalAsciiStringResource {
 };
 
 
-void ReleaseStackTraceDataTest(const char* source) {
+void ReleaseStackTraceDataTest(const char* source, const char* accessor) {
   // Test that the data retained by the Error.stack accessor is released
   // after the first time the accessor is fired.  We use external string
   // to check whether the data is being released since the external string
   // resource's callback is fired when the external string is GC'ed.
+  FLAG_use_ic = false;  // ICs retain objects.
   CcTest::InitializeVM();
   v8::HandleScope scope(CcTest::isolate());
   SourceResource* resource = new SourceResource(i::StrDup(source));
   {
     v8::HandleScope scope(CcTest::isolate());
     v8::Handle<v8::String> source_string = v8::String::NewExternal(resource);
+    HEAP->CollectAllAvailableGarbage();
     v8::Script::Compile(source_string)->Run();
     CHECK(!resource->IsDisposed());
   }
+  // HEAP->CollectAllAvailableGarbage();
+  CHECK(!resource->IsDisposed());
+
+  CompileRun(accessor);
   HEAP->CollectAllAvailableGarbage();
 
   // External source has been released.
@@ -2855,8 +2861,33 @@ TEST(ReleaseStackTraceData) {
                                "} catch (e) {                "
                                "  error = e;                 "
                                "}                            ";
-  ReleaseStackTraceDataTest(source1);
+  static const char* source3 = "var error = null;            "
-  ReleaseStackTraceDataTest(source2);
+  /* Normal Error */           "try {                        "
+  /* as prototype */           "  throw new Error();         "
+                               "} catch (e) {                "
+                               "  error = {};                "
+                               "  error.__proto__ = e;       "
+                               "}                            ";
+  static const char* source4 = "var error = null;            "
+  /* Stack overflow */         "try {                        "
+  /* as prototype   */         "  (function f() { f(); })(); "
+                               "} catch (e) {                "
+                               "  error = {};                "
+                               "  error.__proto__ = e;       "
+                               "}                            ";
+  static const char* getter = "error.stack";
+  static const char* setter = "error.stack = 0";
+
+  ReleaseStackTraceDataTest(source1, setter);
+  ReleaseStackTraceDataTest(source2, setter);
+  // We do not test source3 and source4 with setter, since the setter is
+  // supposed to (untypically) write to the receiver, not the holder.  This is
+  // to emulate the behavior of a data property.
+
+  ReleaseStackTraceDataTest(source1, getter);
+  ReleaseStackTraceDataTest(source2, getter);
+  ReleaseStackTraceDataTest(source3, getter);
+  ReleaseStackTraceDataTest(source4, getter);
 }
 
 

test/mjsunit/mjsunit.status

@@ -43,9 +43,6 @@ regress/regress-524: SKIP
 # This test non-deterministically runs out of memory on Windows ia32.
 regress/regress-crbug-160010: SKIP
 
-# Deferred stack trace formatting is temporarily disabled.
-stack-traces-gc: PASS || FAIL
-
 ##############################################################################
 # Too slow in debug mode with --stress-opt mode.
 compiler/regress-stacktrace-methods: PASS, SKIP if $mode == debug

test/mjsunit/stack-traces-gc.js

@@ -1,119 +0,0 @@
-// Copyright 2012 the V8 project authors. All rights reserved.
-// Redistribution and use in source and binary forms, with or without
-// modification, are permitted provided that the following conditions are
-// met:
-//
-//     * Redistributions of source code must retain the above copyright
-//       notice, this list of conditions and the following disclaimer.
-//     * Redistributions in binary form must reproduce the above
-//       copyright notice, this list of conditions and the following
-//       disclaimer in the documentation and/or other materials provided
-//       with the distribution.
-//     * Neither the name of Google Inc. nor the names of its
-//       contributors may be used to endorse or promote products derived
-//       from this software without specific prior written permission.
-//
-// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
-// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
-// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
-// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-// Flags: --expose-gc --allow-natives-syntax
-
-var fired = [];
-for (var i = 0; i < 100; i++) fired[i] = false;
-
-function getter_function(i) {
-  return %MarkOneShotGetter( function() { fired[i] = true; } );
-}
-
-// Error objects that die young.
-for (var i = 0; i < 100; i++) {
-  var error = new Error();
-  // Replace the getter to observe whether it has been fired,
-  // and disguise it as original getter.
-  var getter = getter_function(i);
-  error.__defineGetter__("stack", getter);
-
-  error = undefined;
-}
-
-gc();
-for (var i = 0; i < 100; i++) {
-  assertFalse(fired[i]);
-}
-
-// Error objects that are kept alive.
-var array = [];
-for (var i = 0; i < 100; i++) {
-  var error = new Error();
-  var getter = getter_function(i);
-  // Replace the getter to observe whether it has been fired,
-  // and disguise it as original getter.
-  error.__defineGetter__("stack", getter);
-
-  array.push(error);
-  error = undefined;
-}
-
-gc();
-// We don't expect all stack traces to be formatted after only one GC.
-assertTrue(fired[0]);
-
-for (var i = 0; i < 10; i++) gc();
-for (var i = 0; i < 100; i++) assertTrue(fired[i]);
-
-// Error objects with custom stack getter.
-var custom_error = new Error();
-var custom_getter_fired = false;
-custom_error.__defineGetter__("stack",
-                              function() { custom_getter_fired = true; });
-gc();
-assertFalse(custom_getter_fired);
-
-// Check that formatting caused by GC is not somehow observable.
-var error;
-
-var obj = { foo: function foo() { throw new Error(); } };
-
-try {
-  obj.foo();
-} catch (e) {
-  delete obj.foo;
-  Object.defineProperty(obj, 'foo', {
-    get: function() { assertUnreachable(); }
-  });
-  error = e;
-}
-
-gc();
-
-Object.defineProperty(Array.prototype, '0', {
-  get: function() { assertUnreachable(); }
-});
-
-try {
-  throw new Error();
-} catch (e) {
-  error = e;
-}
-
-gc();
-
-String.prototype.indexOf = function() { assertUnreachable(); };
-String.prototype.lastIndexOf = function() { assertUnreachable(); };
-var obj = { method: function() { throw Error(); } };
-try {
-  obj.method();
-} catch (e) {
-  error = e;
-}
-
-gc();