Skip to content

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

Closed
radical wants to merge 2 commits into
dotnet:release/7.0-stagingfrom
radical:release/7.0-staging
Closed

[release/7.0-staging][wasm] runtime: Fix creating the stack trace for a ManagedError#89905
radical wants to merge 2 commits into
dotnet:release/7.0-stagingfrom
radical:release/7.0-staging

Conversation

@radical

Copy link
Copy Markdown
Member

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){returnthis.superStack.value;}returnsuper.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)
@radicalradical added the arch-wasm WebAssembly architecture label Aug 3, 2023
@radical
radical requested a review from kgAugust 3, 2023 02:05
@ghost

ghost commented Aug 3, 2023

Copy link
Copy Markdown

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){returnthis.superStack.value;}returnsuper.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:-

@radicalradical 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 ManagedErrorAug 3, 2023
kg
kg approved these changes Aug 3, 2023
@radical
radical marked this pull request as ready for review August 3, 2023 04:57
@radicalradical added the Servicing-consider Issue for next servicing release review label Aug 3, 2023
@radical

Copy link
Copy Markdown
MemberAuthor

@carlossanlop

Copy link
Copy Markdown
Contributor

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
Copy Markdown
MemberAuthor

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
Copy Markdown
Contributor

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

@SamMonoRTSamMonoRT added this to the 7.0.x milestone Aug 22, 2023
@radical

Copy link
Copy Markdown
MemberAuthor

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

Screenshot 2023-08-24 at 01 22 48

@radicalradical closed this Aug 24, 2023
@radicalradical removed the Servicing-consider Issue for next servicing release review label Aug 24, 2023
@SamMonoRTSamMonoRT removed this from the 7.0.x milestone Aug 24, 2023
@ghostghost locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for freeto 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

@radical@carlossanlop@kg@SamMonoRT