diff --git a/.changeset/quiet-shadow-connectivity.md b/.changeset/quiet-shadow-connectivity.md new file mode 100644 index 0000000000..795edf98a4 --- /dev/null +++ b/.changeset/quiet-shadow-connectivity.md @@ -0,0 +1,5 @@ +--- +'posthog-js': patch +--- + +Reduce shadow DOM connectivity checks during session recording. diff --git a/packages/browser/playwright/mocked/session-recording/session-recording-connectivity.spec.ts b/packages/browser/playwright/mocked/session-recording/session-recording-connectivity.spec.ts new file mode 100644 index 0000000000..042f802bfc --- /dev/null +++ b/packages/browser/playwright/mocked/session-recording/session-recording-connectivity.spec.ts @@ -0,0 +1,58 @@ +import { test, expect } from '../utils/posthog-playwright-test-base' +import { start, waitForSessionRecordingToStart } from '../utils/setup' + +for (const patchTiming of ['none', 'before-start', 'after-start'] as const) { + test(`records connected additions with connectivity getter patching: ${patchTiming}`, async ({ page, context }) => { + if (patchTiming === 'before-start') { + await page.addInitScript(() => { + // Leave the clean same-origin iframe realm used by rrweb untouched. + if (window !== window.top) return + Object.defineProperty(Node.prototype, 'isConnected', { + configurable: true, + get: () => false, + }) + }) + } + await start( + { + options: { session_recording: { compress_events: false } }, + flagsResponseOverrides: { + sessionRecording: { endpoint: '/ses/', masking: { maskAllInputs: true } }, + capturePerformance: false, + }, + url: './playground/cypress/index.html', + }, + page, + context + ) + await waitForSessionRecordingToStart(page) + const observedConnected = await page.evaluate((patchTiming) => { + if (patchTiming === 'after-start') { + Object.defineProperty(Node.prototype, 'isConnected', { + configurable: true, + get: () => false, + }) + } + // Avoid Playwright's own isConnected checks while the page prototype is patched. + ;(document.querySelector('[data-cy-input]') as HTMLInputElement).click() + const root = document.createElement('div') + root.attachShadow({ mode: 'open' }).textContent = 'CONNECTED_INCREMENTAL_MARKER' + document.body.append(root) + return root.isConnected + }, patchTiming) + expect(observedConnected).toBe(patchTiming === 'none') + await expect + .poll(async () => + (await page.capturedEvents()) + .filter((event) => event.event === '$snapshot') + .flatMap((event) => event.properties['$snapshot_data']) + .some( + (event) => + event.type === 3 && + event.data.source === 0 && + event.data.adds.some((add) => add.node.textContent === 'CONNECTED_INCREMENTAL_MARKER') + ) + ) + .toBe(true) + }) +} diff --git a/packages/browser/scripts/benchmark-replay-connectivity.md b/packages/browser/scripts/benchmark-replay-connectivity.md new file mode 100644 index 0000000000..5787536570 --- /dev/null +++ b/packages/browser/scripts/benchmark-replay-connectivity.md @@ -0,0 +1,68 @@ +# Shadow connectivity investigation after #4812 + +Base: `f45416053617f752ca05de0dec7439e369e51228`, including all five previous synchronous improvements. Worktree: `posthog-js-4217-connectivity`, branch `perf/replay-dom-connectivity`. This is separate from privacy PR #4815 and does not include its code. + +## Candidate + +Keep `inDom`'s owner-document guard and its existing `Document.contains` fast path. When containment is false, use a native `Node.isConnected` getter instead of walking shadow hosts and checking containment again. Missing or non-native getters retain the old algorithm. + +The optional getter is validated at first use, not just when the Node prototype is first cached. Only the getter function is cached. Every result is read from the current node, with its receiver preserved. This handles a prototype patched before first use and one patched after the native function was cached. It does not cache DOM values, privacy eligibility, geometry or node membership. Queue processing, serialization options, ordering and mirror cleanup are unchanged. + +A connection to an element's own Document is not the same as being rendered in the top page. Tests preserve the existing behavior for adopted nodes, detached iframe documents, ordinary fragments, documents themselves and open/closed shadow roots. + +## Reproduction and rejected approaches + +- The focused work-count test failed on the baseline with 400 containment calls and 700 root lookups for 300 queries involving light and nested-shadow nodes. The candidate uses 300 containment calls and zero root lookups. These are SDK-level call counts, not a claim about the browser's internal algorithm. +- An initial version replaced the common light-DOM path too. Timings were mixed, so it was rejected. +- A getter fast path without native validation dropped incremental recordings when a page patched `Node.prototype.isConnected` to return false. A real Chromium test failed while its unpatched control passed. +- Merely adding the property to the initial prototype validation does not cover patching after that prototype is cached. The final implementation validates the optional getter at first use, falls back for non-native implementations, and retains only a validated function reference. Browser tests cover patching before and after recording starts. +- JSDOM getters are JS implementations. Unit fast-path cases explicitly model their native signature; built-SDK browser tests remain the real-browser oracle. + +## Unprofiled timing comparison + +Fresh #4812-only rerun: same harness source, sequential builds/runs, alternating baseline/candidate order, five complete samples per build and shape. The fifth candidate arm exceeded the outer command's 600-second limit. That entire partial arm was excluded and rerun in a fresh context; no other benchmark process remained active. About 50k nodes, 11 moves, compression on. Chromium on the local development machine. No invasive counters or concurrent builds/tests ran during these comparisons. + +| Shape | Repeated move longest task, baseline/candidate | Mixed move longest task, baseline/candidate | +| ------------------ | ---------------------------------------------- | ------------------------------------------- | +| table | 371 / 372 ms | 361 / 369 ms | +| flat | 378 / 379 ms | 366 / 385 ms | +| deep (32 wrappers) | 409 / 387 ms | 387 / 384 ms | +| shadow | 303 / 294 ms | 291 / 279 ms | + +These are descriptive medians, not statistical significance or customer guarantees. The useful signal is shadow-heavy work: roughly 3–4% lower longest tasks in this fresh rerun. All five paired repeated-shadow samples improved; four mixed-shadow pairs improved and one tied. Earlier three-pair measurements suggested about 8%, which this rerun does not support as a stable estimate. Light-DOM results overlap and include regressions, particularly the flat mixed-move median (about 5% slower); do not claim a general speedup. The O(moves × descendants) preprocessing work remains, and #4217 is not resolved. + +The final recorder grows from 203908 / 64858 raw/gzip bytes to 204203 / 64944 (+295 / +86). + +## Validation + +- 337 recording/accessor tests passed, 2 existing skips, including eight new connectivity tests. +- 18 Chromium/Firefox/WebKit cases passed: nine connectivity cases and nine existing masking cases. +- All 40 complete arms of the fresh 50k comparison (plus the earlier 24-arm comparison) passed their intermediate replay, privacy, ordering, drop/recovery and duplicate-ID checks. +- 10k table/flat/deep/shadow smoke passed with compression on and off. A 4x CPU-throttled 10k matrix passed too. +- Build/type checks, targeted lint and ES5/ES6 bundle checks passed. + +### Independent privacy fix compatibility + +The bare #4812 baseline retains the known held-shadow privacy bug. It must not be described as passing the extended cancellation matrix. + +For compatibility validation only, PR #4815 commit `089f90663`'s mutation checks and tests were temporarily overlaid on both the performance baseline and candidate: + +- Baseline plus privacy fix: 54 cross-browser blocking cases passed. +- Candidate plus privacy fix: all 72 blocking/connectivity/masking cases passed. +- Candidate plus privacy fix: the retained cancellation harness passed all five shapes, compression off and on, with decoded-wire and intermediate replay assertions. + +The overlay was then removed and the standalone performance candidate rebuilt. No #4815 source or test files remain in this worktree. The timing table above does not include the privacy overlay. The benchmark and proposed PR base remain #4812 alone. #4815 is an independent fix, not part of this performance change. + +## Evidence + +- `/tmp/4217-connectivity-red.log`, `-taint-red.log` +- `/tmp/4217-connectivity-4812-recheck-{baseline,candidate}-{1,2,3,4,5}/results.json` (candidate 5 uses `-5-retry`; exclude the incomplete `-5` directory) +- `/tmp/4217-connectivity-final-{baseline,candidate}-{1,2,3}/results.json` (earlier, not the fresh rerun) +- `/tmp/4217-connectivity-final-record-tests.log`, `-final-browser-tests.log` +- `/tmp/4217-connectivity-final-smoke/`, `-throttled/` +- `/tmp/4217-connectivity-overlay-{baseline-tests,candidate-tests}.log`, `-overlay-cancellation/` +- `/tmp/4217-connectivity-restored-build.log` + +Baseline recorder SHA256: `ccd60e2fb1d540c5564c5b07abd74a94c866b3396132e8122a11125c0b779557`. + +Candidate recorder SHA256: `5a020b6298d6dbbb8ce7d4ab0145062145a822e77ea0b65d7803b9ce16119df5`. diff --git a/packages/rrweb/rrweb/src/utils.ts b/packages/rrweb/rrweb/src/utils.ts index 258abff9c5..bf0b639075 100644 --- a/packages/rrweb/rrweb/src/utils.ts +++ b/packages/rrweb/rrweb/src/utils.ts @@ -617,5 +617,9 @@ export function shadowHostInDom(n: Node): boolean { export function inDom(n: Node): boolean { const doc = n.ownerDocument; if (!doc) return false; - return dom.contains(doc, n) || shadowHostInDom(n); + if (dom.contains(doc, n)) return true; + // The common light-DOM path above stays unchanged. Read live connectivity + // rather than walking shadow hosts (or retrying containment for detached nodes). + const connected = dom.isConnected(n); + return typeof connected === 'boolean' ? connected : shadowHostInDom(n); } diff --git a/packages/rrweb/rrweb/test/record/dom-connectivity.test.ts b/packages/rrweb/rrweb/test/record/dom-connectivity.test.ts new file mode 100644 index 0000000000..fb43fdb115 --- /dev/null +++ b/packages/rrweb/rrweb/test/record/dom-connectivity.test.ts @@ -0,0 +1,176 @@ +// @vitest-environment jsdom + +let dom: typeof import('@posthog/rrweb-utils'); +let utils: typeof import('../../src/utils'); + +beforeEach(async () => { + vi.resetModules(); + dom = await import('@posthog/rrweb-utils'); + utils = await import('../../src/utils'); + // JSDOM getters are implemented in JS. Model a native getter for the fast-path + // unit tests; the built-SDK browser cases exercise real native/tainted getters. + const getter = Object.getOwnPropertyDescriptor( + dom.getUntaintedPrototype('Node'), + 'isConnected', + )?.get; + if (getter) { + vi.spyOn(getter, 'toString').mockReturnValue( + 'function get isConnected() { [native code] }', + ); + } +}); + +afterEach(() => { + vi.restoreAllMocks(); + document.body.innerHTML = ''; +}); + +function legacyInDom(node: Node) { + const doc = node.ownerDocument; + return !!doc && (dom.contains(doc, node) || utils.shadowHostInDom(node)); +} + +describe('DOM connectivity', () => { + it('avoids repeated containment and shadow-host walks', () => { + const host = document.createElement('div'); + const light = document.createTextNode('light'); + const shadow = host.attachShadow({ mode: 'open' }); + const innerHost = document.createElement('div'); + const innerShadow = innerHost.attachShadow({ mode: 'closed' }); + const text = document.createTextNode('shadow'); + innerShadow.append(text); + shadow.append(innerHost); + host.append(light); + document.body.append(host); + const nodes = [host, light, text]; + nodes.forEach((node) => expect(utils.inDom(node)).toBe(true)); + const contains = vi.spyOn(dom.default, 'contains'); + const getRootNode = vi.spyOn(dom.default, 'getRootNode'); + for (let i = 0; i < 100; i++) { + nodes.forEach((node) => expect(utils.inDom(node)).toBe(true)); + } + expect({ + containment: contains.mock.calls.length, + roots: getRootNode.mock.calls.length, + }).toEqual({ containment: 300, roots: 0 }); + }); + + it.each(['open', 'closed'] as const)( + 'matches the existing check through %s shadow moves and adoption', + (mode) => { + const host = document.createElement('div'); + const shadow = host.attachShadow({ mode }); + const text = document.createTextNode('content'); + shadow.append(text); + function check(expected: boolean) { + for (const node of [host, shadow, text]) { + expect(utils.inDom(node)).toBe(expected); + expect(utils.inDom(node)).toBe(legacyInDom(node)); + } + } + check(false); + document.body.append(host); + check(true); + host.remove(); + check(false); + const otherDoc = document.implementation.createHTMLDocument('other'); + otherDoc.adoptNode(host); + check(false); + otherDoc.body.append(host); + check(true); + document.body.append(host); + check(true); + }, + ); + + it('preserves ownership semantics for documents and iframe contents', () => { + expect(utils.inDom(document)).toBe(false); + const iframe = document.createElement('iframe'); + document.body.append(iframe); + const child = iframe.contentDocument!.createElement('span'); + iframe.contentDocument!.body.append(child); + expect(utils.inDom(child)).toBe(true); + iframe.remove(); + // Connected to its own Document is not the same as rendered in the top page. + expect(utils.inDom(child)).toBe(legacyInDom(child)); + const fragment = document.createDocumentFragment(); + fragment.append(child); + expect(utils.inDom(child)).toBe(false); + }); + + it('bypasses a patched instance connectivity getter', () => { + const element = document.createElement('div'); + document.body.append(element); + expect(utils.inDom(element)).toBe(true); + const patched = vi + .spyOn(element, 'isConnected', 'get') + .mockImplementation(() => { + throw new Error('patched getter must not run'); + }); + expect(utils.inDom(element)).toBe(true); + element.remove(); + expect(utils.inDom(element)).toBe(false); + expect(patched).not.toHaveBeenCalled(); + }); + + it('falls back if the cached prototype is patched before first use', () => { + const getter = vi.fn(() => false); + Object.defineProperty(dom.getUntaintedPrototype('Node'), 'isConnected', { + get: getter, + configurable: true, + }); + const host = document.createElement('div'); + const child = document.createTextNode('content'); + host.attachShadow({ mode: 'closed' }).append(child); + document.body.append(host); + const roots = vi.spyOn(dom.default, 'getRootNode'); + expect(utils.inDom(child)).toBe(true); + expect(roots).toHaveBeenCalled(); + expect(getter).not.toHaveBeenCalled(); + host.remove(); + expect(utils.inDom(child)).toBe(false); + }); + + it('keeps the native function, not a cached connectivity value', () => { + const host = document.createElement('div'); + const child = document.createTextNode('content'); + host.attachShadow({ mode: 'open' }).append(child); + document.body.append(host); + expect(utils.inDom(child)).toBe(true); + const getter = vi.fn(() => false); + Object.defineProperty(dom.getUntaintedPrototype('Node'), 'isConnected', { + get: getter, + configurable: true, + }); + expect(utils.inDom(child)).toBe(true); + host.remove(); + expect(utils.inDom(child)).toBe(false); + expect(getter).not.toHaveBeenCalled(); + }); + + it('retains the old walk when connectivity is unavailable', () => { + const prototype = dom.getUntaintedPrototype('Node'); + Object.defineProperty(prototype, 'isConnected', { + value: undefined, + configurable: true, + }); + const host = document.createElement('div'); + const shadow = host.attachShadow({ mode: 'closed' }); + const child = document.createTextNode('content'); + shadow.append(child); + for (const node of [host, shadow, child]) { + Object.defineProperty(node, 'isConnected', { + value: undefined, + configurable: true, + }); + } + document.body.append(host); + const contains = vi.spyOn(dom.default, 'contains'); + const roots = vi.spyOn(dom.default, 'getRootNode'); + expect(utils.inDom(child)).toBe(true); + expect(contains).toHaveBeenCalled(); + expect(roots).toHaveBeenCalled(); + host.remove(); + expect(utils.inDom(child)).toBe(false); + }); +}); diff --git a/packages/rrweb/utils/src/index.ts b/packages/rrweb/utils/src/index.ts index d9134f5991..cefbe758f6 100644 --- a/packages/rrweb/utils/src/index.ts +++ b/packages/rrweb/utils/src/index.ts @@ -235,6 +235,24 @@ export function textContent(n: Node): string | null { return getUntaintedAccessor('Node', n, 'textContent'); } +let isConnectedGetter: PropertyDescriptor['get'] | null | undefined; + +export function isConnected(n: Node): boolean | undefined { + if (isConnectedGetter === undefined) { + const getter = Object.getOwnPropertyDescriptor( + getUntaintedPrototype('Node'), + 'isConnected', + )?.get; + // The prototype may have been cached before this optional getter was + // patched. Validate the function at first use, then cache only that function. + // Non-native or unavailable implementations retain the old containment path. + isConnectedGetter = getter?.toString().includes('[native code]') + ? getter + : null; + } + return isConnectedGetter?.call(n); +} + export function contains(n: Node, other: Node): boolean { return getUntaintedMethod('Node', n, 'contains')(other); } @@ -387,6 +405,7 @@ export default { parentNode, parentElement, textContent, + isConnected, contains, getRootNode, host,