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 a few minor memory leaks #2400

Merged
merged 2 commits into from May 22, 2017

Conversation

@mgreter
Copy link
Contributor

commented May 22, 2017

With updates for ISSUE_TEMPLATE.md and CONTRIBUTING.md

mgreter added 2 commits May 22, 2017
Media_Blocks are prone to have circular references.
Copy could leak memory if it does not get picked up.
Looks like we are able to reset block reference for copy
Good as it will ensure a low memory overhead for this fix
So this is a cheap solution with a minimal price
@mgreter mgreter merged commit ad48261 into sass:master May 22, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 86.024%
Details
@glebm

This comment has been minimized.

Copy link
Contributor

commented on src/parser.cpp in f115eb8 Nov 19, 2018

@mgreter @xzyfer How could this change fix a memory leak? selector_schema is always returned and never re-assigned.

This comment has been minimized.

Copy link
Contributor Author

replied Nov 19, 2018

Normally that may can happen if there is an error thrown/catched before the actual return.
Somewhat documented in https://github.com/sass/libsass/blob/master/docs/dev-ast-memory.md

This comment has been minimized.

Copy link
Contributor Author

replied Nov 19, 2018

Also the method wants to return a reference counted Selector_Schema_Obj and you have been actually returning a pointer. Not sure it has anything to do with it, not remembering this particular edge case. But I know I had to cut some corners at some points to get it working as intended. I know its pretty fragile, but it was the best I could accomplish. Also note that I use https://github.com/sass/libsass/blob/master/script/test-leaks.pl to test for leaks via valigrind.

This comment has been minimized.

Copy link
Contributor

replied Nov 19, 2018

The SharedImpl has an implicit one-arg constructor from T*, so that pointer is implicitly converted.

This comment has been minimized.

Copy link
Contributor Author

replied Nov 19, 2018

Yeah, true, detach as before was also just a pointer, so no idea why it would leak with your change ...

This comment has been minimized.

Copy link
Contributor Author

replied Nov 19, 2018

Sorry, just realized your commenting on an old merged commit of myself. I will need to check further why I did this ...

This comment has been minimized.

Copy link
Contributor

replied Nov 19, 2018

This commit didn't leak, I was simply looking through the commit history.

See the discussion #2738

This comment has been minimized.

Copy link
Contributor Author

replied Nov 19, 2018

Sorry for not catching up too quickly, your conclusions seem to be correct!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.