fix(record): port rrweb 2.0.1 WebKit MutationObserver fix#3737
Open
arnohillen wants to merge 3 commits into
Open
fix(record): port rrweb 2.0.1 WebKit MutationObserver fix#3737arnohillen wants to merge 3 commits into
arnohillen wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/rrweb/utils/src/index.ts:134
**Stale cache after cleanup breaks a stop-then-restart recording session on Safari**
`untaintedBasePrototype[key]` is never cleared when `iframeCleanup` is called. On the next call to `getUntaintedPrototype('MutationObserver')` (line 72–73) the cache returns the constructor from the now-removed iframe whose `ScriptExecutionContext` WebKit has already torn down — the exact silent-breakage this PR is fixing. Any workflow that stops and restarts session recording on the same page (e.g. idle-pause / resume) will reproduce the original bug on Safari.
```suggestion
untaintedBaseIframeCleanup[key] = () => {
iframeEl.remove();
// Clear the cached prototype so a subsequent recording session creates a
// fresh iframe rather than reusing a constructor from a torn-down context.
delete untaintedBasePrototype[key];
delete untaintedBaseIframeCleanup[key];
};
```
Reviews (1): Last reviewed commit: "fix(record): port rrweb 2.0.1 WebKit Mut..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // rr-block prevents rrweb from serializing this iframe in later snapshots | ||
| iframeEl.classList.add('rr-block'); | ||
| iframeEl.setAttribute('__rrwebUntaintedMutationObserver', ''); | ||
| untaintedBaseIframeCleanup[key] = () => iframeEl.remove(); |
Contributor
There was a problem hiding this comment.
untaintedBasePrototype[key] is never cleared when iframeCleanup is called. On the next call to getUntaintedPrototype('MutationObserver') (line 72–73) the cache returns the constructor from the now-removed iframe whose ScriptExecutionContext WebKit has already torn down — the exact silent-breakage this PR is fixing. Any workflow that stops and restarts session recording on the same page (e.g. idle-pause / resume) will reproduce the original bug on Safari.
Suggested change
| untaintedBaseIframeCleanup[key] = () => iframeEl.remove(); | |
| untaintedBaseIframeCleanup[key] = () => { | |
| iframeEl.remove(); | |
| // Clear the cached prototype so a subsequent recording session creates a | |
| // fresh iframe rather than reusing a constructor from a torn-down context. | |
| delete untaintedBasePrototype[key]; | |
| delete untaintedBaseIframeCleanup[key]; | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/rrweb/utils/src/index.ts
Line: 134
Comment:
**Stale cache after cleanup breaks a stop-then-restart recording session on Safari**
`untaintedBasePrototype[key]` is never cleared when `iframeCleanup` is called. On the next call to `getUntaintedPrototype('MutationObserver')` (line 72–73) the cache returns the constructor from the now-removed iframe whose `ScriptExecutionContext` WebKit has already torn down — the exact silent-breakage this PR is fixing. Any workflow that stops and restarts session recording on the same page (e.g. idle-pause / resume) will reproduce the original bug on Safari.
```suggestion
untaintedBaseIframeCleanup[key] = () => {
iframeEl.remove();
// Clear the cached prototype so a subsequent recording session creates a
// fresh iframe rather than reusing a constructor from a torn-down context.
delete untaintedBasePrototype[key];
delete untaintedBaseIframeCleanup[key];
};
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Contributor
|
Size Change: +25.6 kB (+0.15%) Total Size: 16.7 MB
ℹ️ View Unchanged
|
Sync the only functional upstream change since the fork's baseline (rrweb@2.0.0-alpha.18): rrweb-io/rrweb#1854. WebKit/Safari tears down a detached iframe's ScriptExecutionContext, so the pristine MutationObserver rrweb pulls from a throwaway iframe (to dodge 3rd-party monkey-patching) silently stopped delivering mutations once the iframe was removed, halting DOM recording on affected Safari pages. Keep that iframe attached on Safari (hidden, rr-block-tagged) and tear it down via a cleanup wired through initMutationObserver's teardown. Co-authored-by: Cursor <cursoragent@cursor.com>
Rebase onto main. Export cleanupUntaintedIframe instead of threading iframeCleanup through initMutationObserver. Add unit and puppeteer tests for monkey-patched MutationObserver recording. Co-authored-by: Cursor <cursoragent@cursor.com>
arnohillen
force-pushed
the
ahillen/rrweb-sync-webkit-mutationobserver
branch
from
addc9ee to
1082c45
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
142
to
143
| return defaultPrototype; | ||
| } finally { | ||
| if (iframeEl.parentNode) { | ||
| document.body.removeChild(iframeEl); | ||
| } | ||
| } |
There was a problem hiding this comment.
Memory leak: The catch block doesn't clean up the iframe that was appended at line 115. The old code had a finally block that removed the iframe in all cases. Now if an exception is thrown after appendChild (line 115) but before the cleanup function is stored (line 135), the iframe will leak on Safari. Even on non-Safari browsers, if an exception occurs between line 115 and line 137, the iframe leaks.
Fix:
catch {
iframeEl.remove();
return defaultPrototype;
}
Suggested change
| return defaultPrototype; | |
| } finally { | |
| if (iframeEl.parentNode) { | |
| document.body.removeChild(iframeEl); | |
| } | |
| } | |
| iframeEl.remove(); | |
| return defaultPrototype; | |
| } |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Cover stop/start recording, iframe cleanup after teardown, and mixed attribute/child-list mutations under a hijacked MutationObserver. Co-authored-by: Cursor <cursoragent@cursor.com>
marandaneto
requested review from
a team,
TueHaulund,
fasyy612 and
ksvat
and removed request for
a team
TueHaulund
reviewed
Jun 15, 2026
Comment on lines
+149
to
+150
| untaintedBaseIframeCleanup[key]?.(); | ||
| delete untaintedBaseIframeCleanup[key]; |
Contributor
There was a problem hiding this comment.
Must-fix: this removes the iframe but never evicts untaintedBasePrototype[key]. Since getUntaintedPrototype early-returns the cached prototype, the next record() on Safari hands back a constructor bound to the now-removed iframe — so mutations record on the first session only and every later start/stop (SPA reinit, reset(), stop/restart) records nothing, the exact bug this PR fixes, one cycle later.
Suggested change
| untaintedBaseIframeCleanup[key]?.(); | |
| delete untaintedBaseIframeCleanup[key]; | |
| untaintedBaseIframeCleanup[key]?.(); | |
| delete untaintedBaseIframeCleanup[key]; | |
| delete untaintedBasePrototype[key]; |
Comment on lines
+49
to
+50
| utils.cleanupUntaintedIframe(); | ||
| expect(document.querySelector('iframe')).toBeNull(); |
Contributor
There was a problem hiding this comment.
This stops at one ctor → cleanup, so nothing here catches the bug above. Add a second mutationObserverCtor() after cleanup and assert a fresh iframe.rr-block is re-created — that fails today (the cached prototype is never evicted) and passes once it is.
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
I reviewed upstream rrweb's releases (
rrweb-io/rrweb) against our vendored fork (packages/rrweb/*,@posthog/rrweb). The fork's upstream baseline isrrweb@2.0.0-alpha.18; upstream has since cut2.0.0and2.0.1.The headline of
2.0.0is a packaging/dist restructure (split plugin packages, new@rrweb/browser-client, new file extensions) that we already do our own incompatible way (our@posthog/rrweb-*workspace split + property mangling + custom vite build), so adopting it would fight our build. Stripping out docs/CI/browser-clientnoise, the only functional code change upstream landed after our baseline is the2.0.1WebKit fix.That fix matters: on WebKit/Safari, when a 3rd-party library has monkey-patched the page, rrweb grabs a pristine
MutationObserverconstructor from a throwaway iframe to avoid the tainted one. WebKit tears down a detached iframe'sScriptExecutionContext, so the observer built from it silently stops delivering mutations once we remove the iframe (webkit.org/b/179224) — i.e. no DOM mutations get recorded on affected Safari pages.Changes
Ported rrweb-io/rrweb#1854, reconciled against our diverged fork:
@posthog/rrweb-utils(getUntaintedPrototype): on Safari, keep the untainted-prototype iframe attached (hidden viadisplay:none, taggedrr-block+__rrwebUntaintedMutationObserverso it isn't serialized into snapshots) instead of removing it.mutationObserverCtor()now returns[ctor, cleanup].@posthog/rrweb(initMutationObserver/initObservers): thread theiframeCleanupthrough and invoke it on observer teardown so we don't leak the iframe.Adapted to our fork's shape: our
initMutationObserverreturns an object ({ observer, buffer }) rather than upstream's tuple, soshadow-dom-managerneeds no change (it just ignores the extraiframeCleanupfield). Deliberately not porting the upstream WebKit Docker/CI test harness (Dockerfile.webkit-test,vitest.config.webkit.ts) — that's a separate CI infrastructure decision.Typecheck + eslint clean on both packages.
Release info Sub-libraries affected
Libraries affected
(Changeset bumps
posthog-js,@posthog/rrweb,@posthog/rrweb-utilsas patch.)Checklist
mutationObserverCtorsignature changed but it has a single internal caller; no externaldom.mutationObserver(consumers.If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Scope was deliberately narrowed from "sync all rrweb updates": a literal full merge of
alpha.18 → 2.0.1(65 commits) would drag in upstream's 2.0.0 packaging restructure which conflicts with our custom build/mangling. The genuine functional delta since our baseline is just the2.0.1WebKit MutationObserver fix, which this PR ports.Made with Cursor