Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/7.0-staging][wasm] runtime: Fix creating the stack trace for a ManagedError #89905

Closed

Conversation

radical
Copy link
Member

@radical radical commented Aug 3, 2023

Backport of a fix from #89890

cc @kg

Customer impact

With the latest stable version of chrome (115.*), some of the errors thrown for exceptions can be missing the original exception message, and the native stack. This fixes the issue.

Details

With the latest chrome (115.*) the following code in runtime/marshal.ts fails because this.superStack.value is no longer available:

    getSuperStack() {
        if (this.superStack) {
            return this.superStack.value;
        }
        return super.stack; // this works on FF
    }

This causes the final error to not have the original managed error message, and also have a "undefined" at the end of the string.

Truncated error missing the native part of the stack, and the message:

   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.ThrowFromJSExport(String message)
   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.__Wrapper_ThrowFromJSExport_271731536(JSMarshalerArgument* __arguments_buffer)
undefined

With the fix:

   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.ThrowFromJSExport(String message)
   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.__Wrapper_ThrowFromJSExport_817705034(JSMarshalerArgument* __arguments_buffer)
Error: -t-e-s-t-
    at sr (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:33284)
    at Br (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:42679)
    at http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:40825
    at Module.catch1stack (http://127.0.0.1:60345/JavaScriptTestHelper.mjs:132:9)
    at http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:36627
    at mr (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:37821)
    at do_icall (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[221]:0x19711)
    at do_icall_wrapper (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[108]:0x157bc)
    at mono_interp_exec_method (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[101]:0x9c92)
    at interp_runtime_invoke (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[141]:0x16cd7)

Thanks to @kg for the fix.

(cherry picked from commit 89f6429)

Testing

This has been tested with manual, and automated tests on main. FIXME: Testing pending on 7.0-staging

Risk

Low. This fixes a regression in behavior caused by a new chrome update.

With the latest chrome (`115.*`) the following code in
`runtime/marshal.ts` fails because `this.superStack.value` is no longer
available:

```js
    getSuperStack() {
        if (this.superStack) {
            return this.superStack.value;
        }
        return super.stack; // this works on FF
    }
```

This causes the final error to not have the original managed error
message, and also have a `"undefined"` at the end of the string.

Truncated error missing the native part of the stack, and the message:
```
   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.ThrowFromJSExport(String message)
   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.__Wrapper_ThrowFromJSExport_271731536(JSMarshalerArgument* __arguments_buffer)
undefined
```

With the fix:
```
   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.ThrowFromJSExport(String message)
   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.__Wrapper_ThrowFromJSExport_817705034(JSMarshalerArgument* __arguments_buffer)
Error: -t-e-s-t-
    at sr (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:33284)
    at Br (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:42679)
    at http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:40825
    at Module.catch1stack (http://127.0.0.1:60345/JavaScriptTestHelper.mjs:132:9)
    at http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:36627
    at mr (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:37821)
    at do_icall (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[221]:0x19711)
    at do_icall_wrapper (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[108]:0x157bc)
    at mono_interp_exec_method (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[101]:0x9c92)
    at interp_runtime_invoke (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[141]:0x16cd7)
```

Thanks to @kg for the fix.

(cherry picked from commit 89f6429)
@radical radical added the arch-wasm WebAssembly architecture label Aug 3, 2023
@radical radical requested a review from kg August 3, 2023 02:05
@ghost
Copy link

ghost commented Aug 3, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of a fix from #89890

cc @kg

Customer impact

With the latest stable version of chrome (115.*), some of the errors thrown for exceptions can be missing the original exception message, and the native stack. This fixes the issue.

Details

With the latest chrome (115.*) the following code in runtime/marshal.ts fails because this.superStack.value is no longer available:

    getSuperStack() {
        if (this.superStack) {
            return this.superStack.value;
        }
        return super.stack; // this works on FF
    }

This causes the final error to not have the original managed error message, and also have a "undefined" at the end of the string.

Truncated error missing the native part of the stack, and the message:

   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.ThrowFromJSExport(String message)
   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.__Wrapper_ThrowFromJSExport_271731536(JSMarshalerArgument* __arguments_buffer)
undefined

With the fix:

   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.ThrowFromJSExport(String message)
   at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.__Wrapper_ThrowFromJSExport_817705034(JSMarshalerArgument* __arguments_buffer)
Error: -t-e-s-t-
    at sr (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:33284)
    at Br (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:42679)
    at http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:40825
    at Module.catch1stack (http://127.0.0.1:60345/JavaScriptTestHelper.mjs:132:9)
    at http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:36627
    at mr (http://127.0.0.1:60345/_framework/dotnet.runtime.js:3:37821)
    at do_icall (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[221]:0x19711)
    at do_icall_wrapper (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[108]:0x157bc)
    at mono_interp_exec_method (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[101]:0x9c92)
    at interp_runtime_invoke (http://127.0.0.1:60345/_framework/dotnet.native.wasm:wasm-function[141]:0x16cd7)

Thanks to @kg for the fix.

(cherry picked from commit 89f6429)

Testing

This has been tested with manual, and automated tests on main. FIXME: Testing pending on 7.0-staging

Risk

Low. This fixes a regression in behavior caused by a new chrome update.

Author: radical
Assignees: -
Labels:

arch-wasm

Milestone: -

@radical radical changed the title [wasm] runtime: Fix creating the stack trace for a ManagedError [release/7.0-staging][wasm] runtime: Fix creating the stack trace for a ManagedError Aug 3, 2023
@radical radical marked this pull request as ready for review August 3, 2023 04:57
@radical radical added the Servicing-consider Issue for next servicing release review label Aug 3, 2023
@radical
Copy link
Member Author

radical commented Aug 3, 2023

@carlossanlop
Copy link
Member

Friendly reminder: if you want this servicing fix to be included in the September 2023 Release, you'll have to merge this PR before August 14th.

@radical
Copy link
Member Author

radical commented Aug 8, 2023

Friendly reminder: if you want this servicing fix to be included in the September 2023 Release, you'll have to merge this PR before August 14th.

Thanks. I want to test the broken vs fixed behavior specifically for 7.0 , which I should be able to do this week.

@carlossanlop
Copy link
Member

@radical since this PR has not yet been approved by Tactics, it will have to wait until the October Release.

@SamMonoRT SamMonoRT added this to the 7.0.x milestone Aug 22, 2023
@radical
Copy link
Member Author

radical commented Aug 24, 2023

This is not required as the exception message is correctly shown without this patch:

Screenshot 2023-08-24 at 01 22 48

@radical radical closed this Aug 24, 2023
@radical radical removed the Servicing-consider Issue for next servicing release review label Aug 24, 2023
@SamMonoRT SamMonoRT removed this from the 7.0.x milestone Aug 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants