From 7930cea622f793519257ed9f19c58c759e98615f Mon Sep 17 00:00:00 2001 From: Tzvetan Mikov Date: Mon, 17 Feb 2025 14:46:28 -0800 Subject: [PATCH] JSError: never fail with invalid getter target" Summary: 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. Add and update tests. - Grafted path static_h to hermes Differential Revision: D69757360 --- 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