fix: clear mutation buffer and remove mirror node on iframe pagehide event #1755
Conversation
🦋 Changeset detectedLatest commit: 7b2416a 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a memory leak by ensuring proper cleanup of iframe-related resources when iframes are unloaded. Specifically, it clears mutation buffers and removes mirror nodes when an iframe's pagehide event fires.
Key Changes:
- Added infrastructure to track and clean up iframe handlers and mutation buffers when iframes are hidden
- Introduced a
pagehideevent listener on iframe content windows to trigger cleanup operations - Implemented methods to identify and remove mutation buffers associated with specific iframes
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rrweb/src/record/observer.ts | Adds findAndRemoveIframeBuffer function to locate and remove mutation buffers belonging to a specific iframe |
| packages/rrweb/src/record/mutation.ts | Adds bufferBelongsToIframe method to identify if a mutation buffer belongs to a given iframe |
| packages/rrweb/src/record/index.ts | Implements iframeHandlersMap to track iframe handlers and cleanup logic via addPageHideListener |
| packages/rrweb/src/record/iframe-manager.ts | Adds pageHideListener property and addPageHideListener method, attaches pagehide event listener to iframe content windows |
| .changeset/eleven-knives-rescue.md | Documents the memory leak fix in the changeset |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| iframeManager.addLoadListener((iframeEl) => { | ||
| try { | ||
| handlers.push(observe(iframeEl.contentDocument!)); | ||
| iframeHandlersMap.set(iframeEl, observe(iframeEl.contentDocument!)); |
There was a problem hiding this comment.
The handler is added to iframeHandlersMap but not to the handlers array. This creates an inconsistency where the cleanup in the return function iterates both collections separately. Consider adding the handler to both collections or documenting why this handler should only be in the map.
| iframeHandlersMap.set(iframeEl, observe(iframeEl.contentDocument!)); | |
| const handler = observe(iframeEl.contentDocument!); | |
| iframeHandlersMap.set(iframeEl, handler); | |
| handlers.push(handler); |
| iframeEl.contentWindow?.addEventListener('pagehide', () => { | ||
| this.pageHideListener?.(iframeEl); | ||
| this.mirror.removeNodeFromMap(iframeEl.contentDocument!); | ||
| this.crossOriginIframeMap.delete(iframeEl.contentWindow!); | ||
| }); |
There was a problem hiding this comment.
The pagehide event listener is added but never removed, which could cause memory leaks if the iframe element is reused or if the manager is disposed. Consider storing the listener function reference and providing a cleanup mechanism to remove it when the iframe is detached.
1 participant
No description provided.