From c615b115dfa471c108256982ce07f10adee63667 Mon Sep 17 00:00:00 2001 From: Tzvetan Mikov Date: Mon, 17 Feb 2025 20:01:43 -0800 Subject: [PATCH] JSError: never fail with invalid getter target" (#1621) Summary: Pull Request resolved: https://github.com/facebook/hermes/pull/1621 JS developers expect this to work and print "undefined": ```js function MyEror() { this.message = "MyError"; } MyError.prototype = Object.create(Error.prototype); try { throw new MyError(); } catch (e) { print(e.stack); } ``` We give it to them by preventing the getter from throwing errors. In V8 this is implemented differently by attaching the getter to the actual instance. Arguably, this makes sense, because property reads shouldn't throw. Add and update tests. - Grafted path static_h to hermes Reviewed By: avp Differential Revision: D69757360 fbshipit-source-id: 216e870e82c941a985020202dc14aa5948164da8 --- include/hermes/VM/JSError.h | 9 -------- lib/VM/JSError.cpp | 17 +++++++++------- test/hermes/error-capture-stack-trace.js | 8 ++++---- test/hermes/error-in-proto.js | 13 ++++++++++++ test/hermes/error.js | 8 ++------ test/hermes/regress-proxy-parent.js | 26 ++++++++---------------- 6 files changed, 38 insertions(+), 43 deletions(-) diff --git a/include/hermes/VM/JSError.h b/include/hermes/VM/JSError.h index cbf5006fb95..1a54be011c6 100644 --- a/include/hermes/VM/JSError.h +++ b/include/hermes/VM/JSError.h @@ -205,15 +205,6 @@ class JSError final : public JSObject { Handle selfHandle, size_t index, llvh::SmallVectorImpl &str); - - /// Given an object \p targetHandle: - /// 1. If its [[CapturedError]] slot has a non-null handle, return it as a - /// JSError. - /// 2. Otherwise, return \t targetHandle cast to JSError. - /// Throws if any cast or property access fails. - static CallResult> getErrorFromStackTarget( - Runtime &runtime, - Handle targetHandle); }; } // namespace vm diff --git a/lib/VM/JSError.cpp b/lib/VM/JSError.cpp index fc8c9801373..566545d1c01 100644 --- a/lib/VM/JSError.cpp +++ b/lib/VM/JSError.cpp @@ -54,7 +54,11 @@ void JSErrorBuildMeta(const GCCell *cell, Metadata::Builder &mb) { mb.addField("domains", &self->domains_); } -CallResult> JSError::getErrorFromStackTarget( +/// Given an object \p targetHandle which may be nullptr: +/// 1. Look for [[CapturedError]] in the object or its prototype chain and +/// return it a JSError. +/// 2. Otherwise, return llvh::None. +static llvh::Optional> getErrorFromStackTarget( Runtime &runtime, Handle targetHandle) { MutableHandle mutHnd = @@ -78,8 +82,7 @@ CallResult> JSError::getErrorFromStackTarget( mutHnd.set(targetHandle->getParent(runtime)); } - return runtime.raiseTypeError( - "Error.stack getter called with an invalid receiver"); + return llvh::None; } CallResult @@ -87,11 +90,11 @@ errorStackGetter(void *, Runtime &runtime, NativeArgs args) { GCScope gcScope(runtime); auto targetHandle = args.dyncastThis(); - auto errorHandleRes = JSError::getErrorFromStackTarget(runtime, targetHandle); - if (errorHandleRes == ExecutionStatus::EXCEPTION) { - return ExecutionStatus::EXCEPTION; + auto errorHandleOpt = getErrorFromStackTarget(runtime, targetHandle); + if (!errorHandleOpt.hasValue()) { + return HermesValue::encodeUndefinedValue(); } - auto errorHandle = *errorHandleRes; + auto errorHandle = *errorHandleOpt; if (!errorHandle->stacktrace_) { // Stacktrace has not been set, we simply return empty string. // This is different from other VMs where stacktrace is created when diff --git a/test/hermes/error-capture-stack-trace.js b/test/hermes/error-capture-stack-trace.js index f8daeaba327..2e44bc8bebb 100644 --- a/test/hermes/error-capture-stack-trace.js +++ b/test/hermes/error-capture-stack-trace.js @@ -161,7 +161,7 @@ print(target.stack); var dummy = {}; Error.captureStackTrace(dummy); const stackGetter = Object.getOwnPropertyDescriptor(dummy, 'stack').get; -try { stackGetter.apply({}); } catch (e) { print(e); } -//CHECK: TypeError: Error.stack getter called with an invalid receiver -try { stackGetter.apply(null); } catch (e) { print(e); } -//CHECK: TypeError: Error.stack getter called with an invalid receiver +print(stackGetter.apply({})); +//CHECK: undefined +print(stackGetter.apply(null)); +//CHECK: undefined diff --git a/test/hermes/error-in-proto.js b/test/hermes/error-in-proto.js index 250acb86660..f8f2a271c2b 100644 --- a/test/hermes/error-in-proto.js +++ b/test/hermes/error-in-proto.js @@ -24,3 +24,16 @@ try { } // CHECK: Caught Error MyError: 1234 // CHECK-NEXT: at global{{.*}} + + +function MyError2() { + this.message = "MyError2"; +} +MyError2.prototype = Object.create(Error.prototype); +try { + throw new MyError2(); +} catch (e) { + print("Caught", e, e.stack); +} + +// CHECK-NEXT: Caught Error: MyError2 undefined diff --git a/test/hermes/error.js b/test/hermes/error.js index 480d04b9962..a8f697a2120 100644 --- a/test/hermes/error.js +++ b/test/hermes/error.js @@ -90,12 +90,8 @@ print(e); // CHECK-NEXT: 12345: 67890 // Check exception case of accessing Error.stack from a different 'this' -try { - new Error().__lookupGetter__("stack").call({}); -} catch (e) { - print(e.name); -} -//CHECK-NEXT: TypeError +print(new Error().__lookupGetter__("stack").call({})); +// CHECK-NEXT: undefined // Regression test: setting the stack while getting the message causes a null dereference. diff --git a/test/hermes/regress-proxy-parent.js b/test/hermes/regress-proxy-parent.js index 72aa758b8d9..41f6a4cdc64 100644 --- a/test/hermes/regress-proxy-parent.js +++ b/test/hermes/regress-proxy-parent.js @@ -15,22 +15,14 @@ // Install stack trace information on Object.prototype. Error.captureStackTrace(Object.prototype); -// Check that lookup on a regular proxy fails. -try{ - var p = new Proxy(new Error(), {}); - p.stack; -} catch (e) { - print(e); -} -// CHECK: TypeError: Error.stack getter called with an invalid receiver +// Check that lookup on a regular proxy returns undefined. +var p = new Proxy(new Error(), {}); +print(p.stack); +// CHECK: undefined // Check that lookup on a callable proxy fails. -try { - function foo(){} - foo.__proto__ = new Error(); - var cp = new Proxy(foo, {}); - cp.stack; -} catch (e) { - print(e); -} -// CHECK-NEXT: TypeError: Error.stack getter called with an invalid receiver +function foo(){} +foo.__proto__ = new Error(); +var cp = new Proxy(foo, {}); +print(cp.stack); +// CHECK-NEXT: undefined