fix: Validate if WebGLRenderingContext exists before capturing it#1777
fix: Validate if WebGLRenderingContext exists before capturing it#1777Juice10 merged 10 commits intorrweb-io:masterfrom
Conversation
Remove handlers.push because if WebGL2RenderingContext is undefined will fail
🦋 Changeset detectedLatest commit: f1c6ba0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| ): listenerHandler { | ||
| const handlers: listenerHandler[] = []; | ||
|
|
||
| handlers.push( |
There was a problem hiding this comment.
maybe i'm being silly, but this block isn't a straight duplicate of the block in the conditional below
could you share the errors you were seeing?
There was a problem hiding this comment.
I received: Undefined is not a object in WebGLRenderingContext.prototype
There was a problem hiding this comment.
so i suppose the problem here is we're treating all the different RenderingContexts as definitely available when they're not
so each of these handler pushes should be validating presence?
surprising it wasn't there... do you know what browser this was or what stacktrace the error had
There was a problem hiding this comment.
sure!, this happen in iOS (webview and safari) the stacktrace mentions Undefined is not a object in WebGLRenderingContext.prototype
There was a problem hiding this comment.
So this isn't a duplicate of the code below (although hard to see because there is only one char difference).
This has to do with WebGL One (aka WebGL without a number), the code below with WebGL Two (aka WebGL2). Some older browser didn't support webgl2 so I added an explicit check for it. But I assumed all browsers rrweb supports, also support WebGL 1. So I didn't add a check for it. We could do exactly as the if statement does below where we check if its available before we try to patch it, that's no problem
There was a problem hiding this comment.
it's not showing updated code for me 🙈
There was a problem hiding this comment.
ah, no... i see
you've put it in the wrong block
Co-authored-by: Paul D'Ambra <paul.dambra@gmail.com>
pauldambra
left a comment
There was a problem hiding this comment.
that looks ok to me now
i gues it needs a changeset
i don't have edit rights but the PR title / description need updating too :)
Juice10
left a comment
There was a problem hiding this comment.
Very close, als Paul says alls we have to do is fix formatting and update title/description of the PR and add a changeset
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
## Summary Adopts upstream rrweb PR rrweb-io/rrweb#1777 Some browsers (notably iOS Safari/WebView) don't support WebGL 1, causing a crash when accessing `WebGLRenderingContext.prototype` unconditionally. This adds the same `typeof` guard already used for `WebGL2RenderingContext`. ## Changes - `packages/rrweb/src/record/observers/canvas/webgl.ts` — wrap `WebGLRenderingContext.prototype` access in `typeof` check ## Test plan - [ ] CI passes - [ ] No regression in WebGL canvas recording tests Related issue: #147
Remove handlers.push because if WebGL2RenderingContext is undefined will fail is duplicated logic with post validationEdit: Validate if WebGLRenderingContext exists before trying to capture it