Move error stack accessor to Error.prototype

Summary: Move the stack accessors on Errors to the prototype. This makes them easy to globally replace, which is useful for synth traces. Reviewed By: avp Differential Revision: D60323579 fbshipit-source-id: 283795a4dd5f4863b3a87b978da5991e48c1d92f
facebook · Aug 1, 2024 · 9c54f43 · 9c54f43
1 parent 20b13b9
commit 9c54f43
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 70 deletions.
diff --git a/lib/VM/JSError.cpp b/lib/VM/JSError.cpp
@@ -217,63 +217,6 @@ PseudoHandle<JSError> JSError::create(
   return JSObjectInit::initToPseudoHandle(runtime, cell);
 }
 
-ExecutionStatus JSError::setupStack(
-    Handle<JSObject> selfHandle,
-    Runtime &runtime) {
-  // Lazily allocate the accessor.
-  if (runtime.jsErrorStackAccessor.isUndefined()) {
-    // This code path allocates quite a few handles, so make sure we
-    // don't disturb the parent GCScope and free them.
-    GCScope gcScope{runtime};
-
-    auto getter = NativeFunction::create(
-        runtime,
-        Handle<JSObject>::vmcast(&runtime.functionPrototype),
-        nullptr,
-        errorStackGetter,
-        Predefined::getSymbolID(Predefined::emptyString),
-        0,
-        Runtime::makeNullHandle<JSObject>());
-
-    auto setter = NativeFunction::create(
-        runtime,
-        Handle<JSObject>::vmcast(&runtime.functionPrototype),
-        nullptr,
-        errorStackSetter,
-        Predefined::getSymbolID(Predefined::emptyString),
-        1,
-        Runtime::makeNullHandle<JSObject>());
-
-    runtime.jsErrorStackAccessor =
-        PropertyAccessor::create(runtime, getter, setter);
-  }
-
-  auto accessor =
-      Handle<PropertyAccessor>::vmcast(&runtime.jsErrorStackAccessor);
-
-  DefinePropertyFlags dpf{};
-  dpf.setEnumerable = 1;
-  dpf.setConfigurable = 1;
-  dpf.setGetter = 1;
-  dpf.setSetter = 1;
-  dpf.enumerable = 0;
-  dpf.configurable = 1;
-
-  auto res = JSObject::defineOwnProperty(
-      selfHandle,
-      runtime,
-      Predefined::getSymbolID(Predefined::stack),
-      dpf,
-      accessor);
-
-  // Ignore failures to set the "stack" property as other engines do.
-  if (LLVM_UNLIKELY(res == ExecutionStatus::EXCEPTION)) {
-    runtime.clearThrownValue();
-  }
-
-  return ExecutionStatus::RETURNED;
-}
-
 CallResult<Handle<StringPrimitive>> JSError::toString(
     Handle<JSObject> O,
     Runtime &runtime) {

diff --git a/lib/VM/JSLib/Error.cpp b/lib/VM/JSLib/Error.cpp
@@ -14,6 +14,7 @@
 
 #include "hermes/VM/CallResult.h"
 #include "hermes/VM/Operations.h"
+#include "hermes/VM/PropertyAccessor.h"
 #include "hermes/VM/StringBuilder.h"
 #include "hermes/VM/StringPrimitive.h"
 
@@ -36,7 +37,7 @@ Handle<JSObject> createErrorConstructor(Runtime &runtime) {
       0);
 
   // Error.prototype.xxx properties.
-  // Error.prototype has two own properties: name and message.
+  // Error.prototype has three own properties: name, message, and stack.
   auto defaultName = runtime.getPredefinedString(Predefined::Error);
   defineProperty(
       runtime,
@@ -51,6 +52,49 @@ Handle<JSObject> createErrorConstructor(Runtime &runtime) {
       Predefined::getSymbolID(Predefined::message),
       runtime.makeHandle(HermesValue::encodeStringValue(defaultMessage)));
 
+  auto getter = NativeFunction::create(
+      runtime,
+      Handle<JSObject>::vmcast(&runtime.functionPrototype),
+      nullptr,
+      errorStackGetter,
+      Predefined::getSymbolID(Predefined::emptyString),
+      0,
+      Runtime::makeNullHandle<JSObject>());
+
+  auto setter = NativeFunction::create(
+      runtime,
+      Handle<JSObject>::vmcast(&runtime.functionPrototype),
+      nullptr,
+      errorStackSetter,
+      Predefined::getSymbolID(Predefined::emptyString),
+      1,
+      Runtime::makeNullHandle<JSObject>());
+
+  // Save the accessors on the runtime so we can use them for captureStackTrace.
+  runtime.jsErrorStackAccessor =
+      PropertyAccessor::create(runtime, getter, setter);
+
+  auto accessor =
+      Handle<PropertyAccessor>::vmcast(&runtime.jsErrorStackAccessor);
+
+  DefinePropertyFlags dpf{};
+  dpf.setEnumerable = 1;
+  dpf.setConfigurable = 1;
+  dpf.setGetter = 1;
+  dpf.setSetter = 1;
+  dpf.enumerable = 0;
+  dpf.configurable = 1;
+
+  auto stackRes = JSObject::defineOwnProperty(
+      errorPrototype,
+      runtime,
+      Predefined::getSymbolID(Predefined::stack),
+      dpf,
+      accessor);
+  (void)stackRes;
+
+  assert(*stackRes && "Failed to define stack accessor");
+
   auto cons = defineSystemConstructor<JSError>(
       runtime,
       Predefined::getSymbolID(Predefined::Error),
@@ -123,13 +167,6 @@ static CallResult<HermesValue> constructErrorObject(
     return ExecutionStatus::EXCEPTION;
   }
 
-  // Initialize stack accessor.
-  if (LLVM_UNLIKELY(
-          JSError::setupStack(selfHandle, runtime) ==
-          ExecutionStatus::EXCEPTION)) {
-    return ExecutionStatus::EXCEPTION;
-  }
-
   // new Error(message).
   // Only proceed when 'typeof message' isn't undefined.
   if (!message->isUndefined()) {
@@ -320,10 +357,24 @@ errorCaptureStackTrace(void *, Runtime &runtime, NativeArgs args) {
   }
 
   // Initialize the `stack` accessor on the target object.
-  if (LLVM_UNLIKELY(
-          JSError::setupStack(targetHandle, runtime) ==
-          ExecutionStatus::EXCEPTION)) {
-    return ExecutionStatus::EXCEPTION;
+  DefinePropertyFlags dpf{};
+  dpf.setEnumerable = 1;
+  dpf.setConfigurable = 1;
+  dpf.setGetter = 1;
+  dpf.setSetter = 1;
+  dpf.enumerable = 0;
+  dpf.configurable = 1;
+
+  auto stackRes = JSObject::defineOwnProperty(
+      targetHandle,
+      runtime,
+      Predefined::getSymbolID(Predefined::stack),
+      dpf,
+      Handle<>(&runtime.jsErrorStackAccessor));
+
+  // Ignore failures to set the "stack" property as other engines do.
+  if (LLVM_UNLIKELY(stackRes == ExecutionStatus::EXCEPTION)) {
+    runtime.clearThrownValue();
   }
 
   return HermesValue::encodeUndefinedValue();

diff --git a/lib/VM/Runtime.cpp b/lib/VM/Runtime.cpp
@@ -1281,7 +1281,6 @@ uint32_t Runtime::getCurrentFrameOffset() const {
 static ExecutionStatus
 raisePlaceholder(Runtime &runtime, Handle<JSError> errorObj, Handle<> message) {
   JSError::recordStackTrace(errorObj, runtime);
-  JSError::setupStack(errorObj, runtime);
   JSError::setMessage(errorObj, runtime, message);
   return runtime.setThrownValue(errorObj.getHermesValue());
 }

diff --git a/utils/testsuite/testsuite_skiplist.py b/utils/testsuite/testsuite_skiplist.py
@@ -1287,6 +1287,8 @@
     "mjsunit/regress/regress-prepare-break-while-recompile.js",
     # Fails when ASAN/UBSAN are limiting the max native stack depth.
     "mjsunit/compiler/regress-lazy-deopt.js",
+    # Error stack getter is on Error.prototype in Hermes.
+    "mjsunit/regress/regress-3404.js",
     # Uncategorized mjsunit failures
     "mjsunit/accessor-map-sharing.js",
     "mjsunit/accessors-on-global-object.js",