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

Detached nodes after unmount component(Memory leaks) #5363

Closed
JhonWeawer opened this issue Feb 5, 2022 · 19 comments
Closed

Detached nodes after unmount component(Memory leaks) #5363

JhonWeawer opened this issue Feb 5, 2022 · 19 comments
Labels
🐞 bug Something isn't working
Projects

Comments

@JhonWeawer
Copy link

JhonWeawer commented Feb 5, 2022

Version

3.2.29

Reproduction link

github.com

Steps to reproduce

first step run npm run build, open dist/index.html
vue3-1
highlight element with id app , write in console $0.__vue_app__.unmount()
after unmount app create heap snapshot, we have memory leaks in heap snapshot
vue3-2

What is expected?

zero detached dom elements and detached vue EventListeners...

What is actually happening?

exist in heap snapshot:
2001 Detached HTMLDivElement
Detached EventListener
...

@JhonWeawer JhonWeawer changed the title Detached nodes due to Vue v-on working(Memory leaks) Detached nodes after unmount component(Memory leaks) Feb 5, 2022
@JhonWeawer JhonWeawer reopened this Feb 5, 2022
@caozhong1996

This comment was marked as outdated.

@LinusBorg
Copy link
Member

@Akryum Do you agree with @caozhong1996? Should we move this issue to the devtools repo?

@JhonWeawer
Copy link
Author

JhonWeawer commented Feb 8, 2022

@caozhong1996 memory leak reproducible, I showed an example without vue-devtools(prod build npm run build) see url in second screenshot ends with dist/index.html , and i reproduce in incognito mode, please check again

@caozhong1996
Copy link
Contributor

Sorry, I misunderstood you, it's similar to vue-devtools's behavior, but it's another problem😥.
There is a related issue

It seems that those invokers not be cleaned during unmount.

const invokers = el._vei || (el._vei = {})

@JhonWeawer
Copy link
Author

JhonWeawer commented Feb 8, 2022

@caozhong1996 Thanks for the research, and what to do about it?
Also i made an issue with the same problem on vue 2 please answer it too(bug also reproduce after prod build npm run build for vue 2 have detached nodes, check pls) link for the same issue memory leak for vue 2 - > vuejs/vue#12445

@LinusBorg
Copy link
Member

@caozhong1996 Interesting. I don't see an apparent reason though why these should not be garbage collectable - the elements are detached, the reference to the component instance in the invoker's closure should also not be a reason as those instances have been unmounted and detached as well.

Do you have an idea/suspicion?

@caozhong1996
Copy link
Contributor

I'm just as confused by this as you, even in this situation which still appeared😥

<!-- no event binding -->
<div v-for="item in 1000" v-bind:key="item">
  <div>111<div/>
</div>

@JhonWeawer
Copy link
Author

@caozhong1996 what exactly confused you ?

@LinusBorg
Copy link
Member

We just have a hard time figuring out a reason as to why detached elements aren't garbage collected.

@JhonWeawer
Copy link
Author

@LinusBorg In view 2 the same behavior of detached nodes after destoy App. vuejs/vue#12445, maybe this isue for vue 2 will be useful

@JhonWeawer
Copy link
Author

I'm just as confused by this as you, even in this situation which still appeared😥

<!-- no event binding -->
<div v-for="item in 1000" v-bind:key="item">
  <div>111<div/>
</div>

@caozhong1996 Event binding is in the example, on the button element

@caozhong1996
Copy link
Contributor

@LinusBorg Now I'm pretty sure it's a bug.

We created invoker at here and reference component instance.

function createInvoker(
initialValue: EventValue,
instance: ComponentInternalInstance | null
) {
const invoker: Invoker = (e: Event) => {
// async edge case #6566: inner click event triggers patch, event handler
// attached to outer element during patch, and triggered again. This
// happens because browsers fire microtask ticks between event propagation.
// the solution is simple: we save the timestamp when a handler is attached,
// and the handler would only fire if the event passed to it was fired
// AFTER it was attached.
const timeStamp = e.timeStamp || _getNow()
if (skipTimestampCheck || timeStamp >= invoker.attached - 1) {
callWithAsyncErrorHandling(
patchStopImmediatePropagation(e, invoker.value),
instance,
ErrorCodes.NATIVE_EVENT_HANDLER,
[e]
)
}
}
invoker.value = initialValue
invoker.attached = getNow()
return invoker
}

