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

fix: Handle exceptions thrown from postcss when calling adaptCssForReplay #1580

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guntherjh
Copy link

This PR adds a try/catch around postcss().process() so that exceptions won't cause playback to crash. I was encountering issues where invalid CSS was being passed to postcss which would result in CssSyntaxError. I am still investigating why invalid CSS is passed to adaptCssForReplay in the first place, but for now this change should at least make sure any exceptions from postcss are at least handled and logged. I am currently using warn to log the exceptions but am happy to switch that to something else if need be.

Related discussion in rrweb slack: https://rrweb.slack.com/archives/C01BYDC5C93/p1727975312250899

Copy link

changeset-bot bot commented Oct 8, 2024

🦋 Changeset detected

Latest commit: 27f01fe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 19 packages
Name Type
rrweb-snapshot Patch
rrweb Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/all Patch
@rrweb/replay Patch
@rrweb/record Patch
@rrweb/types Patch
@rrweb/packer Patch
@rrweb/utils Patch
@rrweb/web-extension Patch
rrvideo Patch
@rrweb/rrweb-plugin-console-record Patch
@rrweb/rrweb-plugin-console-replay Patch
@rrweb/rrweb-plugin-sequential-id-record Patch
@rrweb/rrweb-plugin-sequential-id-replay Patch
@rrweb/rrweb-plugin-canvas-webrtc-record Patch
@rrweb/rrweb-plugin-canvas-webrtc-replay Patch

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

@otan
Copy link

otan commented Oct 10, 2024

heads up, i think the root cause is #1575

@guntherjh
Copy link
Author

@otan Good call out! That definitely seems like one of the ways invalid CSS can be passed to postcss. I'm also looking at another potential cause and will report back here if I find anything useful (or if it turns out to be the same problem as the one described in the issue you linked).

For this particular PR, I just wanted to simply make sure exceptions from postcss were handled so it wouldn't cause playback to crash.

Vadman97 added a commit to highlight/highlight that referenced this pull request Oct 10, 2024
## Summary

Bring back postcss css parsing which offers better reliability with
inlined stylesheets and better performance.
Fixes replay of a session where the existing CSS parsing of rrweb was
broken.
Incorporates rrweb-io/rrweb#1580 to fix broken
replays due to postcss crash.

## How did you test this change?


https://preview.highlight.io/102751/sessions/35zPFX0OPXs166n5L7m1GEZaqWgL?%7Er_preview=%7B%22action%22%3A%22start%22%2C%22mode%22%3A%22production%22%2C%22region%22%3A%22ewr%22%2C%22variantId%22%3A%2201J8GFGDTWBWCRZZZ7G92P9HJX%22%2C%22variantData%22%3A%22%7B%5C%22type%5C%22%3A%5C%22branch%5C%22%2C%5C%22branch%5C%22%3A%5C%22vadim%2Fsession-troubleshooting%5C%22%2C%5C%22githubOwnerName%5C%22%3A%5C%22highlight%5C%22%2C%5C%22githubRepositoryName%5C%22%3A%5C%22highlight%5C%22%7D%22%7D

fixes replay that is otherwise broken 


https://preview.highlight.io/14083/sessions/77X2eTJA5TIbEydiUytAFmZhJoXn?commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&page=1&query=viewed_by_anyone%3Dtrue+visited-url%3D%22*6532da*%22&commentId=90302&commentId=90302&%7Er_preview=%7B%22action%22%3A%22start%22%2C%22mode%22%3A%22production%22%2C%22region%22%3A%22ewr%22%2C%22variantId%22%3A%2201J8GFGDTWBWCRZZZ7G92P9HJX%22%2C%22variantData%22%3A%22%7B%5C%22type%5C%22%3A%5C%22branch%5C%22%2C%5C%22branch%5C%22%3A%5C%22vadim%2Fsession-troubleshooting%5C%22%2C%5C%22githubOwnerName%5C%22%3A%5C%22highlight%5C%22%2C%5C%22githubRepositoryName%5C%22%3A%5C%22highlight%5C%22%7D%22%7D

