Constify Sass_Data_Context::source_string

[craigbarnes]

#51, round 2

[craigbarnes]

The failed test seems to be caused by an unrelated change in the sass-spec repo.

[xzyfer]

Thanks @craigbarnes . You're correct the failure can be ignored for now.

Merge this at your discretion @mgreter

[mgreter]

It is actually purposely not constant, since the memory you pass as source_string is not copied by libsass; the ownership transfers to libsass, while we do make copies of const chars*. To be honest I'm still confused where to make it const or not, but @QuLogic always gave good advices in this direction!?

[craigbarnes]

the ownership transfers to libsass

libsass currently doesn't seem to free source_string. Unless I'm missing something?

On a related note, running sassc via valgrind seems to show several memory leaks in the library.

[QuLogic]

libsass currently doesn't seem to free source_string. Unless I'm missing something?

Yes, but line 471 is ctx->source_string = source_string;, i.e., the string is not duplicated. Which means it should continue to exist once the function exits. This is usually signified by leaving off the const (not a hard rule, though.) But you are correct that libsass should probably be freeing it if it owns it.

[craigbarnes]

The current contract of the API seems to be:

• libsass reserves the right to mutate source_string (even though it actually doesn't).
• libsass leaves it up to the user to guess whether or not to free source_string.

IMHO, libsass should:

• Promise to not mutate source_string by declaring it const, thereby allowing garbage-collected languages to pass a pointer to the GC'd heap instead of making a full copy.
• Leave the client responsible for freeing/collecting the memory and consider Sass_Data_Context::source_string a "borrowed" pointer to it.
• Mention ownership in the API docs, so that knowledge of library internals isn't required for correct usage.

[mgreter]

You seem to have a point here, but IMHO we want to go in the other direction.
The one remaining confusion is IMO fixed with this commit: 496f4f9
Unfortunately this is a breaking change for implementers (//CC @am11)

You should always be able to pass libsass the whole memory and libsass will take care to free the memory. We currently keep copies of the pointers in various locations, but finally all these pointers should also be in Context::sources, which will be freed by libsass. So in case your implementation does not allow you to let libsass "overtake" that memory, you have to make a copy yourself before passing it to libsass (the costs should be pretty minimal, what's 1MB of RAM?). We do it that way, because we a) want to allow implementors to only allocate the memory once and pass it to libsass as is and b) we do not trust the outside world with the lifespan of that data (we want to make sure we can keep it around as long as we need, so that libsass is in control of the memory).

I hope this makes sense. If libsass doesn't free the memory, please open a new bug about that. Although my latest memory tests indicate that everything is clear in that direction!

BTW. this was/is also reflected in the C-API:

sass_make_data_context(char* source_string)

[am11]

Thanks for the heads-up @mgreter! 👍

Is it a good idea that I backport this change to v3.1.0, since node-sass vNext might go without libsass v3.2.0?

This might fix the intermittent hung-ups by libuv and SIGSEGVs which we sometimes encounter with bleeding edge version of io.js.