Improve error messages when JS code throws exceptions (#173)

pedrovgs · j-f1 · web-flow · commit 6ae7311fc4c7 · 2022-03-31T17:34:57.000+09:00
* Remove a not neede try/catch when evaluating JSFunction values so we can improve error reporting by not hiding the information we get form the JS crash

* Modify JSThrowingFunction to be based on the safe version of  _call_function_with_this and _call_function instead of the unsafe version we created

* Update swjs_call_function_unsafe to notify wasm when there is an exception

Co-authored-by: Jed Fox &lt;git@jedfox.com&gt;

* Modify swjs_call_function_with_this_unsafe to notify WASM there was an exception as Jed suggested

* Recover the already written test cases for JSThrowingFunction and create a new test case for the new functionality

* Rename unsafe fuctions to no_catch and simplify invokeNonThrowingJSFunction implementation

* Bump swjs_library_version version

Co-authored-by: Jed Fox &lt;git@jedfox.com&gt;
diff --git a/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift b/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift
@@ -103,6 +103,12 @@ func expectThrow<T>(_ body: @autoclosure () throws -> T, file: StaticString = #f
     throw MessageError("Expect to throw an exception", file: file, line: line, column: column)
 }
 
+func wrapUnsafeThrowableFunction(_ body: @escaping () -> Void, file: StaticString = #file, line: UInt = #line, column: UInt = #column) throws -> Error {
+    JSObject.global.callThrowingClosure.function!(JSClosure { _ in 
+            body() 
+            return .undefined
+    })
+}
 func expectNotNil<T>(_ value: T?, file: StaticString = #file, line: UInt = #line, column: UInt = #column) throws {
     switch value {
     case .some: return
diff --git a/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift b/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift
@@ -700,7 +700,6 @@ try test("Exception") {
     // }
     // ```
     //
-
     let globalObject1 = JSObject.global.globalObject1
     let prop_9: JSValue = globalObject1.prop_9
 
@@ -735,6 +734,46 @@ try test("Exception") {
     try expectNotNil(errorObject3)
 }
 
+try test("Unhandled Exception") {
+    // ```js
+    // global.globalObject1 = {
+    //   ...
+    //   prop_9: {
+    //       func1: function () {
+    //           throw new Error();
+    //       },
+    //       func2: function () {
+    //           throw "String Error";
+    //       },
+    //       func3: function () {
+    //           throw 3.0
+    //       },
+    //   },
+    //   ...
+    // }
+    // ```
+    //
+
+    let globalObject1 = JSObject.global.globalObject1
+    let prop_9: JSValue = globalObject1.prop_9
+
+    // MARK: Throwing method calls
+    let error1 = try wrapUnsafeThrowableFunction { _ = prop_9.object!.func1!() }
+    try expectEqual(error1 is JSValue, true)
+    let errorObject = JSError(from: error1 as! JSValue)
+    try expectNotNil(errorObject)
+
+    let error2 = try wrapUnsafeThrowableFunction { _ = prop_9.object!.func2!() }
+    try expectEqual(error2 is JSValue, true)
+    let errorString = try expectString(error2 as! JSValue)
+    try expectEqual(errorString, "String Error")
+
+    let error3 = try wrapUnsafeThrowableFunction { _ = prop_9.object!.func3!() }
+    try expectEqual(error3 is JSValue, true)
+    let errorNumber = try expectNumber(error3 as! JSValue)
+    try expectEqual(errorNumber, 3.0)
+}
+
 /// If WebAssembly.Memory is not accessed correctly (i.e. creating a new view each time),
 /// this test will fail with `TypeError: Cannot perform Construct on a detached ArrayBuffer`,
 /// since asking to grow memory will detach the backing ArrayBuffer.
diff --git a/IntegrationTests/bin/primary-tests.js b/IntegrationTests/bin/primary-tests.js
@@ -82,6 +82,14 @@ global.Animal = function (name, age, isCat) {
     }
 };
 
+global.callThrowingClosure = (c) => { 
+    try {
+        c() 
+    } catch (error) {
+        return error
+    }
+}
+
 const { startWasiTask } = require("../lib");
 
 startWasiTask("./dist/PrimaryTests.wasm").catch((err) => {
diff --git a/Runtime/src/index.ts b/Runtime/src/index.ts
@@ -14,7 +14,7 @@ export class SwiftRuntime {
     private _instance: WebAssembly.Instance | null;
     private _memory: Memory | null;
     private _closureDeallocator: SwiftClosureDeallocator | null;
-    private version: number = 705;
+    private version: number = 706;
 
     private textDecoder = new TextDecoder("utf-8");
     private textEncoder = new TextEncoder(); // Only support utf-8
@@ -226,6 +226,44 @@ export class SwiftRuntime {
                 this.memory
             );
         },
+        swjs_call_function_no_catch: (
+            ref: ref,
+            argv: pointer,
+            argc: number,
+            kind_ptr: pointer,
+            payload1_ptr: pointer,
+            payload2_ptr: pointer
+        ) => {
+            const func = this.memory.getObject(ref);
+            let isException = true;
+            try {
+                const result = Reflect.apply(
+                    func,
+                    undefined,
+                    JSValue.decodeArray(argv, argc, this.memory)
+                );
+                JSValue.write(
+                    result,
+                    kind_ptr,
+                    payload1_ptr,
+                    payload2_ptr,
+                    false,
+                    this.memory
+                );
+                isException = false;
+            } finally {
+                if (isException) {
+                    JSValue.write(
+                        undefined,
+                        kind_ptr,
+                        payload1_ptr,
+                        payload2_ptr,
+                        true,
+                        this.memory
+                    );
+                }
+            }
+        },
 
         swjs_call_function_with_this: (
             obj_ref: ref,
@@ -265,6 +303,47 @@ export class SwiftRuntime {
                 this.memory
             );
         },
+
+        swjs_call_function_with_this_no_catch: (
+            obj_ref: ref,
+            func_ref: ref,
+            argv: pointer,
+            argc: number,
+            kind_ptr: pointer,
+            payload1_ptr: pointer,
+            payload2_ptr: pointer
+        ) => {
+            const obj = this.memory.getObject(obj_ref);
+            const func = this.memory.getObject(func_ref);
+            let isException = true;
+            try {
+                const result = Reflect.apply(
+                    func,
+                    obj,
+                    JSValue.decodeArray(argv, argc, this.memory)
+                );
+                JSValue.write(
+                    result,
+                    kind_ptr,
+                    payload1_ptr,
+                    payload2_ptr,
+                    false,
+                    this.memory
+                );
+                isException = false
+            } finally {
+                if (isException) {
+                    JSValue.write(
+                        undefined,
+                        kind_ptr,
+                        payload1_ptr,
+                        payload2_ptr,
+                        true,
+                        this.memory
+                    );
+                }
+            }
+        },
         swjs_call_new: (ref: ref, argv: pointer, argc: number) => {
             const constructor = this.memory.getObject(ref);
             const instance = Reflect.construct(
diff --git a/Runtime/src/types.ts b/Runtime/src/types.ts
@@ -58,6 +58,14 @@ export interface ImportedFunctions {
         payload1_ptr: pointer,
         payload2_ptr: pointer
     ): void;
+    swjs_call_function_no_catch(
+        ref: number,
+        argv: pointer,
+        argc: number,
+        kind_ptr: pointer,
+        payload1_ptr: pointer,
+        payload2_ptr: pointer
+    ): void;
     swjs_call_function_with_this(
         obj_ref: ref,
         func_ref: ref,
@@ -67,6 +75,15 @@ export interface ImportedFunctions {
         payload1_ptr: pointer,
         payload2_ptr: pointer
     ): void;
+    swjs_call_function_with_this_no_catch(
+        obj_ref: ref,
+        func_ref: ref,
+        argv: pointer,
+        argc: number,
+        kind_ptr: pointer,
+        payload1_ptr: pointer,
+        payload2_ptr: pointer
+    ): void;
     swjs_call_new(ref: number, argv: pointer, argc: number): number;
     swjs_call_throwing_new(
         ref: number,
diff --git a/Sources/JavaScriptKit/FundamentalObjects/JSFunction.swift b/Sources/JavaScriptKit/FundamentalObjects/JSFunction.swift
@@ -19,7 +19,7 @@ public class JSFunction: JSObject {
     /// - Returns: The result of this call.
     @discardableResult
     public func callAsFunction(this: JSObject? = nil, arguments: [ConvertibleToJSValue]) -> JSValue {
-        try! invokeJSFunction(self, arguments: arguments, this: this)
+        invokeNonThrowingJSFunction(self, arguments: arguments, this: this)
     }
 
     /// A variadic arguments version of `callAsFunction`.
@@ -84,30 +84,27 @@ public class JSFunction: JSObject {
     }
 }
 
-internal func invokeJSFunction(_ jsFunc: JSFunction, arguments: [ConvertibleToJSValue], this: JSObject?) throws -> JSValue {
-    let (result, isException) = arguments.withRawJSValues { rawValues in
-        rawValues.withUnsafeBufferPointer { bufferPointer -> (JSValue, Bool) in
+private func invokeNonThrowingJSFunction(_ jsFunc: JSFunction, arguments: [ConvertibleToJSValue], this: JSObject?) -> JSValue {
+    arguments.withRawJSValues { rawValues in
+        rawValues.withUnsafeBufferPointer { bufferPointer -> (JSValue) in
             let argv = bufferPointer.baseAddress
             let argc = bufferPointer.count
             var kindAndFlags = JavaScriptValueKindAndFlags()
             var payload1 = JavaScriptPayload1()
             var payload2 = JavaScriptPayload2()
             if let thisId = this?.id {
-                _call_function_with_this(thisId,
+                _call_function_with_this_no_catch(thisId,
                                          jsFunc.id, argv, Int32(argc),
                                          &kindAndFlags, &payload1, &payload2)
             } else {
-                _call_function(
+                _call_function_no_catch(
                     jsFunc.id, argv, Int32(argc),
                     &kindAndFlags, &payload1, &payload2
                 )
             }
+            assert(!kindAndFlags.isException)
             let result = RawJSValue(kind: kindAndFlags.kind, payload1: payload1, payload2: payload2)
-            return (result.jsValue(), kindAndFlags.isException)
+            return result.jsValue()
         }
     }
-    if isException {
-        throw result
-    }
-    return result
 }
diff --git a/Sources/JavaScriptKit/FundamentalObjects/JSThrowingFunction.swift b/Sources/JavaScriptKit/FundamentalObjects/JSThrowingFunction.swift
@@ -62,3 +62,31 @@ public class JSThrowingFunction {
         try new(arguments: arguments)
     }
 }
+
+private func invokeJSFunction(_ jsFunc: JSFunction, arguments: [ConvertibleToJSValue], this: JSObject?) throws -> JSValue {
+    let (result, isException) = arguments.withRawJSValues { rawValues in
+        rawValues.withUnsafeBufferPointer { bufferPointer -> (JSValue, Bool) in
+            let argv = bufferPointer.baseAddress
+            let argc = bufferPointer.count
+            var kindAndFlags = JavaScriptValueKindAndFlags()
+            var payload1 = JavaScriptPayload1()
+            var payload2 = JavaScriptPayload2()
+            if let thisId = this?.id {
+                _call_function_with_this(thisId,
+                                         jsFunc.id, argv, Int32(argc),
+                                         &kindAndFlags, &payload1, &payload2)
+            } else {
+                _call_function(
+                    jsFunc.id, argv, Int32(argc),
+                    &kindAndFlags, &payload1, &payload2
+                )
+            }
+            let result = RawJSValue(kind: kindAndFlags.kind, payload1: payload1, payload2: payload2)
+            return (result.jsValue(), kindAndFlags.isException)
+        }
+    }
+    if isException {
+        throw result
+    }
+    return result
+}
diff --git a/Sources/JavaScriptKit/XcodeSupport.swift b/Sources/JavaScriptKit/XcodeSupport.swift
@@ -53,6 +53,13 @@ import _CJavaScriptKit
         _: UnsafeMutablePointer<JavaScriptPayload1>!,
         _: UnsafeMutablePointer<JavaScriptPayload2>!
     ) { fatalError() }
+    func _call_function_no_catch(
+        _: JavaScriptObjectRef,
+        _: UnsafePointer<RawJSValue>!, _: Int32,
+        _: UnsafeMutablePointer<JavaScriptValueKindAndFlags>!,
+        _: UnsafeMutablePointer<JavaScriptPayload1>!,
+        _: UnsafeMutablePointer<JavaScriptPayload2>!
+    ) { fatalError() }
     func _call_function_with_this(
         _: JavaScriptObjectRef,
         _: JavaScriptObjectRef,
@@ -61,6 +68,14 @@ import _CJavaScriptKit
         _: UnsafeMutablePointer<JavaScriptPayload1>!,
         _: UnsafeMutablePointer<JavaScriptPayload2>!
     ) { fatalError() }
+    func _call_function_with_this_no_catch(
+        _: JavaScriptObjectRef,
+        _: JavaScriptObjectRef,
+        _: UnsafePointer<RawJSValue>!, _: Int32,
+        _: UnsafeMutablePointer<JavaScriptValueKindAndFlags>!,
+        _: UnsafeMutablePointer<JavaScriptPayload1>!,
+        _: UnsafeMutablePointer<JavaScriptPayload2>!
+    ) { fatalError() }
     func _call_new(
         _: JavaScriptObjectRef,
         _: UnsafePointer<RawJSValue>!, _: Int32
diff --git a/Sources/_CJavaScriptKit/_CJavaScriptKit.c b/Sources/_CJavaScriptKit/_CJavaScriptKit.c
@@ -36,7 +36,7 @@ void swjs_cleanup_host_function_call(void *argv_buffer) {
 /// this and `SwiftRuntime.version` in `./Runtime/src/index.ts`.
 __attribute__((export_name("swjs_library_version")))
 int swjs_library_version(void) {
-    return 705;
+    return 706;
 }
 
 int _library_features(void);
diff --git a/Sources/_CJavaScriptKit/include/_CJavaScriptKit.h b/Sources/_CJavaScriptKit/include/_CJavaScriptKit.h
@@ -170,6 +170,21 @@ extern void _call_function(const JavaScriptObjectRef ref, const RawJSValue *argv
                            JavaScriptPayload1 *result_payload1,
                            JavaScriptPayload2 *result_payload2);
 
+/// `_call_function` calls JavaScript function with given arguments list without capturing any exception
+///
+/// @param ref The target JavaScript function to call.
+/// @param argv A list of `RawJSValue` arguments to apply.
+/// @param argc The length of `argv``.
+/// @param result_kind A result pointer of JavaScript value kind of returned result or thrown exception.
+/// @param result_payload1 A result pointer of first payload of JavaScript value of returned result or thrown exception.
+/// @param result_payload2 A result pointer of second payload of JavaScript value of returned result or thrown exception.
+__attribute__((__import_module__("javascript_kit"),
+               __import_name__("swjs_call_function_no_catch")))
+extern void _call_function_no_catch(const JavaScriptObjectRef ref, const RawJSValue *argv,
+                           const int argc, JavaScriptValueKindAndFlags *result_kind,
+                           JavaScriptPayload1 *result_payload1,
+                           JavaScriptPayload2 *result_payload2);
+
 /// `_call_function_with_this` calls JavaScript function with given arguments list and given `_this`.
 ///
 /// @param _this The value of `this` provided for the call to `func_ref`.
@@ -188,6 +203,24 @@ extern void _call_function_with_this(const JavaScriptObjectRef _this,
                                      JavaScriptPayload1 *result_payload1,
                                      JavaScriptPayload2 *result_payload2);
 
+/// `_call_function_with_this` calls JavaScript function with given arguments list and given `_this` without capturing any exception.
+///
+/// @param _this The value of `this` provided for the call to `func_ref`.
+/// @param func_ref The target JavaScript function to call.
+/// @param argv A list of `RawJSValue` arguments to apply.
+/// @param argc The length of `argv``.
+/// @param result_kind A result pointer of JavaScript value kind of returned result or thrown exception.
+/// @param result_payload1 A result pointer of first payload of JavaScript value of returned result or thrown exception.
+/// @param result_payload2 A result pointer of second payload of JavaScript value of returned result or thrown exception.
+__attribute__((__import_module__("javascript_kit"),
+               __import_name__("swjs_call_function_with_this_no_catch")))
+extern void _call_function_with_this_no_catch(const JavaScriptObjectRef _this,
+                                     const JavaScriptObjectRef func_ref,
+                                     const RawJSValue *argv, const int argc,
+                                     JavaScriptValueKindAndFlags *result_kind,
+                                     JavaScriptPayload1 *result_payload1,
+                                     JavaScriptPayload2 *result_payload2);
+
 /// `_call_new` calls JavaScript object constructor with given arguments list.
 ///
 /// @param ref The target JavaScript constructor to call.

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ public class JSFunction: JSObject {`
`19`	`19`	`/// - Returns: The result of this call.`
`20`	`20`	`@discardableResult`
`21`	`21`	`public func callAsFunction(this: JSObject? = nil, arguments: [ConvertibleToJSValue]) -> JSValue {`
`22`		`- try! invokeJSFunction(self, arguments: arguments, this: this)`
	`22`	`+ invokeNonThrowingJSFunction(self, arguments: arguments, this: this)`
`23`	`23`	`}`
`24`	`24`
`25`	`25`	/// A variadic arguments version of `callAsFunction`.
`@@ -84,30 +84,27 @@ public class JSFunction: JSObject {`
`84`	`84`	`}`
`85`	`85`	`}`
`86`	`86`
`87`		`-internal func invokeJSFunction(_ jsFunc: JSFunction, arguments: [ConvertibleToJSValue], this: JSObject?) throws -> JSValue {`
`88`		`- let (result, isException) = arguments.withRawJSValues { rawValues in`
`89`		`- rawValues.withUnsafeBufferPointer { bufferPointer -> (JSValue, Bool) in`
	`87`	`+private func invokeNonThrowingJSFunction(_ jsFunc: JSFunction, arguments: [ConvertibleToJSValue], this: JSObject?) -> JSValue {`
	`88`	`+ arguments.withRawJSValues { rawValues in`
	`89`	`+ rawValues.withUnsafeBufferPointer { bufferPointer -> (JSValue) in`
`90`	`90`	`let argv = bufferPointer.baseAddress`
`91`	`91`	`let argc = bufferPointer.count`
`92`	`92`	`var kindAndFlags = JavaScriptValueKindAndFlags()`
`93`	`93`	`var payload1 = JavaScriptPayload1()`
`94`	`94`	`var payload2 = JavaScriptPayload2()`
`95`	`95`	`if let thisId = this?.id {`
`96`		`- _call_function_with_this(thisId,`
	`96`	`+ _call_function_with_this_no_catch(thisId,`
`97`	`97`	`jsFunc.id, argv, Int32(argc),`
`98`	`98`	`&kindAndFlags, &payload1, &payload2)`
`99`	`99`	`} else {`
`100`		`- _call_function(`
	`100`	`+ _call_function_no_catch(`
`101`	`101`	`jsFunc.id, argv, Int32(argc),`
`102`	`102`	`&kindAndFlags, &payload1, &payload2`
`103`	`103`	`)`
`104`	`104`	`}`
	`105`	`+ assert(!kindAndFlags.isException)`
`105`	`106`	`let result = RawJSValue(kind: kindAndFlags.kind, payload1: payload1, payload2: payload2)`
`106`		`- return (result.jsValue(), kindAndFlags.isException)`
	`107`	`+ return result.jsValue()`
`107`	`108`	`}`
`108`	`109`	`}`
`109`		`- if isException {`
`110`		`- throw result`
`111`		`- }`
`112`		`- return result`
`113`	`110`	`}`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ void swjs_cleanup_host_function_call(void *argv_buffer) {`
`36`	`36`	/// this and `SwiftRuntime.version` in `./Runtime/src/index.ts`.
`37`	`37`	`__attribute__((export_name("swjs_library_version")))`
`38`	`38`	`int swjs_library_version(void) {`
`39`		`- return 705;`
	`39`	`+ return 706;`
`40`	`40`	`}`
`41`	`41`
`42`	`42`	`int _library_features(void);`