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

Wasm integration doesn't work for uncaught WebAssembly.Exception #13787

Closed
3 tasks done
kurrak opened this issue Sep 25, 2024 · 5 comments
Closed
3 tasks done

Wasm integration doesn't work for uncaught WebAssembly.Exception #13787

kurrak opened this issue Sep 25, 2024 · 5 comments
Labels
Package: wasm Issues related to the Sentry Wasm SDK

Comments

@kurrak
Copy link

kurrak commented Sep 25, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/wasm

SDK Version

8.26.0

Framework Version

No response

Link to Sentry event

https://explain-everything.sentry.io/issues/5899639997/events/6735b6aa338e4a90b997c83410c1ef92/?project=4507549500047360

Reproduction Example/SDK Setup

I wasn't able to make minimal example showcasing the problem, as I was not able to find a way to use wasmIntegration using loader/CDN setup (In my app I'm integrating @sentry/wasm via npm). I'll happily update this if pointed to possible solution/workaround.

In order to run the example you need to install Emscripten SDK for compiling c++ code to wasm.

index.html:

<!DOCTYPE html>

<html>
<head>
  <meta charset="utf-8">
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>

<body>
  
  <script>
    window.sentryOnLoad = function () {
      Sentry.init({
        dsn: '',
        release: 'develop',
        attachStacktrace: true,
        // FOLLOWING LINE BREAKS THE EXAMPLE DUE TO MISSING WASM INTEGRATION IN LOADED SENTRY SCRIPT
        integrations: [wasmIntegration()],
        beforeSend(event, hint) {
          return event
        }
      });
    };
  </script>
  
  <script src="https://js.sentry-cdn.com/USE_YOUR_LINK.min.js" crossorigin="anonymous"></script>
  
  <!-- Add the javascript glue code (index.js) as generated by Emscripten -->
  <script src="index.js"></script>
  
</body>

</html>

main.cpp:

#include <stdexcept>

int main() {
  // abort(); <-- this would work just fine - Sentry's wasmIntegration would properly process event 
  throw std::runtime_error("exception from wasm/c++"); // this is not handled by Sentry's wasmIntegration
  return 0;
}

