Inconsistent import identifiers + memory leak

[saper]

I just spent some time analyzing sass/node-sass#894 (thanks @jameslnewell) which ends up to be a consequence of problems reported also as #1040 #1041 #1035 (comment)

Testing done using:

• saper@ce80c54 which is https://github.com/sass/libsass/tree/3.2.2 plus some ad-hoc debug logging
• sass/node-sass@2fce303

Here are the results of my tests:

https://gist.github.com/saper/5263902b8446431ae136

• issue-894-two-strings demonstrates how two string results from the importer override each other;
• issue-894-two-files demonstrates how two files get included under the same import name;
• issue-894-one-file-one-string demonstrates how a string-based import cannot override the one from a file;

libsass is not consistent about what is being put into style_sheets - sometimes it is a short import name, sometimes file base name.

There is also a memory leak in context.cpp#L330 - in case of two-strings.js an AST compiled as a result of the first import gets overwritten when we are storing a second AST under the same name foobar issue-894-two-strings.out#L14.

It should be decided what do the short import names really mean in the context of custom importers (that can return random content every time they are called).

Looks to me that add_file and add_string need some love. The cleanup there can also help resolve some memory allocation issues hiding in the code (for example only internally allocated strings should end up on the sources list, and not those provided by the caller).

Generally I think there too many single-purpose little lists, may be it would be better to have something similar to the parser input queue (with items similar to today's Sass_Queued) that contains a directory of all sources:

• source import name
• backing file name (if any)
• text buffer
• buffer heap allocated by libsass/not
• information to assist source map creation and error reporting.

those source material could be related to the output information:

• parsed AST
• final output destination type / filename
• final CSS output
• source map

[drewwells]

Would it help if that issue was broken up into different tasks? There seems to be a lot of different bugs filed under one all encompassing issue. This might help carve off some of the bugs into actionable items.

For instance, demonstrates how two string results from the importer override each other;. That sounds like an isolated problem from some of the others mentioned.

[saper]

I think this is one issue in one part of code, and this code needs to be torn apart a bit. It is kind of orthogonal to the problems reported.

[mgreter]

The memory leaks should be fixed by #1330.
AFAIR there were no changes on the C-API needed for this!

[saper]

will try to provide test case for the leak

[saper]

To clarify my intentions about better C-API I have opened #1368 for discussion.

[mgreter]

@saper can you give an update what real issues are still relevant at the time beeing? Thanks!

[mgreter]

Any updates here?

[mgreter]

Closing this due to inactivity, problems should be resolved. Re-open if not!