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 memory leak and other improvements #925

Merged
merged 4 commits into from
Mar 8, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Mar 7, 2015

This contains one breaking change for implementors (//CC @am11):

We made an error with sass_make_data_context(char* data). We made a copy of that string pointer, which we should not have be doing, since the API indicates that libsass will take ownership of that memory. You may either have leaked memory unintentionally (which would then be fixed by this commit) or you will need to create a copy before passing it to libsass (or tell your GC that it is no longer in control of that memory).

struct Sass_Data_Context* data_ctx = sass_make_data_context(strdup(input_string));

Otherwise it fixes a few memory leaks in cssize.cpp and removes the copy_c_str sources in favor of another small function in util. Addresses #869

@mgreter mgreter added this to the 3.2 milestone Mar 7, 2015
@mgreter mgreter self-assigned this Mar 7, 2015
@mgreter mgreter force-pushed the bugfix/memory-leak branch 3 times, most recently from e45ca94 to c90ab53 Compare March 7, 2015 01:32
@mgreter
Copy link
Contributor Author

mgreter commented Mar 7, 2015

Fixed some left over issues with interpolate parsing:
Added some more spec tests to sass/sass-spec#270

@am11
Copy link
Contributor

am11 commented Mar 7, 2015

👍
I was not sure what to make of this: #869 (comment). Perhaps both fixes will play role in addressing the flaky behavior of uv_closing in higher versions libuv (that io.js uses).

The higher the version is, more sensitive these libraries become (from GC perspective).. (-8

mgreter added 4 commits March 8, 2015 04:21
Plus a few other minor improvements
Addresses sass#869

This is a breaking change since implementers need to
make a copy of the string if they are not able to pass
the complete memory ownership to libsass.
@mgreter mgreter force-pushed the bugfix/memory-leak branch from 9d07a48 to 5ce9d23 Compare March 8, 2015 03:22
mgreter added a commit that referenced this pull request Mar 8, 2015
Fix memory leak and other improvements
@mgreter mgreter merged commit 5757248 into sass:master Mar 8, 2015
@mgreter mgreter mentioned this pull request Mar 10, 2015
@am11
Copy link
Contributor

am11 commented Mar 15, 2015

(disclaimer I haven't debugged libsass latest code yet)

@mgreter, is it also applicable to file_context?

After upgrading to 3.2.0-beta.1, if I dispose input_string for data context, it starts failing tests, which makes perfect sense, because we are not making copy before sending to libsass. So I removed the free statement to avoid double free and it worked! 🎉

Next, I also removed the free statement for input_path in file context case and tested, it was working. But then I added back the free statement for this input path, it still does not break or fail anything (still working fine), whereas I was expecting it to throw as this is a double free case! So, if libsass is not owning input_path memory for file context, is this by design?

@mgreter
Copy link
Contributor Author

mgreter commented Mar 15, 2015

I hope the following explains your question

struct Sass_File_Context* sass_make_file_context (const char* input_path);
struct Sass_Data_Context* sass_make_data_context (char* source_string);

So we do make a copy of the path, since we expect it to always be a pretty cheap operation.

@am11
Copy link
Contributor

am11 commented Mar 15, 2015

@mgreter, thanks for the comment.
I will then keep the free() statement for input_path.

There are some source_map path related issues I am facing with 3.2.0-beta.1. Some tests are failing because file member (which points to CSS file) is now returning an absolute path, when we are send the abs. path from node-sass. The expected behavior (the one that libsass v3.1.0 possesses and we shipped with node-sass v3.0.0-pre) is to get the file path resolved w.r.t location of outfile. I couldn't find anything related to paths in changelog. Should I open an issue for it? Or Gitter?

am11 added a commit to am11/node-sass that referenced this pull request Mar 15, 2015
* Updates sass-spec to corresponding commit.
* Prevents source_string from freeing in
  data context case in binding code.
  * Corresponds to sass/libsass#925.
am11 added a commit to am11/node-sass that referenced this pull request Mar 18, 2015
* Updates sass-spec to corresponding commit.
* Prevents source_string from freeing in
  data context case in binding code.
  * Corresponds to sass/libsass#925.
am11 added a commit to am11/node-sass that referenced this pull request Mar 18, 2015
* Updates sass-spec to corresponding commit.
* Prevents source_string from freeing in
  data context case in binding code.
  * Corresponds to sass/libsass#925.
am11 added a commit to am11/node-sass that referenced this pull request Mar 18, 2015
* Updates sass-spec to corresponding commit.
* Prevents source_string from freeing in
  data context case in binding code.
  * Corresponds to sass/libsass#925.
am11 added a commit to am11/node-sass that referenced this pull request Mar 18, 2015
* Updates sass-spec to corresponding commit.
* Prevents source_string from freeing in
  data context case in binding code.
  * Corresponds to sass/libsass#925.
am11 added a commit to am11/node-sass that referenced this pull request Mar 18, 2015
* Updates sass-spec to corresponding commit.
* Prevents source_string from freeing in
  data context case in binding code.
  * Corresponds to sass/libsass#925.
am11 added a commit to am11/node-sass that referenced this pull request Mar 18, 2015
* Updates sass-spec to corresponding commit.
* Prevents source_string from freeing in
  data context case in binding code.
  * Corresponds to sass/libsass#925.
am11 added a commit to am11/node-sass that referenced this pull request Mar 18, 2015
* Updates sass-spec to corresponding commit.
* Prevents source_string from freeing in
  data context case in binding code.
  * Corresponds to sass/libsass#925.
@mgreter mgreter deleted the bugfix/memory-leak branch April 6, 2015 17:13
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.

2 participants