Steps to Reproduce

  1. Create index.html and main.cpp files as listed above in the same directory
  2. Compile c++ file using Emscripten: emcc -fwasm-exceptions -sEXCEPTION_STACK_TRACES=1 -Oz -g3 main.cpp -o index.js
  3. Run HTTP server (I'm using python3 -m http.server) and open index.html in the browser (http://localhost:8000 in my case).

Expected Result

Reported event should be recognised as Wasm event and should have stack trace showing stack trace when exception was thrown. This would allow proper symbolication and event grouping.

Actual Result

Reported event is recognised as regular JS event and has wrong stack trace. Stack trace has only JS frames for state completely not related to wasm/c++ exception.

Note that following shows results for my real app that integrates with sentry via npm

When using DevTools to see what event is provided to beforeSend hook, I see that event.exception.values has:

  • single entry with stacktrace not related to actual exception
  • proper (meaning: desired for reporting) stack trace is available in value.stack (please note that value is WebAssembly.Exception).

Image

Wasm integration is not processing such event because of exception's stacktrace not containing any wasm frame source.

Above scenario differs from wasm crash (not exception) that is properly processed by Sentry's wasm integration and thus properly seen from Sentry's UI. Following is example of event contents seen from beforeSend in crashing scenario (see comment in main.cpp to try yourself):

Image

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 25, 2024
@github-actions github-actions bot added the Package: wasm Issues related to the Sentry Wasm SDK label Sep 25, 2024
@trzeciak
Copy link
Contributor

Hey 👋
I make some investigation in this topic, and I made POC to solved this, see patches:

diff --git a/node_modules/@sentry/browser/build/npm/cjs/eventbuilder.js b/node_modules/@sentry/browser/build/npm/cjs/eventbuilder.js
index d6a2a81..d099c2e 100644
--- a/node_modules/@sentry/browser/build/npm/cjs/eventbuilder.js
+++ b/node_modules/@sentry/browser/build/npm/cjs/eventbuilder.js
@@ -10,7 +10,10 @@ function exceptionFromError(stackParser, ex) {
   // Get the frames first since Opera can lose the stack if we touch anything else first
   const frames = parseStackFrames(stackParser, ex);
 
-  const exception = {
+  const exception = ex instanceof WebAssembly.Exception ? {
+    type: ex.message[0],
+    value: ex.message[ex.message.length - 1],
+  } : {
     type: ex && ex.name,
     value: extractMessage(ex),
   };
diff --git a/node_modules/@sentry/browser/build/npm/esm/eventbuilder.js b/node_modules/@sentry/browser/build/npm/esm/eventbuilder.js
index dda7161..cbafc04 100644
--- a/node_modules/@sentry/browser/build/npm/esm/eventbuilder.js
+++ b/node_modules/@sentry/browser/build/npm/esm/eventbuilder.js
@@ -8,7 +8,10 @@ function exceptionFromError(stackParser, ex) {
   // Get the frames first since Opera can lose the stack if we touch anything else first
   const frames = parseStackFrames(stackParser, ex);
 
-  const exception = {
+  const exception = ex instanceof WebAssembly.Exception ? {
+    type: ex.message[0],
+    value: ex.message[ex.message.length - 1],
+  } : {
     type: ex && ex.name,
     value: extractMessage(ex),
   };
diff --git a/node_modules/@sentry/utils/build/cjs/is.js b/node_modules/@sentry/utils/build/cjs/is.js
index 956b1fb..33e8ddf 100644
--- a/node_modules/@sentry/utils/build/cjs/is.js
+++ b/node_modules/@sentry/utils/build/cjs/is.js
@@ -15,6 +15,7 @@ function isError(wat) {
     case '[object Error]':
     case '[object Exception]':
     case '[object DOMException]':
+    case '[object WebAssembly.Exception]':
       return true;
     default:
       return isInstanceOf(wat, Error);
diff --git a/node_modules/@sentry/utils/build/esm/is.js b/node_modules/@sentry/utils/build/esm/is.js
index 32b6f6a..c661ea2 100644
--- a/node_modules/@sentry/utils/build/esm/is.js
+++ b/node_modules/@sentry/utils/build/esm/is.js
@@ -13,6 +13,7 @@ function isError(wat) {
     case '[object Error]':
     case '[object Exception]':
     case '[object DOMException]':
+    case '[object WebAssembly.Exception]':
       return true;
     default:
       return isInstanceOf(wat, Error);

(partially generated by patch-package)

@Lms24
Copy link
Member

Lms24 commented Sep 26, 2024

Hi @kurrak thanks for reporting! I think you're on the right track with your suspicion. Also @trzeciak thanks for taking a look! Your solution looks very reasonable to me! Are you interested in submitting a PR for this? Happy to review :) Otherwise, no worries, just let me know and we'll pick it up.

Full disclosure: Our WASM integration hasn't seen much love in the last years so chances are very high that we don't cover a lot of cases.

@trzeciak
Copy link
Contributor

@Lms24 We talked about it locally and it turned out that some minor details need to be corrected, I'll try to prepare the PR over the weekend, if I don't find the time I'll let you know.

Hehe, we have already encountered a few other problems with wasm and sentry (you can see it on my github activity) xD. Despite the issues I have encountered, I have always enjoyed working with sentry! 💪

@trzeciak
Copy link
Contributor

trzeciak commented Oct 2, 2024

@Lms24 Okay, I prepared pull-request #13854. I don't know javascript (sometimes I confuse it with java xD), that's why I couldn't: run lint, test, or yarn at all.. sorry about that.
I tested it manually and it seems to work.

@getsantry getsantry bot moved this from Waiting for: Community to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 2, 2024
@getsantry getsantry bot removed the status in GitHub Issues with 👀 3 Oct 3, 2024
lforst added a commit that referenced this issue Oct 4, 2024
…13854)

Add support for wasm WebAssembly.Exception (uncaught exception in
emscripten).

## References
- [WebAssembly.Exception | MDN Web
Docs](https://developer.mozilla.org/en-US/docs/WebAssembly/JavaScript_interface/Exception)
- #13787

---------

Co-authored-by: s1gr1d <[email protected]>
Co-authored-by: Sigrid Huemer <[email protected]>
Co-authored-by: Luca Forstner <[email protected]>
@kurrak
Copy link
Author

kurrak commented Oct 4, 2024

@trzeciak @Lms24 thanks much, I'm closing this now 👍

@kurrak kurrak closed this as completed Oct 4, 2024
alexandresoro pushed a commit to alexandresoro/ouca that referenced this issue Oct 10, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@sentry/node](https://github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.33.1` -> `8.34.0`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.33.1/8.34.0) |
| [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.33.1` -> `8.34.0`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.33.1/8.34.0) |

---

### Release Notes

<details>
<summary>getsentry/sentry-javascript (@&#8203;sentry/node)</summary>

### [`v8.34.0`](https://github.com/getsentry/sentry-javascript/releases/tag/8.34.0)

[Compare Source](getsentry/sentry-javascript@8.33.1...8.34.0)

##### Important Changes

-   **ref(nextjs): Remove dead code ([#&#8203;13828](getsentry/sentry-javascript#13903

Relevant for users of the `@sentry/nextjs` package: If you have previously configured a
`SENTRY_IGNORE_API_RESOLUTION_ERROR` environment variable, it is now safe to unset it.

##### Other Changes

-   feat(cdn): Export `getReplay` in replay CDN bundles
    ([#&#8203;13881](getsentry/sentry-javascript#13881))
-   feat(replay): Clear fallback buffer when switching buffers
    ([#&#8203;13914](getsentry/sentry-javascript#13914))
-   feat(replay): Upgrade rrweb packages to 2.28.0 ([#&#8203;13732](getsentry/sentry-javascript#13732))
-   fix(docs): Correct supported browsers due to `globalThis`
    ([#&#8203;13788](getsentry/sentry-javascript#13788))
-   fix(nextjs): Adjust path to `requestAsyncStorageShim.js` template file
    ([#&#8203;13928](getsentry/sentry-javascript#13928))
-   fix(nextjs): Detect new locations for request async storage to support Next.js v15.0.0-canary.180 and higher
    ([#&#8203;13920](getsentry/sentry-javascript#13920))
-   fix(nextjs): Drop `_not-found` spans for all HTTP methods
    ([#&#8203;13906](getsentry/sentry-javascript#13906))
-   fix(nextjs): Fix resolution of request storage shim fallback
    ([#&#8203;13929](getsentry/sentry-javascript#13929))
-   fix(node): Ensure graphql options are correct when preloading
    ([#&#8203;13769](getsentry/sentry-javascript#13769))
-   fix(node): Local variables handle error ([#&#8203;13827](getsentry/sentry-javascript#13827))
-   fix(node): Remove `dataloader` instrumentation from default integrations
    ([#&#8203;13873](getsentry/sentry-javascript#13873))
-   fix(nuxt): Create declaration files for Nuxt module
    ([#&#8203;13909](getsentry/sentry-javascript#13909))
-   fix(replay): Ensure `replay_id` is removed from frozen DSC when stopped
    ([#&#8203;13893](getsentry/sentry-javascript#13893))
-   fix(replay): Try/catch `sendBufferedReplayOrFlush` to prevent cycles
    ([#&#8203;13900](getsentry/sentry-javascript#13900))
-   fix(sveltekit): Ensure trace meta tags are always injected
    ([#&#8203;13231](getsentry/sentry-javascript#13231))
-   fix(sveltekit): Update `wrapServerRouteWithSentry` to respect ParamMatchers
    ([#&#8203;13390](getsentry/sentry-javascript#13390))
-   fix(wasm): Integration wasm uncaught WebAssembly.Exception
    ([#&#8203;13787](getsentry/sentry-javascript#13787)) ([#&#8203;13854](getsentry/sentry-javascript#13854))
-   ref(nextjs): Ignore sentry spans based on query param attribute
    ([#&#8203;13905](getsentry/sentry-javascript#13905))
-   ref(utils): Move `vercelWaitUntil` to utils ([#&#8203;13891](getsentry/sentry-javascript#13891))

Work in this release was contributed by [@&#8203;trzeciak](https://github.com/trzeciak), [@&#8203;gurpreetatwal](https://github.com/gurpreetatwal), [@&#8203;ykzts](https://github.com/ykzts) and [@&#8203;lizhiyao](https://github.com/lizhiyao). Thank you for your
contributions!

##### Bundle size 📦

| Path                                                             | Size              |
| ---------------------------------------------------------------- | ----------------- |
| [@&#8203;sentry/browser](https://github.com/sentry/browser)                                                  | 22.73 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) - with treeshaking flags                         | 21.53 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing)                                  | 34.97 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay)                          | 71.62 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay) - with treeshaking flags | 62.03 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay with Canvas)              | 75.97 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback)                | 88.73 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback, metrics)       | 90.59 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. metrics)                                  | 27 KB     |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Feedback)                                 | 39.87 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. sendFeedback)                             | 27.38 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. FeedbackAsync)                            | 32.17 KB  |
| [@&#8203;sentry/react](https://github.com/sentry/react)                                                    | 25.49 KB  |
| [@&#8203;sentry/react](https://github.com/sentry/react) (incl. Tracing)                                    | 37.94 KB  |
| [@&#8203;sentry/vue](https://github.com/sentry/vue)                                                      | 26.91 KB  |
| [@&#8203;sentry/vue](https://github.com/sentry/vue) (incl. Tracing)                                      | 36.86 KB  |
| [@&#8203;sentry/svelte](https://github.com/sentry/svelte)                                                   | 22.87 KB  |
| CDN Bundle                                                       | 24.05 KB  |
| CDN Bundle (incl. Tracing)                                       | 36.76 KB  |
| CDN Bundle (incl. Tracing, Replay)                               | 71.38 KB  |
| CDN Bundle (incl. Tracing, Replay, Feedback)                     | 76.7 KB   |
| CDN Bundle - uncompressed                                        | 70.53 KB  |
| CDN Bundle (incl. Tracing) - uncompressed                        | 109.04 KB |
| CDN Bundle (incl. Tracing, Replay) - uncompressed                | 221.4 KB  |
| CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed      | 234.62 KB |
| [@&#8203;sentry/nextjs](https://github.com/sentry/nextjs) (client)                                          | 37.91 KB  |
| [@&#8203;sentry/sveltekit](https://github.com/sentry/sveltekit) (client)                                       | 35.56 KB  |
| [@&#8203;sentry/node](https://github.com/sentry/node)                                                     | 124.5 KB  |
| [@&#8203;sentry/node](https://github.com/sentry/node) - without tracing                                   | 93.64 KB  |
| [@&#8203;sentry/aws-serverless](https://github.com/sentry/aws-serverless)                                           | 103.3 KB  |

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4xMTYuMCIsInVwZGF0ZWRJblZlciI6IjM4LjExNi4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=-->

Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/212
Reviewed-by: Alexandre Soro <[email protected]>
Co-authored-by: renovate <[email protected]>
Co-committed-by: renovate <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: wasm Issues related to the Sentry Wasm SDK
Projects
Archived in project
Development

No branches or pull requests

4 participants