These elements reference invoker's closure (_vei)

const invoker = (invokers[rawName] = createInvoker(nextValue, instance))
addEventListener(el, name, invoker, options)

So after removeChild, these elements become detached, even can't remove component instance.

parent.removeChild(child)

Because we should remove these references before removeChild:

As long as a reference is kept on the removed child, it still exists in memory, but is no longer part of the DOM. It can still be reused later in the code. mdn

@LinusBorg
Copy link
Member

LinusBorg commented Feb 11, 2022

the invoker closure doesn't reference the element, it only references the instance. the MDN statement you quoted refers to things that are still active on the main thread (Whatever the correct term may be) having a reference on the removed element, which is not the case here?

@caozhong1996
Copy link
Contributor

@LinusBorg here we go
无标题-2022-02-11-1656
impicture_20220211_175745
impicture_20220211_180029

@jnorris-cs
Copy link

Any update on this? This memory leak is hurting our application's performance.

@LinusBorg LinusBorg added the 🐞 bug Something isn't working label May 2, 2022
@LinusBorg LinusBorg added this to High Priority in Next Patch May 2, 2022
@yyx990803 yyx990803 added ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. and removed ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. labels May 10, 2022
@yyx990803
Copy link
Member

yyx990803 commented May 10, 2022

I investigated the repro and noticed a few very complicated conditions. For the original reproduction, the "leak" only happens if I click the button first, then unmount the app via the inspector + console.

  • If I do not click the button, then the leak doesn't happen. This is a weird condition.
  • If the app is unmounted programmatically in other ways, the leak also disappears:

If you change main.js to the following:

const app = createApp(App)
app.mount('#app')

setTimeout(() => {
  app.unmount()
}, 1000)

The leak doesn't happen after app is unmounted.

If you add a button outside of the app (in public/index.html), and then manually attach an event listener to it that calls app.unmount(), the leak also doesn't happen. So I don't think the leak is caused by Vue's patching mechanism, and in some way Chrome devtool inspection also affects the behavior.

The leak does happen if the unmount is triggered by an event dispatched from an element within the unmounted parent element. Interestingly, this seems to be a Chrome bug - because I was able to reproduce this without Vue involved at all in this gist.

In addition, this leak seems to be "temporary" - i.e. if this process is repeated by clicking the "reset" button in the above gist, the total amount of nodes in memory does not increase. In fact, I've noticed that the detached nodes are garbage collected when another event listener is triggered. Previously detached nodes are somehow reclaimed. So in some way, this isn't really a "leak" since the total memory does not increase over time.


To sum up: this seems to be a Chrome issue, and I couldn't find any way to "fix" this in Vue, since even replacing current event patching with plain addEventListener calls makes no difference from my tests. The fact that it also affects Vue 2 may be an indication that this is a recently introduced bug in Chrome. I've filed a Chrome bug here: https://bugs.chromium.org/p/chromium/issues/detail?id=1324096

@qiyuan-wang
Copy link
Contributor

Try @JhonWeawer 's reproduce repo on chrome 104.0.5112.79 with vuejs@3.2.37, still reproduce this problem.

It seems not a chrome issue or the fixed issue still not work with vuejs.

@LinusBorg @yyx990803 Is this would be fixed? It hurts performance.

@azhar283
Copy link

azhar283 commented Sep 1, 2022

I am seeing the same issue of a large number of detached html elements. However, I have noticed this in chrome. Firefox works fine. If this is a chrome issue, can someone link it here?

@jaredmcateer
Copy link

I tested @yyx990803's gist in chrome 104 and 112, while the chromium issue appeared to have been fixed in 104, it's back in 112 or something similar anyways. I also tested the original reproduction with Vue 3.2.47 in both chrome 104 and 112 and I see detached nodes in both versions, so as mentioned above it doesn't seem like it was ever fixed.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

8 participants