## Are there any deployment considerations?

frontend only

continues to work
![Screenshot from 2024-10-09
17-36-41](https://github.com/user-attachments/assets/f4e6b68a-e993-4292-9d76-0049d3e7b797)


## Does this work require review from our design team?

no
@guntherjh guntherjh force-pushed the jg-app-handle-postcss-exceptions branch from a08c325 to ae97dac Compare October 14, 2024 14:14
guntherjh added a commit to pendo-io/rrweb that referenced this pull request Oct 21, 2024
- Apply formatting changes
- Remove import of postcss in test file

Upstream PR: rrweb-io#1580
@guntherjh guntherjh force-pushed the jg-app-handle-postcss-exceptions branch from ae97dac to c1accef Compare October 21, 2024 17:53
- Apply formatting changes
- Remove import of postcss in test file
@guntherjh guntherjh force-pushed the jg-app-handle-postcss-exceptions branch from c1accef to 27f01fe Compare October 21, 2024 17:54
giraffekey pushed a commit to giraffekey/highlight that referenced this pull request Nov 21, 2024
## Summary

Bring back postcss css parsing which offers better reliability with
inlined stylesheets and better performance.
Fixes replay of a session where the existing CSS parsing of rrweb was
broken.
Incorporates rrweb-io/rrweb#1580 to fix broken
replays due to postcss crash.

## How did you test this change?


https://preview.highlight.io/102751/sessions/35zPFX0OPXs166n5L7m1GEZaqWgL?%7Er_preview=%7B%22action%22%3A%22start%22%2C%22mode%22%3A%22production%22%2C%22region%22%3A%22ewr%22%2C%22variantId%22%3A%2201J8GFGDTWBWCRZZZ7G92P9HJX%22%2C%22variantData%22%3A%22%7B%5C%22type%5C%22%3A%5C%22branch%5C%22%2C%5C%22branch%5C%22%3A%5C%22vadim%2Fsession-troubleshooting%5C%22%2C%5C%22githubOwnerName%5C%22%3A%5C%22highlight%5C%22%2C%5C%22githubRepositoryName%5C%22%3A%5C%22highlight%5C%22%7D%22%7D

fixes replay that is otherwise broken 


https://preview.highlight.io/14083/sessions/77X2eTJA5TIbEydiUytAFmZhJoXn?commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&page=1&query=viewed_by_anyone%3Dtrue+visited-url%3D%22*6532da*%22&commentId=90302&commentId=90302&%7Er_preview=%7B%22action%22%3A%22start%22%2C%22mode%22%3A%22production%22%2C%22region%22%3A%22ewr%22%2C%22variantId%22%3A%2201J8GFGDTWBWCRZZZ7G92P9HJX%22%2C%22variantData%22%3A%22%7B%5C%22type%5C%22%3A%5C%22branch%5C%22%2C%5C%22branch%5C%22%3A%5C%22vadim%2Fsession-troubleshooting%5C%22%2C%5C%22githubOwnerName%5C%22%3A%5C%22highlight%5C%22%2C%5C%22githubRepositoryName%5C%22%3A%5C%22highlight%5C%22%7D%22%7D

## Are there any deployment considerations?

frontend only

continues to work
![Screenshot from 2024-10-09
17-36-41](https://github.com/user-attachments/assets/f4e6b68a-e993-4292-9d76-0049d3e7b797)


## Does this work require review from our design team?

no
@eoghanmurray
Copy link
Contributor

#1600 should fix that those types of fragments get passed into postcss in the first place. Happy to add this try/except but hopefully it shouldn't be encountered after #1600 (assuming the underlying css is fine)

@guntherjh
Copy link
Author

@eoghanmurray thanks for taking a look at my PR! I'll need to double check the original issue I was encountering to see if #1600 fixes it (I thought I checked and it didn't but I honestly can't remember 😓). In any case, I am inclined to keep the try/catch just to prevent any future exceptions from postcss from going unhandled. I'm happy to make any updates to how the exceptions are logged (or any other issue you see). Thanks again for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants