Skip to content

Fix memory leak #1266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix memory leak #1266

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9bcb665
Add AbortController to clean up event listeners
May 7, 2025
e971bb1
Add AbortController to clean up event listeners
May 7, 2025
136748b
Merge branch 'master' into fix-memory-leak
afc163 May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/FixedHolder/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ const FixedHolder = React.forwardRef<HTMLDivElement, FixedHeaderProps<any>>((pro
}, []);

React.useEffect(() => {
const abortController = new AbortController();

function onWheel(e: WheelEvent) {
const { currentTarget, deltaX } = e as unknown as React.WheelEvent<HTMLDivElement>;
if (deltaX) {
Expand All @@ -104,10 +106,13 @@ const FixedHolder = React.forwardRef<HTMLDivElement, FixedHeaderProps<any>>((pro
e.preventDefault();
}
}
scrollRef.current?.addEventListener('wheel', onWheel, { passive: false });
scrollRef.current?.addEventListener('wheel', onWheel, {
passive: false,
signal: abortController.signal,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...)
};

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onMouseUp onMouseMove & checkScrollBarVisible are correctly handle with closure for each render. Could you provide a minimun reproduce of this?

});

return () => {
scrollRef.current?.removeEventListener('wheel', onWheel);
abortController.abort();
};
}, []);

Expand Down
42 changes: 30 additions & 12 deletions src/stickyScrollBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,18 +141,25 @@ const StickyScrollBar: React.ForwardRefRenderFunction<unknown, StickyScrollBarPr
}));

React.useEffect(() => {
document.body.addEventListener(MOUSEUP_EVENT, onMouseUp, false);
document.body.addEventListener(MOUSEMOVE_EVENT, onMouseMove, false);
const abortController = new AbortController();
document.body.addEventListener(MOUSEUP_EVENT, onMouseUp, {
capture: false,
signal: abortController.signal,
});
document.body.addEventListener(MOUSEMOVE_EVENT, onMouseMove, {
capture: false,
signal: abortController.signal,
});
checkScrollBarVisible();
return () => {
document.body.removeEventListener(MOUSEUP_EVENT, onMouseUp);
document.body.removeEventListener(MOUSEMOVE_EVENT, onMouseMove);
abortController.abort();
};
}, [scrollBarWidth, isActive]);

// Loop for scroll event check
React.useEffect(() => {
if (!scrollBodyRef.current) return;
const abortController = new AbortController();

const scrollParents: (HTMLElement | SVGElement)[] = [];
let parent = getDOM(scrollBodyRef.current);
Expand All @@ -161,16 +168,27 @@ const StickyScrollBar: React.ForwardRefRenderFunction<unknown, StickyScrollBarPr
parent = parent.parentElement;
}

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);
scrollParents.forEach(p =>
p.addEventListener(SCROLL_EVENT, checkScrollBarVisible, {
capture: false,
signal: abortController.signal,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compatibility requirements for using signal are too high

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@yoyo837 您提出的关于 signal 参数兼容性要求过高的问题是很有道理的。根据最新的兼容性数据，addEventListener 的 signal 参数支持是比较新的特性：

  • Chrome 88+ (2021年1月)
  • Firefox 68+ (2019年7月)
  • Safari 15.4+ (2022年3月)
  • Edge 88+ (2021年1月)

对于一个广泛使用的组件库来说，这确实可能带来兼容性问题。以下是可能的替代方案：

  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]);
  1. 使用特性检测进行优雅降级：
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:

🌐 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 AbortController and AbortSignal were available for fetch and some other APIs, they could not be used with addEventListener.
  • 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 addEventListener options allows passing an AbortSignal. 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:

@yoyo837 您提出的关于signal参数兼容性问题非常有价值。根据最新的浏览器兼容性数据，addEventListener的signal选项是相对较新的特性：

  • Chrome: 88版本（2021年1月）开始支持，90版本默认启用
  • Firefox: 86版本（2021年2月）开始支持
  • Safari: 15版本（2021年9月）开始支持
  • Edge: 遵循Chromium（90+）

对于需要广泛兼容性的React组件库，这确实可能带来问题。以下是几种替代方案：

  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.removeEventListener(RESIZE_EVENT, checkScrollBarVisible, false);
    window.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
    container.removeEventListener(SCROLL_EVENT, checkScrollBarVisible, false);
  };
}, [container]);
  1. 特性检测方式：
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.

}),
);
window.addEventListener(RESIZE_EVENT, checkScrollBarVisible, {
capture: false,
signal: abortController.signal,
});
window.addEventListener(SCROLL_EVENT, checkScrollBarVisible, {
capture: false,
signal: abortController.signal,
});
container.addEventListener(SCROLL_EVENT, checkScrollBarVisible, {
capture: false,
signal: abortController.signal,
});

return () => {
scrollParents.forEach(p => p.removeEventListener(SCROLL_EVENT, checkScrollBarVisible));
window.removeEventListener(RESIZE_EVENT, checkScrollBarVisible);
window.removeEventListener(SCROLL_EVENT, checkScrollBarVisible);
container.removeEventListener(SCROLL_EVENT, checkScrollBarVisible);
abortController.abort();
};
}, [container]);

Expand Down