[stack-trace] Use ErrorStack accessor for formatted stack traces

When a stack trace is captured, it is stored in a private symbol on
the respective Error object. The first access to "Error.stack"  will
then format the stack trace, with a possible call into user JS via
the Error.prepareStackTrace callback.

Until now, the accessor converted ".stack" to a normal data
property containing the formatted stack trace. This causes a new Map
with a new DescriptorArray to be created, which will not be shared
with anything else (also not other error objects with formated
stack traces).

This CL changes the accessor to store the formatted stack trace in
the same symbol (stack_trace_symbol) as the structured data. The
result is that an error object will have the same Map before and
after "Error.stack" is accessed.

Bug: v8:9115
Change-Id: I7d6bf49be76d63b57fbbaf904cc6ed7dbdbfb96b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1564061
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60865}

src/accessors.cc

@@ -786,35 +786,6 @@ Handle<AccessorInfo> Accessors::MakeBoundFunctionNameInfo(Isolate* isolate) {
 // Accessors::ErrorStack
 //
 
-namespace {
-
-MaybeHandle<JSReceiver> ClearInternalStackTrace(Isolate* isolate,
-                                                Handle<JSObject> error) {
-  RETURN_ON_EXCEPTION(
-      isolate,
-      Object::SetProperty(
-          isolate, error, isolate->factory()->stack_trace_symbol(),
-          isolate->factory()->undefined_value(), StoreOrigin::kMaybeKeyed,
-          Just(ShouldThrow::kThrowOnError)),
-      JSReceiver);
-  return error;
-}
-
-bool IsAccessor(Handle<Object> receiver, Handle<Name> name,
-                Handle<JSObject> holder) {
-  LookupIterator it(receiver, name, holder,
-                    LookupIterator::OWN_SKIP_INTERCEPTOR);
-  // Skip any access checks we might hit. This accessor should never hit in a
-  // situation where the caller does not have access.
-  if (it.state() == LookupIterator::ACCESS_CHECK) {
-    CHECK(it.HasAccess());
-    it.Next();
-  }
-  return (it.state() == LookupIterator::ACCESSOR);
-}
-
-}  // namespace
-
 void Accessors::ErrorStackGetter(
     v8::Local<v8::Name> key, const v8::PropertyCallbackInfo<v8::Value>& info) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
@@ -822,7 +793,9 @@ void Accessors::ErrorStackGetter(
   Handle<JSObject> holder =
       Handle<JSObject>::cast(Utils::OpenHandle(*info.Holder()));
 
-  // Retrieve the structured stack trace.
+  // Retrieve the stack trace. It can either be structured data in the form of
+  // a FrameArray, an already formatted stack trace (string) or whatever the
+  // "prepareStackTrace" callback produced.
 
   Handle<Object> stack_trace;
   Handle<Symbol> stack_trace_symbol = isolate->factory()->stack_trace_symbol();
@@ -835,8 +808,15 @@ void Accessors::ErrorStackGetter(
     return;
   }
 
-  // Format it, clear the internal structured trace and reconfigure as a data
-  // property.
+  // Only format the stack-trace the first time around. The check for a
+  // FixedArray is sufficient as the user callback can not create plain
+  // FixedArrays and the result is a String in case we format the stack
+  // trace ourselves.
+
+  if (!stack_trace->IsFixedArray()) {
+    info.GetReturnValue().Set(Utils::ToLocal(stack_trace));
+    return;
+  }
 
   Handle<Object> formatted_stack_trace;
   if (!ErrorUtils::FormatStackTrace(isolate, holder, stack_trace)
@@ -845,34 +825,16 @@ void Accessors::ErrorStackGetter(
     return;
   }
 
-  MaybeHandle<Object> result = ClearInternalStackTrace(isolate, holder);
+  // Replace the structured stack-trace with the formatting result.
+  MaybeHandle<Object> result = Object::SetProperty(
+      isolate, holder, isolate->factory()->stack_trace_symbol(),
+      formatted_stack_trace, StoreOrigin::kMaybeKeyed,
+      Just(ShouldThrow::kThrowOnError));
   if (result.is_null()) {
     isolate->OptionalRescheduleException(false);
     return;
   }
 
-  // If stack is still an accessor (this could have changed in the meantime
-  // since FormatStackTrace can execute arbitrary JS), replace it with a data
-  // property.
-  Handle<Object> receiver =
-      Utils::OpenHandle(*v8::Local<v8::Value>(info.This()));
-  Handle<Name> name = Utils::OpenHandle(*key);
-  if (IsAccessor(receiver, name, holder)) {
-    result = Accessors::ReplaceAccessorWithDataProperty(receiver, holder, name,
-                                                        formatted_stack_trace);
-    if (result.is_null()) {
-      isolate->OptionalRescheduleException(false);
-      return;
-    }
-  } else {
-    // The stack property has been modified in the meantime.
-    if (!JSObject::GetProperty(isolate, holder, name)
-             .ToHandle(&formatted_stack_trace)) {
-      isolate->OptionalRescheduleException(false);
-      return;
-    }
-  }
-
   v8::Local<v8::Value> value = Utils::ToLocal(formatted_stack_trace);
   info.GetReturnValue().Set(value);
 }
@@ -884,14 +846,17 @@ void Accessors::ErrorStackSetter(
   HandleScope scope(isolate);
   Handle<JSObject> obj = Handle<JSObject>::cast(
       Utils::OpenHandle(*v8::Local<v8::Value>(info.This())));
+  Handle<Object> value = Handle<Object>::cast(Utils::OpenHandle(*val));
 
-  // Clear internal properties to avoid memory leaks.
-  Handle<Symbol> stack_trace_symbol = isolate->factory()->stack_trace_symbol();
-  if (JSReceiver::HasOwnProperty(obj, stack_trace_symbol).FromMaybe(false)) {
-    ClearInternalStackTrace(isolate, obj);
+  // Store the value in the internal symbol to avoid reconfiguration to
+  // a data property.
+  MaybeHandle<Object> result = Object::SetProperty(
+      isolate, obj, isolate->factory()->stack_trace_symbol(), value,
+      StoreOrigin::kMaybeKeyed, Just(ShouldThrow::kThrowOnError));
+  if (result.is_null()) {
+    isolate->OptionalRescheduleException(false);
+    return;
   }
-
-  Accessors::ReconfigureToDataProperty(name, val, info);
 }
 
 Handle<AccessorInfo> Accessors::MakeErrorStackInfo(Isolate* isolate) {

test/mjsunit/error-stack.js

+// Copyright 2019 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.
+//
+// Flags: --allow-natives-syntax
+
+(function TestErrorObjectsRetainMap() {
+  const error1 = new Error("foo");
+  const error2 = new Error("bar");
+
+  assertTrue(%HaveSameMap(error1, error2));
+
+  // Trigger serialization of the stack-trace.
+  error1.stack;
+  assertTrue(%HaveSameMap(error1, error2));
+
+  error2.stack;
+  assertTrue(%HaveSameMap(error1, error2));
+})();
+
+(function TestPrepareStackTraceCallback() {
+  Error.prepareStackTrace = (error, frame) => {
+    return "custom stack trace No. 42";
+  };
+
+  const error = new Error("foo");
+
+  // Check it twice, so both code paths in the accessor are exercised.
+  assertEquals(error.stack, "custom stack trace No. 42");
+  assertEquals(error.stack, "custom stack trace No. 42");
+})();
+
+(function TestPrepareStackTraceCallbackMessesWithProperty() {
+  Error.prepareStackTrace = (error, frames) => {
+    error.stack = "Yes, we can write to this!";
+    return 42;
+  };
+
+  const error = new Error("foo");
+
+  // Check it twice. The first returns the formatting result,
+  // the second the value of the private symbol.
+  assertEquals(error.stack, 42);
+  assertEquals(error.stack, 42);
+})();
+
+(function TestPrepareStackTraceCallbackInstallsGetter() {
+  Error.prepareStackTrace = (error, frames) => {
+    Object.defineProperty(error, "stack", { get: () => 42 });
+    return "<formatted stack trace>";
+  };
+
+  const error = new Error("foo");
+
+  // Check it twice. The second time the accessor should be used.
+  assertEquals(error.stack, "<formatted stack trace>");
+  assertEquals(error.stack, 42);
+})();
+
+(function TestPrepareStackTraceCallbackInstallsSetter() {
+  Error.prepareStackTrace = (error, frames) => {
+    Object.defineProperty(error, "stack", { set: (x) => {
+      error[42] = x;
+    }});
+    return "<formatted stack trace>";
+  };
+
+  const error = new Error("foo");
+  // Cause the accessor to get installed.
+  error.stack;
+
+  error.stack = "Who needs stack traces anyway?";
+  assertEquals(error[42], "Who needs stack traces anyway?");
+  assertEquals(error.stack, undefined); // No getter.
+})();

test/mjsunit/stack-traces.js

@@ -368,13 +368,17 @@ assertEquals(undefined, fake_error.stack);
 
 // Check that overwriting the stack property during stack trace formatting
 // does not crash.
-error = new Error();
+error = new Error("foobar");
 error.__defineGetter__("name", function() { error.stack = "abc"; });
-assertEquals("abc", error.stack);
+assertTrue(error.stack.startsWith("Error"));
+assertTrue(error.stack.includes("foobar"));
 
-error = new Error();
+error = new Error("foobar");
 error.__defineGetter__("name", function() { delete error.stack; });
-assertEquals(undefined, error.stack);
+// The first time, Error.stack returns the formatted stack trace,
+// not the content of the property.
+assertTrue(error.stack.startsWith("Error"));
+assertEquals(error.stack, undefined);
 
 // Check that repeated trace collection does not crash.
 error = new Error();