From 7e851dace81b3290cc84a9c2b79d2d93ca0d0bbb Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 12 Oct 2023 14:20:24 -0400 Subject: [PATCH 1/4] fix(replay): Fix potential broken CSS in styled-components NOTE: This requires a bump to rrweb library Fixes an issue where the Replay integration can potentially break applications that use `styled-components`. `styled-components` [relies on an exception being throw](https://github.com/styled-components/styled-components/blob/b7b374bb1ceff1699f7035b15881bc807110199a/packages/styled-components/src/sheet/Tag.ts#L32-L40) for CSS rules that are not supported by the browser engine. However, our SDK suppresses any exceptions thrown from within rrweb, so `styled-components` assumes that an unsupported rule was inserted successfully and increases a rule index, which causes following inserted rules to fail due to an out-of-bounds error. https://github.com/getsentry/rrweb/pull/111 introduces a change the adds metadata to exceptions that occur when calling `insertRule`, and this PR will re-throw those exceptions that will then bubble up to `styled-components`. Fixes https://github.com/getsentry/sentry-javascript/issues/9170 --- packages/replay/src/integration.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index 753fd62d5660..6cecc105d6eb 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -145,10 +145,14 @@ export class Replay implements Integration { // collect fonts, but be aware that `sentry.io` needs to be an allowed // origin for playback collectFonts: true, - errorHandler: (err: Error) => { + errorHandler: (err: Error & {__rrweb__?: boolean, __source__?: string}) => { try { - // @ts-expect-error Set this so that replay SDK can ignore errors originating from rrweb err.__rrweb__ = true; + + // Re-throw as styled-components relies on thrown exception when CSS rule fails to be inserted. + if (err.__source__ === 'CSSStyleSheet.insertRule') { + throw err; + } } catch { // avoid any potential hazards here } From b7444943ba6cdc849f8a0b0a68c95c7d9f2803fa Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 12 Oct 2023 15:00:49 -0400 Subject: [PATCH 2/4] errorHandler should only annotate error, we want it to re-throw so do not return true --- .../suites/replay/exceptions/template.html | 8 ++++ .../suites/replay/exceptions/test.ts | 43 +++++++++++++++++++ packages/replay/src/integration.ts | 15 +------ 3 files changed, 53 insertions(+), 13 deletions(-) create mode 100644 packages/browser-integration-tests/suites/replay/exceptions/template.html create mode 100644 packages/browser-integration-tests/suites/replay/exceptions/test.ts diff --git a/packages/browser-integration-tests/suites/replay/exceptions/template.html b/packages/browser-integration-tests/suites/replay/exceptions/template.html new file mode 100644 index 000000000000..0915a77b0cd9 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/exceptions/template.html @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/packages/browser-integration-tests/suites/replay/exceptions/test.ts b/packages/browser-integration-tests/suites/replay/exceptions/test.ts new file mode 100644 index 000000000000..28480dd4cdc3 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/exceptions/test.ts @@ -0,0 +1,43 @@ +import { expect } from '@playwright/test'; +import { SDK_VERSION } from '@sentry/browser'; + +import { sentryTest } from '../../../utils/fixtures'; +import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers'; + +sentryTest('exceptions within rrweb and re-thrown and annotated', async ({ getLocalTestPath, page, browserName}) => { + if (shouldSkipReplayTest() || browserName !== 'chromium') { + sentryTest.skip(); + } + + // const reqPromise0 = waitForReplayRequest(page, 0); + // const reqPromise1 = waitForReplayRequest(page, 1); + // + // await page.route('https://dsn.ingest.sentry.io/**/*', route => { + // return route.fulfill({ + // status: 200, + // contentType: 'application/json', + // body: JSON.stringify({ id: 'test-id' }), + // }); + // }); + // + const url = await getLocalTestPath({ testDir: __dirname }); + + await page.goto(url); + +expect(await page.evaluate(() => { + try { + const s = new CSSStyleSheet(); + s.insertRule('body::-ms-expand{display: none}'); + s.insertRule('body {background-color: #fff;}'); + return s.cssRules.length; + } catch { + return false; + } + })).toBe(false); + +expect(await page.evaluate(() => { + const s = new CSSStyleSheet(); + s.insertRule('body {background-color: #fff;}'); + return s.cssRules.length; + })).toBe(1); +}) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index 6cecc105d6eb..d208fcde61e5 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -145,19 +145,8 @@ export class Replay implements Integration { // collect fonts, but be aware that `sentry.io` needs to be an allowed // origin for playback collectFonts: true, - errorHandler: (err: Error & {__rrweb__?: boolean, __source__?: string}) => { - try { - err.__rrweb__ = true; - - // Re-throw as styled-components relies on thrown exception when CSS rule fails to be inserted. - if (err.__source__ === 'CSSStyleSheet.insertRule') { - throw err; - } - } catch { - // avoid any potential hazards here - } - // return true to suppress throwing the error inside of rrweb - return true; + errorHandler: (err: Error & {__rrweb__?: boolean}) => { + err.__rrweb__ = true; }, }; From 0f8dd6a27f3b8c48e3e3f313182c3018282553ff Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 12 Oct 2023 16:18:08 -0400 Subject: [PATCH 3/4] prettier --- .../suites/replay/exceptions/test.ts | 52 ++++++++----------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/exceptions/test.ts b/packages/browser-integration-tests/suites/replay/exceptions/test.ts index 28480dd4cdc3..203550d55759 100644 --- a/packages/browser-integration-tests/suites/replay/exceptions/test.ts +++ b/packages/browser-integration-tests/suites/replay/exceptions/test.ts @@ -1,43 +1,35 @@ import { expect } from '@playwright/test'; -import { SDK_VERSION } from '@sentry/browser'; import { sentryTest } from '../../../utils/fixtures'; -import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers'; +import { shouldSkipReplayTest } from '../../../utils/replayHelpers'; -sentryTest('exceptions within rrweb and re-thrown and annotated', async ({ getLocalTestPath, page, browserName}) => { +sentryTest('exceptions within rrweb and re-thrown and annotated', async ({ getLocalTestPath, page, browserName }) => { if (shouldSkipReplayTest() || browserName !== 'chromium') { sentryTest.skip(); } - // const reqPromise0 = waitForReplayRequest(page, 0); - // const reqPromise1 = waitForReplayRequest(page, 1); - // - // await page.route('https://dsn.ingest.sentry.io/**/*', route => { - // return route.fulfill({ - // status: 200, - // contentType: 'application/json', - // body: JSON.stringify({ id: 'test-id' }), - // }); - // }); - // const url = await getLocalTestPath({ testDir: __dirname }); await page.goto(url); -expect(await page.evaluate(() => { - try { - const s = new CSSStyleSheet(); - s.insertRule('body::-ms-expand{display: none}'); - s.insertRule('body {background-color: #fff;}'); - return s.cssRules.length; - } catch { - return false; - } - })).toBe(false); + expect( + await page.evaluate(() => { + try { + const s = new CSSStyleSheet(); + s.insertRule('body::-ms-expand{display: none}'); + s.insertRule('body {background-color: #fff;}'); + return s.cssRules.length; + } catch { + return false; + } + }), + ).toBe(false); -expect(await page.evaluate(() => { - const s = new CSSStyleSheet(); - s.insertRule('body {background-color: #fff;}'); - return s.cssRules.length; - })).toBe(1); -}) + expect( + await page.evaluate(() => { + const s = new CSSStyleSheet(); + s.insertRule('body {background-color: #fff;}'); + return s.cssRules.length; + }), + ).toBe(1); +}); From a66168ffec2839d4725cfb28e8a931f47ed5878c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 13 Oct 2023 10:15:51 +0200 Subject: [PATCH 4/4] try-catch writing error --- packages/replay/src/integration.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index d208fcde61e5..725a61681d1c 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -145,8 +145,13 @@ export class Replay implements Integration { // collect fonts, but be aware that `sentry.io` needs to be an allowed // origin for playback collectFonts: true, - errorHandler: (err: Error & {__rrweb__?: boolean}) => { - err.__rrweb__ = true; + errorHandler: (err: Error & { __rrweb__?: boolean }) => { + try { + err.__rrweb__ = true; + } catch (error) { + // ignore errors here + // this can happen if the error is frozen or does not allow mutation for other reasons + } }, };