JSError: never fail with invalid getter target" (#1621)

Summary:
Pull Request resolved: #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

@@ -205,15 +205,6 @@ class JSError final : public JSObject {
       Handle<JSError> selfHandle,
       size_t index,
       llvh::SmallVectorImpl<char16_t> &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<Handle<JSError>> getErrorFromStackTarget(
-      Runtime &runtime,
-      Handle<JSObject> targetHandle);
 };
 
 } // namespace vm

lib/VM/JSError.cpp

@@ -54,7 +54,11 @@ void JSErrorBuildMeta(const GCCell *cell, Metadata::Builder &mb) {
   mb.addField("domains", &self->domains_);
 }
 
-CallResult<Handle<JSError>> 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<Handle<JSError>> getErrorFromStackTarget(
     Runtime &runtime,
     Handle<JSObject> targetHandle) {
   MutableHandle<JSObject> mutHnd =
@@ -78,20 +82,19 @@ CallResult<Handle<JSError>> JSError::getErrorFromStackTarget(
 
     mutHnd.set(targetHandle->getParent(runtime));
   }
-  return runtime.raiseTypeError(
-      "Error.stack getter called with an invalid receiver");
+  return llvh::None;
 }
 
 CallResult<HermesValue>
 errorStackGetter(void *, Runtime &runtime, NativeArgs args) {
   GCScope gcScope(runtime);
 
   auto targetHandle = args.dyncastThis<JSObject>();
-  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

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

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

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.

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