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

Possible memory leak with dropdown and vue-router #1391

Closed
tochoromero opened this Issue Nov 21, 2017 · 26 comments

Comments

Projects
None yet
3 participants
@tochoromero
Contributor

tochoromero commented Nov 21, 2017

Hello guys, I have found a memory leak when using the drop-down component and switching between routes.
Here is a reproduction link https://jsfiddle.net/tochoromero/1tup51he/2/

If you use the link to move back and forth and take a look at the memory usage you will see that the VueComponents are not being garbage collected.

I initialize opened an issue against the Vue repo, but they suggested to open it here instead, here is the link to the other issue vuejs/vue#7101.

I hope you guys have an idea of what is going on and can provide an isolated reproduction example without having to add bootstrap-vue.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

Thanks, we'll check into this to see what may be happening. It is probably that Popper.js instance may not be garbage collected when routes change.

Would you by chance be using the <keep-alive> Vue component?

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Nov 21, 2017

Thanks for looking into this. And no, I'm not using the <keep-alive> component.

@tmorehouse tmorehouse changed the title from Memory leak with vue-bootstrap to Possible memory leak with dropdown and vue-router Nov 21, 2017

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

I think we have discovered the possible culprit... doing some dropdown garbage collection on destroyed() rather than beforeDestroy()

tmorehouse added a commit that referenced this issue Nov 21, 2017

fix(dropdowns): remove memory leak on destroy
Also, we don't bother instantiating Popper if dropdown is in a navbar.

Closes issue #1391
@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 22, 2017

Fix should be available in next release.

tmorehouse added a commit that referenced this issue Nov 22, 2017

fix(dropdowns): prevent memory leak on destroy (#1392)
* fix(dropdowns): remove memory leak on destroy

Also, we don't bother instantiating Popper if dropdown is in a navbar.

Closes issue #1391

* ESLint

pi0 added a commit that referenced this issue Nov 29, 2017

fix(dropdowns): prevent memory leak on destroy (#1392)
* fix(dropdowns): remove memory leak on destroy

Also, we don't bother instantiating Popper if dropdown is in a navbar.

Closes issue #1391

* ESLint
@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Nov 29, 2017

Unfortunately, I'm still able to reproduce the issue on 1.3.0 :(

You can still see it on the same jsfiddle
https://jsfiddle.net/tochoromero/1tup51he/2/

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 29, 2017

Hmmm, I am not sure what would be causing this, other than Popper.js itself, as we are removing the Popper.js instance during the beforeDestroy() hook.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 29, 2017

What browser are you using?

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Nov 29, 2017

I'm using Chrome 62.0.3202.94 on Mac OS High Sierra

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 29, 2017

Do you see similar issues in, say Firefox?

Also, popovers and tooltips are similar in that they also use Popper.js. do you notice any memory issues with them?

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Nov 29, 2017

It does happen in Firefox.

And it doesn't happen with tooltips

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

Hmm, how about b-nav-item-dropdown? It uses the same shared code as b-dropdown with the exception that it doesn't instantiate a Popper.js instance.

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Nov 30, 2017

It can see the memory leak as well with b-nav-item-dropdown

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

The only issue I can think of is that <router-view> is re-using some of the inner component elements when it swaps the components (and hence not actually removing all of the component inner elements).

try adding a :key to your router view, to let vue-router know not to re-use any of the vdom:

<router-view :key="$route.fullPath"></router-view>
@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Nov 30, 2017

Adding :key="$route.fullPath" has no effects, the memory leak is still there :(

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

This is quite puzzling

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

The only other thing I can think of is the needed onTouchStart listeners, that get added to all the immediate children of <body> to get around an iOS bug. The listeners are removed when the dropdown is hidden, but if any of the body's immediate children change, then it might be possible that some get left behind.

Modal also uses this same onTouchStart listener concept to get around this iOS bug.

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Nov 30, 2017

This is quite puzzling

I know, I spent 1 week tracking down the memory leak, it is possible there is a bug on Vue itself, but when I opened a bug against them they ask me to bring it up here first, they will not take reproduction jsfiddle with a dependency to any third party libraries.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

PR #1416 may address this issue by making the popper.js instance reference non-reactive. If it doesn't then I am unsure of what the root cause may be.

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Nov 30, 2017

I will test it as soon we have a build with the fix. Thank you for looking into this promptly.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

Then again, I am not sure this will fix the issue, as dropdowns inside a navbar do not have a popper instance instantiated.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Dec 4, 2017

The latest dev branch has a few tweaks to dropdowns (in some of the event listeners), which may address the issue.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Dec 17, 2017

@tochoromero v1.4.0 is now available, which includes a few more "tweaks" to dropdowns, which might help address this issue.

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Dec 18, 2017

I can still reproduce it with the JSFiddle I provided :(

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Dec 21, 2017

What would the next step be, how can I help you? This memory leak makes the library unusable for us.

@tochoromero

This comment has been minimized.

Contributor

tochoromero commented Jan 24, 2018

I believe I have found the root of the memory leak.
The leak affects any component that uses the clickout mixin.
The leak was caused because removeEventListener was being called on the wrong element.

@pi0 pi0 closed this in #1557 Jan 25, 2018

pi0 added a commit that referenced this issue Jan 25, 2018

fix: call `removeEventListener` on the right element (#1557)
When removing the click listener, we were calling `removeEventListener` on the wrong element, and this was causing a memory leak. fixes #1391.

pi0 added a commit that referenced this issue Jan 25, 2018

fix: call `removeEventListener` on the right element (#1557)
When removing the click listener, we were calling `removeEventListener` on the wrong element, and this was causing a memory leak. fixes #1391.
@Chathula

This comment has been minimized.

Chathula commented Mar 21, 2018

modal also has a memory leak.. when modal content is big it takes lot of time to build.. (with nuxt)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment