From fb60ada6ce3ba4c7208178170f7453876b34aad5 Mon Sep 17 00:00:00 2001
From: Linus Groh <mail@linusgroh.de>
Date: Tue, 15 Mar 2022 14:19:20 +0000
Subject: [PATCH] LibJS: Handle non-Error this object in Error.prototype.stack
 getter

This is taken from the abandoned error stacks proposal, which
already serves as the source of truth for the setter. It only requires
the this value to be an object - if it's not an Error object, the getter
returns undefined.
I have not compared this behavior to the non-standard implementations of
the stack property in other engines, but presumably the spec authors
already did that work.

This change gets the Sentry browser SDK working to a point where it can
actually send uncaught exceptions via the API :^)
---
 .../LibJS/Runtime/ErrorPrototype.cpp          | 21 ++++++++++++++-----
 .../builtins/Error/Error.prototype.stack.js   | 18 ++++++++++++++--
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/Userland/Libraries/LibJS/Runtime/ErrorPrototype.cpp b/Userland/Libraries/LibJS/Runtime/ErrorPrototype.cpp
index dd4563ed73..1167e37de1 100644
--- a/Userland/Libraries/LibJS/Runtime/ErrorPrototype.cpp
+++ b/Userland/Libraries/LibJS/Runtime/ErrorPrototype.cpp
@@ -69,17 +69,29 @@ JS_DEFINE_NATIVE_FUNCTION(ErrorPrototype::to_string)
     return js_string(vm, String::formatted("{}: {}", name, message));
 }
 
+// B.1.1 get Error.prototype.stack ( ), https://tc39.es/proposal-error-stacks/#sec-get-error.prototype-stack
 JS_DEFINE_NATIVE_FUNCTION(ErrorPrototype::stack_getter)
 {
-    auto* error = TRY(typed_this_value(global_object));
+    // 1. Let E be the this value.
+    // 2. If ! Type(E) is not Object, throw a TypeError exception.
+    auto* this_object = TRY(PrototypeObject::this_object(global_object));
+
+    // 3. If E does not have an [[ErrorData]] internal slot, return undefined.
+    if (!is<Error>(this_object))
+        return js_undefined();
+
+    auto& error = static_cast<Error&>(*this_object);
+
+    // 4. Return ? GetStackString(error).
+    // NOTE: These steps are not implemented based on the proposal, but to roughly follow behavior of other browsers.
 
     String name = "Error";
-    auto name_property = TRY(error->get(vm.names.name));
+    auto name_property = TRY(error.get(vm.names.name));
     if (!name_property.is_undefined())
         name = TRY(name_property.to_string(global_object));
 
     String message = "";
-    auto message_property = TRY(error->get(vm.names.message));
+    auto message_property = TRY(error.get(vm.names.message));
     if (!message_property.is_undefined())
         message = TRY(message_property.to_string(global_object));
 
@@ -87,8 +99,7 @@ JS_DEFINE_NATIVE_FUNCTION(ErrorPrototype::stack_getter)
     if (!message.is_empty())
         header = String::formatted("{}: {}", name, message);
 
-    return js_string(vm,
-        String::formatted("{}\n{}", header, error->stack_string()));
+    return js_string(vm, String::formatted("{}\n{}", header, error.stack_string()));
 }
 
 // B.1.2 set Error.prototype.stack ( value ), https://tc39.es/proposal-error-stacks/#sec-set-error.prototype-stack
diff --git a/Userland/Libraries/LibJS/Tests/builtins/Error/Error.prototype.stack.js b/Userland/Libraries/LibJS/Tests/builtins/Error/Error.prototype.stack.js
index 807cf7944d..3c845dbe5d 100644
--- a/Userland/Libraries/LibJS/Tests/builtins/Error/Error.prototype.stack.js
+++ b/Userland/Libraries/LibJS/Tests/builtins/Error/Error.prototype.stack.js
@@ -1,4 +1,5 @@
 const stackDescriptor = Object.getOwnPropertyDescriptor(Error.prototype, "stack");
+const stackGetter = stackDescriptor.get;
 const stackSetter = stackDescriptor.set;
 
 describe("getter - normal behavior", () => {
@@ -7,9 +8,9 @@ describe("getter - normal behavior", () => {
             /^    at .*Error \(.*\/Error\.prototype\.stack\.js:\d+:\d+\)$/,
             /^    at .+\/Error\/Error\.prototype\.stack\.js:\d+:\d+$/,
             /^    at test \(.+\/test-common.js:557:21\)$/,
-            /^    at .+\/Error\/Error\.prototype\.stack\.js:5:33$/,
+            /^    at .+\/Error\/Error\.prototype\.stack\.js:6:33$/,
             /^    at describe \(.+\/test-common\.js:534:21\)$/,
-            /^    at .+\/Error\/Error\.prototype\.stack\.js:4:38$/,
+            /^    at .+\/Error\/Error\.prototype\.stack\.js:5:38$/,
         ];
         const values = [
             {
@@ -41,6 +42,19 @@ describe("getter - normal behavior", () => {
             }
         }
     });
+
+    test("this value must be an object", () => {
+        expect(() => {
+            stackGetter.call("foo");
+        }).toThrowWithMessage(TypeError, "foo is not an object");
+        expect(() => {
+            stackGetter.call(42);
+        }).toThrowWithMessage(TypeError, "42 is not an object");
+    });
+
+    test("returns undefined when called with non-Error object this value", () => {
+        expect(stackGetter.call({})).toBeUndefined();
+    });
 });
 
 describe("setter - normal behavior", () => {