Skip to content
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.

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

fix:addEventlistener memory leak. #2883

Merged
merged 6 commits into from
Jan 28, 2024
Merged

fix:addEventlistener memory leak. #2883

merged 6 commits into from
Jan 28, 2024

Conversation

qiongshusheng
Copy link
Contributor

场景1:当用户通过addEventlistener多次添加同一个监听时,如果listener和capture/useCapture都相同,浏览器会认为是同一个监听,后添加的监听会覆盖之前添加的监听,取消的时候也只需要取消一次;
场景2:另外当options的once为true时,如果监听执行了是不需要取消的。

问题:针对以上两种情形,如果用户多次添加同一个监听,或者options中的once设为true,没有移除监听,就会导致listenerMap中的监听无法移除。

解决方案:针对多次添加同一个监听情况做判断,如果是同一个监听capture/useCapture都相同,则先移除之前的监听在添加新的监听,针对once场景,当监听执行后自动移除listenerMap中的监听。

Copy link

changeset-bot bot commented Jan 10, 2024

⚠️ No Changeset found

Latest commit: 75f69c0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jan 10, 2024

@qiongshusheng is attempting to deploy a commit to a Personal Account owned by @umijs on Vercel.

@umijs first needs to authorize it.

@qiongshusheng qiongshusheng requested a review from kuitos January 12, 2024 02:32
);
if (findIndex > -1) {
const { listener: findListener, options: findOptions } = cachedTypeListeners[findIndex];
// 如果存在相同的监听,先移除之前的监听,在添加新的监听
Copy link
Member

Choose a reason for hiding this comment

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

这里不需要先移除吧,同一个 listener 什么都不干就行吧?

Copy link
Contributor Author

@qiongshusheng qiongshusheng Jan 17, 2024

Choose a reason for hiding this comment

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

这里不需要先移除吧,同一个 listener 什么都不干就行吧?

之前想过相同的不移除也不添加,但是对于浏览器来说相同的会覆盖之前,因为没有对options里面的每一个参数都做比对,虽然listener和capture/useCapture相同,但是其他options有可能不同,所以最简单的方法就是先移除,在添加,达到覆盖的效果。

Copy link
Member

Choose a reason for hiding this comment

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

原生的行为是什么样的?listener 相同但是 options 不同的情况下

Copy link
Member

Choose a reason for hiding this comment

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

image
只需要判断 capture 就行了,跟 removeEventListener 逻辑是一样的

Copy link
Contributor Author

@qiongshusheng qiongshusheng Jan 17, 2024

Choose a reason for hiding this comment

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

image 只需要判断 capture 就行了,跟 removeEventListener 逻辑是一样的

是的,我代码里面有贴这一部分的developer.mozilla上的说明链接,listener和capture/useCapture相同的情况下,不管是移除还是添加,浏览器都会认为他们是同一个,移除的话直接移除,添加的话会覆盖之前的,所以才移除在添加的。

Copy link
Member

Choose a reason for hiding this comment

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

添加的话会覆盖之前的

这个好像没看到出处,贴一下链接?

Copy link
Contributor Author

@qiongshusheng qiongshusheng Jan 20, 2024

Choose a reason for hiding this comment

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

添加的话会覆盖之前的

这个好像没看到出处,贴一下链接?

image
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
这块之前看错了,对于DOM0和DOM1级的事件(target.onEvent = fn)会覆盖,DOM2和DOM3级的事件(addEventListener)如果target 的event listeners 的list上已经添加了就会忽略掉, 我做了一个简单的测试,跟文档上提到的一样,我已经重新改了代码。

demo1: 窗口改变只会执行一次
const fn = ()=> console.log('onresize');
window.addEventListener('resize', fn, {once: true});
window.addEventListener('resize', fn, {once: false});

demo2: 窗口改变会执行多次
const fn = ()=> console.log('onresize');
window.addEventListener('resize', fn, {once: false});
window.addEventListener('resize', fn, {once: true});

Copy link
Member

@kuitos kuitos left a comment

Choose a reason for hiding this comment

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

LGTM

@kuitos kuitos merged commit cbd6cc0 into umijs:master Jan 28, 2024
3 of 4 checks passed
@kuitos kuitos mentioned this pull request Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants