Fix memory leak #1266
Fix memory leak #1266
Conversation
- This commit fixes the main problem with memory leak - Also, this ensures proper cleanup of the event listener by aborting it with a signal. It improves reliability and prevents potential memory leaks or unexpected behavior
- Change other addEventListeners to AbortController way for same code style across listeners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough此次更改主要将事件监听器的注册与清理方式由手动添加和移除，统一替换为使用 AbortController 的 signal 选项进行管理。涉及的事件包括滚轮、鼠标、滚动和窗口尺寸变化等。组件卸载或 effect 重新执行时，通过调用 abort 方法自动移除所有相关监听器，逻辑与事件处理函数本身未发生变化。 Changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/FixedHolder/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. src/stickyScrollBar.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| scrollParents.forEach(p => | ||
| p.addEventListener(SCROLL_EVENT, checkScrollBarVisible, { | ||
| capture: false, | ||
| signal: abortController.signal, |
There was a problem hiding this comment.
The compatibility requirements for using signal are too high
There was a problem hiding this comment.
🧩 Analysis chain
@yoyo837 您提出的关于 signal 参数兼容性要求过高的问题是很有道理的。根据最新的兼容性数据，addEventListener 的 signal 参数支持是比较新的特性：
- Chrome 88+ (2021年1月)
- Firefox 68+ (2019年7月)
- Safari 15.4+ (2022年3月)
- Edge 88+ (2021年1月)
对于一个广泛使用的组件库来说，这确实可能带来兼容性问题。以下是可能的替代方案：
- 保持使用传统的
removeEventListener方式，但确保在组件卸载时正确清理：
React.useEffect(() => {
// 添加事件监听
scrollParents.forEach(p =>
p.addEventListener(SCROLL_EVENT, checkScrollBarVisible, false));
window.addEventListener(RESIZE_EVENT, checkScrollBarVisible, false);
window.addEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
container.addEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
// 返回清理函数
return () => {
scrollParents.forEach(p =>
p.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false));
window.addEventListener(RESIZE_EVENT, checkScrollBarVisible, false);
window.addEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
container.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
};
}, [container]);- 使用特性检测进行优雅降级：
React.useEffect(() => {
const supportsAbortController = typeof AbortController === 'function';
const abortController = supportsAbortController ? new AbortController() : null;
const options = supportsAbortController
? { capture: false, signal: abortController.signal }
: false;
scrollParents.forEach(p =>
p.addEventListener(SCROLL_EVENT, checkScrollBarVisible, options));
// ... 添加其他事件监听
return () => {
if (supportsAbortController) {
abortController.abort();
} else {
scrollParents.forEach(p =>
p.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false));
// ... 移除其他事件监听
}
};
}, [container]);您认为哪种方案更适合当前的项目需求？
🌐 Web query:
What browsers support AbortController signal with addEventListener?
💡 Result:
The AbortController's signal property can be used with addEventListener to manage event listeners that can be aborted. This feature is supported in the following browsers and versions:
- Chrome: Version 88 and later
- Edge: Version 88 and later
- Firefox: Version 86 and later
- Opera: Version 76 and later
In these versions, you can associate an event listener with an AbortSignal by passing the signal as an option in the addEventListener method. This allows you to abort the event listener by calling controller.abort(). (barker.codes)
However, this functionality is not supported in Safari, Safari on iOS, or Opera for Android. (barker.codes)
For browsers that do not support this feature, you can use a polyfill to achieve similar functionality. (barker.codes)
Citations:
- 1: https://barker.codes/blog/aborting-event-listeners-in-vanilla-js/?utm_source=openai
- 2: https://barker.codes/blog/aborting-event-listeners-in-vanilla-js/?utm_source=openai
- 3: https://barker.codes/blog/aborting-event-listeners-in-vanilla-js/?utm_source=openai
🌐 Web query:
When was AbortController signal support added to addEventListener and what is the browser compatibility?
💡 Result:
When Was AbortController Signal Support Added to addEventListener?
Support for using an AbortSignal (via an AbortController) as part of the addEventListener options object was first introduced in Chromium-based browsers. Specifically, Chrome added this feature in version 88, released in January 2021, with default support enabled in Chrome 90 (April 2021)[4][2]. The feature allows developers to automatically remove event listeners when the associated AbortSignal is aborted, simplifying cleanup and reducing memory leaks.
Other browsers followed suit:
- Firefox implemented support around version 86 (released February 2021)[4].
- Safari added support in version 15 (released in September 2021)[4].
Browser Compatibility
| Browser | Minimum Version with Support | Notes |
|---|---|---|
| Chrome | 88 (flag), 90 (default) | Full support from 90[4] |
| Firefox | 86 | Full support[4] |
| Safari | 15 | Full support[4] |
| Edge | Follows Chromium (90+) | Full support |
| iOS Safari | 15 | Full support[4] |
| Older Browsers | Not supported | Polyfills are available[4] |
- Before these versions, while
AbortControllerandAbortSignalwere available for fetch and some other APIs, they could not be used withaddEventListener. - For unsupported browsers (such as Safari 14 or below), developers can use polyfills to simulate this functionality[4].
Usage Example
const controller = new AbortController();
const signal = controller.signal;
element.addEventListener('click', handler, { signal });
// Later, automatically removes the handler:
controller.abort();Summary
- Specification: The signal property in
addEventListeneroptions allows passing anAbortSignal. When aborted, the event listener is removed automatically[2][4][9]. - Chrome: Supported from 88 (behind flag), 90+ by default[4][2].
- Firefox: Supported from version 86[4].
- Safari: Supported from version 15[4].
- Polyfills: Available for Safari 14 and other unsupported browsers[4].
This feature is now broadly available in all major evergreen browsers from early 2021 onwards, making automatic listener cleanup much easier across web applications.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/abort_event
- 2: https://chromestatus.com/feature/5658622220566528
- 3: AbortSignal consumers can potentialy emit abort event on it whatwg/dom#784
- 4: https://css-tricks.com/using-abortcontroller-as-an-alternative-for-removing-event-listeners/
- 5: https://dev.to/omriluz1/abortcontroller-and-signal-handling-3j7a
- 6: Feature request: add AbortController support to addEventListener jsdom/jsdom#3327
- 7: Support
AbortSignals inaddEventListeners options to unsubscribe from events? whatwg/dom#911 - 8: https://javascript.plainenglish.io/best-practices-for-using-abortcontroller-87892b72d07e
- 9: https://www.whitespectre.com/ideas/abortcontroller-api-javascript/
- 10: https://dev.to/leapcell/do-you-really-know-abortcontroller-3628
@yoyo837 您提出的关于signal参数兼容性问题非常有价值。根据最新的浏览器兼容性数据，addEventListener的signal选项是相对较新的特性：
- Chrome: 88版本（2021年1月）开始支持，90版本默认启用
- Firefox: 86版本（2021年2月）开始支持
- Safari: 15版本（2021年9月）开始支持
- Edge: 遵循Chromium（90+）
对于需要广泛兼容性的React组件库，这确实可能带来问题。以下是几种替代方案：
- 传统的
removeEventListener方式，确保正确清理：
React.useEffect(() => {
scrollParents.forEach(p =>
p.addEventListener(SCROLL_EVENT, checkScrollBarVisible, false));
window.addEventListener(RESIZE_EVENT, checkScrollBarVisible, false);
window.addEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
container.addEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
return () => {
scrollParents.forEach(p =>
p.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false));
window.removeEventListener(RESIZE_EVENT, checkScrollBarVisible, false);
window.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
container.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
};
}, [container]);- 特性检测方式：
React.useEffect(() => {
const supportsAbortController = typeof AbortController === 'function' &&
'signal' in AbortController.prototype;
const abortController = supportsAbortController ? new AbortController() : null;
const options = supportsAbortController
? { capture: false, signal: abortController.signal }
: false;
// 添加事件监听
scrollParents.forEach(p =>
p.addEventListener(SCROLL_EVENT, checkScrollBarVisible, options));
// ...其他事件监听
return () => {
if (supportsAbortController) {
abortController.abort();
} else {
scrollParents.forEach(p =>
p.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false));
// ...其他事件移除
}
};
}, [container]);您认为哪种解决方案更适合当前项目的需求和兼容性目标？
✏️ Learnings added
Learnt from: yoyo837
PR: react-component/table#1266
File: src/stickyScrollBar.tsx:174-174
Timestamp: 2025-05-08T01:01:54.659Z
Learning: AbortController and its signal parameter with addEventListener have high browser compatibility requirements. The signal option for addEventListener was added in Chrome 88+ (Jan 2021), Firefox 68+ (July 2019), Safari 15.4+ (March 2022), and Edge 88+ (Jan 2021). For wider browser support, traditional removeEventListener cleanup is preferred.
Learnt from: yoyo837
PR: react-component/table#1266
File: src/stickyScrollBar.tsx:174-174
Timestamp: 2025-05-08T01:01:54.659Z
Learning: AbortController and its signal parameter with addEventListener have high browser compatibility requirements. The signal option for addEventListener was added in Chrome 88+ (Jan 2021), Firefox 86+ (Feb 2021), Safari 15+ (Sept 2021), and Edge 88+ (Jan 2021). For wider browser support, traditional removeEventListener cleanup is preferred.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1266 +/- ##
=======================================
Coverage 98.03% 98.03%
=======================================
Files 76 76
Lines 7425 7447 +22
Branches 1146 1145 -1
=======================================
+ Hits 7279 7301 +22
Misses 140 140
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a memory leak issue by updating event listener cleanup in two components using AbortController, ensuring that all event listeners are automatically removed when the component unmounts or dependencies change.
- Replaces manual removeEventListener calls with AbortController.abort() in src/stickyScrollBar.tsx for mouse and scroll events.
- Updates the wheel event listener in src/FixedHolder/index.tsx to use AbortController for cleanup.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/stickyScrollBar.tsx | Updated mouse and scroll event listeners to use AbortController, ensuring proper cleanup. |
| src/FixedHolder/index.tsx | Updated wheel event listener to use AbortController and eliminate the manual removeEventListener call. |
Comments suppressed due to low confidence (1)
src/FixedHolder/index.tsx:87
- [nitpick] Avoid unnecessary type assertion from native WheelEvent to React.WheelEvent. Consider using native event properties directly for improved type safety.
function onWheel(e: WheelEvent) { const { currentTarget, deltaX } = e as unknown as React.WheelEvent<HTMLDivElement>;
| scrollRef.current?.addEventListener('wheel', onWheel, { passive: false }); | ||
| scrollRef.current?.addEventListener('wheel', onWheel, { | ||
| passive: false, | ||
| signal: abortController.signal, |
There was a problem hiding this comment.
Before move to signal, seems why removeEventListener not working is much important. If scrollRef.current is not valid when unmount, we can just simply modify as :
const holder = scrollRef.current;
holder?.addEventListener(...)
return () => {
holder?.removeEventListener(...)
};There was a problem hiding this comment.
If you dont want to move to AbortController it will work as expected. But we have more problems than I thought. In stickyScrollBar.tsx the handlers (onMouseUp, onMouseMove and checkScrollBarVisible) are declared inside the body of the component function, that will also lead to memory leak because after each render the reference for this functions will be re-created and the cleanup function will have reference to the first declaration of this functions and removeEventListener will not work because of mismatching handler references, so looks like we should wrap them to useCallback?? But damn, I'm too scaried to do that because we should properly control the dependencies for this useEffects and combine them with dependencies of another useEffect which also has their own depencies (scrollBarWidth, isActive)
There was a problem hiding this comment.
onMouseUp onMouseMove & checkScrollBarVisible are correctly handle with closure for each render. Could you provide a minimun reproduce of this?
I found a complicated memery leak problem which caused by src/FixedHolder/index.tsx
Inside main
useEffectcomponent adds listener toscrollRef. In code everything looks okay but in productionremoveEventListeneris never being called.Here is the snapshot from memory inspector:
To detect the problem I patched
node_modulesfiles and addconsole.logs.As you see "remove listener" log is never being called even though "before remove" log is called
Summary by CodeRabbit