Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix some minor memory leaks with context options #2213

Merged
merged 3 commits into from
Oct 20, 2016

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Oct 19, 2016

Note: Sass_Context is also a Sass_Options, therefore one only needs to call either sass_delete_compiler or sass_delete_options. They're created via sass_make_TYPE_compiler or sass_make_options respectively. Therefore if you use sass_make_options you probably must add the corresponding sass_delete_options now (this ie. is the case with sassc).

@mgreter mgreter added this to the 3.3.7 milestone Oct 19, 2016
mgreter added a commit to mgreter/sassc that referenced this pull request Oct 19, 2016
@mgreter
Copy link
Contributor Author

mgreter commented Oct 19, 2016

# valgrind --leak-check=full sassc/bin/sassc sassc/foo.scss
==3514== Memcheck, a memory error detector
==3514== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==3514== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==3514== Command: sassc/bin/sassc sassc/foo.scss
==3514==
foo-bar {
  baz: baz; }
==3514==
==3514== HEAP SUMMARY:
==3514==     in use at exit: 0 bytes in 0 blocks
==3514==   total heap usage: 2,403 allocs, 2,403 frees, 166,497 bytes allocated
==3514==
==3514== All heap blocks were freed -- no leaks are possible
==3514==
==3514== For counts of detected and suppressed errors, rerun with: -v
==3514== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@mgreter mgreter self-assigned this Oct 19, 2016
@mgreter
Copy link
Contributor Author

mgreter commented Oct 19, 2016

Currently running valgrind over all spec tests (could take a while):

use strict;
use warnings;

use IPC::Run3;
use File::Find::Rule;

my $root = "libsass/sass-spec/spec";
my @files = File::Find::Rule->file()
            ->name('input.scss')->in($root);

foreach my $file (@files) {
  my $cmd = sprintf('libsass/sassc/bin/sassc %s', $file);
  my $check = sprintf('valgrind --leak-check=yes %s', $cmd);
  run3($check, undef, \ my $out, \ my $err);
  next if $err =~ m/in use at exit: 0 bytes in 0 blocks/;
  warn "got a leak $? !!\n", $err, "#" x 80, "\n";
}

@mgreter mgreter force-pushed the bugfix/ctx-options-leak branch from e2f2c03 to 716255b Compare October 19, 2016 22:14
@mgreter mgreter merged commit 1661755 into sass:master Oct 20, 2016
@mgreter
Copy link
Contributor Author

mgreter commented Oct 20, 2016

No further leaks detected via valgrind (ran all spec tests via the script from above). Doesn't say there still exist some more leaks on the C-API level or anywhere else for that matter 🐛

@nschonni
Copy link
Collaborator

Do you think it's worth adding valgrind to the CI?

@mgreter
Copy link
Contributor Author

mgreter commented Oct 20, 2016

IMHO to slow for CI, probably even takes too long for CI to complete.

@xzyfer xzyfer modified the milestones: 3.3.7, 3.4 Oct 20, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants