feat(wasm): initialised sentryWasmImages for webworkers #18812
Conversation
|
Thanks for the contribution @harshit078. Are you done working on this or are you done (asking since it's a draft). |
|
Hey @andreiborza , yes I have finished working on this PR and converted it to review. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| expect(frames[0]?.platform).toBe('native'); | ||
| expect(frames[0]?.addr_mode).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing integration or E2E tests for feat PR
Low Severity · Bugbot Rules
This feat PR adds new functionality for initializing sentryWasmImages for web workers but only includes unit tests. Per the review rules file, feat PRs require at least one integration or E2E test. While the unit tests cover the core functionality (registerWebWorkerWasm, patchFrames with worker images, WASM image message processing), there's no E2E test that validates the complete flow of WASM modules loaded in workers being symbolicated correctly in captured events. The existing browser-webworker-vite E2E test application doesn't cover WASM functionality.
Additional Locations (1)
| (WINDOW as typeof WINDOW & { _sentryWasmImages?: Array<DebugImage> })._sentryWasmImages || []; | ||
| const newImages = event.data._sentryWasmImages.filter( | ||
| (newImg: DebugImage) => !existingImages.some(existing => existing.code_file === newImg.code_file), | ||
| ); |
There was a problem hiding this comment.
Missing array element validation can cause TypeError crash
Low Severity
The _sentryWasmImages validation checks only Array.isArray() but not the array elements. The processing code then accesses newImg.code_file on each element. If the array contains null or undefined (e.g., [null]), this throws a TypeError: Cannot read property 'code_file' of null. Other message fields like _sentryWorkerError use isPlainObject() validation which rejects null, but the array element validation is missing this safeguard.
Additional Locations (1)
| */ | ||
| export function registerWebWorkerWasm({ self }: RegisterWebWorkerWasmOptions): void { | ||
| patchWebAssemblyWithForwarding(self); | ||
| } |
There was a problem hiding this comment.
Missing integration or E2E test for feature
Low Severity · Bugbot Rules
Per the review rules, feat PRs should include at least one integration or E2E test. This PR adds the registerWebWorkerWasm function and related WASM image forwarding functionality but only includes unit tests. The unit tests verify individual pieces (patching, frame processing) but don't test the complete flow of a WASM module being instantiated in a worker, the debug image being forwarded to the main thread, and then being used correctly during error symbolication.
Additional Locations (1)
|
working on comments addressed by cursor ! |
2 participants
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes #18779