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

[BUGFIX release] don’t leak last destroyedComponents #14987

Merged
merged 1 commit into from
Mar 8, 2017
Merged

Conversation

stefanpenner
Copy link
Member

Ensure we don’t retain the previous set of destroyed components.


Although commit did reset the destroyedComponents array, it does seem strange to retain the last set of destroyedComponents until the next render.

Ensure we don’t retain the previous set of destroyed components.
@chadhietala chadhietala merged commit dacfaa5 into master Mar 8, 2017
// components queued for destruction must be destroyed before firing
// `didCreate` to prevent errors when removing and adding a component
// with the same name (would throw an error when added to view registry)
for (let i = 0; i < this.destroyedComponents.length; i++) {
this.destroyedComponents[i].destroy();
for (let i = 0; i < destroyedComponents.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you want to cache length here?

Copy link
Member Author

Choose a reason for hiding this comment

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

tell me more

Copy link
Contributor

Choose a reason for hiding this comment

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

lol stop messing with the old guy...

}

commit() {
let destroyedComponents = this.destroyedComponents;
this.destroyedComponents = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do this assignment here. Why not just reassign to an empty array after the subsequent loop? More of a stylistic thing I think.

Copy link
Member Author

@stefanpenner stefanpenner Mar 8, 2017

Choose a reason for hiding this comment

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

if the loop fails (destroy could throw), we still want to release the contents of the original destroyedComponents. So this way, we ensure they are released before we invoke any further code.


this.registerComponent('foo-bar', { ComponentClass: FooBarComponent, template: 'hello' });

this.render('{{#if switch}}{{#foo-bar}}{{foo-bar}}{{/foo-bar}}{{/if}}', { switch: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

foobar inception...

Copy link
Member Author

@stefanpenner stefanpenner Mar 8, 2017

Choose a reason for hiding this comment

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

@stefanpenner stefanpenner deleted the mem-leak branch March 8, 2017 03:55
@krisselden
Copy link
Contributor

Can you set the length to 0? It will release it, is there a reason to make a new array?

@stefanpenner
Copy link
Member Author

Can you set the length to 0? It will release it, is there a reason to make a new array?

I could, in this case I didn't believe the cost of a new array mattered so i left it closer to how it was originally implemented.

@rwjblue
Copy link
Member

rwjblue commented Mar 8, 2017

Good catch @stefanpenner!

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.

5 participants