SourceMap: Don't entertain garbage data (#586)

[am11]

It turned out to be std::bad_alloc exception. When source-map column is not updated node-x86 on Windows x64 throws.

Turned out, the generated_position.line was getting garbage value: std::numeric_limits<size_t>::max() (4294967295) and its variants: sometimes 4294967293 or 4294967292.

This fixes gh-586. 💡

/cc @mgreter, @xzyfer

[am11]

Exactly, this line was throwing exception.

[am11]

Added a small cleanup commit as well: 80010aa. (spent hours with this code so.. :D)

[coveralls]

Coverage increased (+0.01%) when pulling b6efc33 on am11:master into 7a381b8 on sass:master.

[xzyfer]

This is really great detective work!

[xzyfer]

It looks like this change is doing something very different than the original. Could you please elaborate on the reasoning here?

[am11]

It is appending ; N times (using the (second) overloaded ctor) to result and setting previous_generated_line = generated_line (exactly what while loop was previously doing).

[mgreter]

IMO this is not correct and should look like that instead (??):
result += std::string(generated_line - previous_generated_line, ';');

[coveralls]

Coverage increased (+0.01%) when pulling 96b2cb0 on am11:master into 7a381b8 on sass:master.

[mgreter]

@am11 you are the one to call this one solved on windows, because nobody else was able to reproduce. If no one else sees any regressions with this I'm all for merging this! Great work @am11, and yeah, I don't really see why this was really causing any issues in the first place! What do you say @xzyfer?

[am11]

Perhaps #586 (comment) has some gotcha?

I tested it, and it passed the test case under question. The issue I was getting with Windows x64 with Node.js x86 installation. Also, on x64 Windows with x64 node, it was taking a lot of time on running tests (sass/node-sass#497 (comment)). I haven't tested there, but this seems to be the problem.

[mgreter]

I just rebased my [WIP] branch to your latest changes. Lets see what travis ci has to say about it :) My local perl-libsass also compiles fine (strawberry perl on windows with mingw gcc).
[update] Travis-CI is still ok with your PR!

IMO source maps do need some better testing. I may include more sophisticated tests in perl-libsass (which I basically already use to test the C interface) for source maps, but it would be much better to have this tested directly by libsass. Maybe we should open anissue to track all source map related issues!??

[am11]

Having one generic [RFC]: Source Maps revamping (request for comments) issue would be great!

I think we can just trust LESS on that one. It has a lot of source-map related options:

CLI

C:\temp> node .\node_modules\less\bin\lessc --help
usage: lessc [option option=parameter ...] <source> [destination]

If source is set to `-' (dash or hyphen-minus), input is read from stdin.

options:
...
...
 --source-map[=FILENAME]  Outputs a v3 sourcemap to the filename (or output filename.map)
 --source-map-rootpath=X  adds this path onto the sourcemap filename and less file paths
 --source-map-basepath=X  Sets sourcemap base path, defaults to current working directory.
 --source-map-less-inline puts the less files into the map instead of referencing them
 --source-map-map-inline  puts the map (and any less files) into the output css file
 --source-map-url=URL     the complete url and filename put in the less file
...
...

We can take the inspirations from it's source-maps test and revamp our source-map code to match the details it captures. Even ruby-sass isn't even there yet (#324). The criteria is to capture mapping of as many tokens as possible to establish one-on-one link between source and generated output.

[mgreter]

Feel free to open a more genric issue about source maps!
I actually have done a lot in this area (mostly in perl for webmerge). I have seen a lot of different approaches to generate source maps. Some tools even decide to map every byte of the input to the output. But I fully ACK that it should mention every token that is relevant. Unfortunately there is no documentation what so ever how source maps were intially implemented in libsass (no blame to anyone)!

IMP my [WIP] branch actually implements these less options:

--source-map-less-inline puts the less files into the map instead of referencing them
 --source-map-map-inline  puts the map (and any less files) into the output css file

[mgreter]

I can now confirm the issue on node-sass and windows. Also that this PR "fixes" it.
I also got a local node-sass branch to compile and pass the full test suite.

[am11]

This is a great news! :)

Thanks. I can also confirm it.

Just one issue, (we don't have a test for it); in your node-sass root, run node and enter the interactive shell. Paste the following:

var stats = {};
var output = path.join(process.cwd(), './test/fixtures/simple/');

require("./lib/index").renderSync({
  file: './test/fixtures/simple/index.scss',
  outFile: path.join(output + 'build.css'),
  stats: stats,
  sourceMap: path.join(output, 'maps/build.css.map')
});

JSON.parse(stats.sourceMap);

You will notice that the mappings is empty.

[mgreter]

Closing this in favor of #591

[mgreter]

By the way I think I now understand what the problem was:

remove_line gets called too often (locigally removing more lines than added). This lead to an overflow on output_position.line. It then probably tried to add 18446744073709551615 semicolons to the source map. I will add the fallowing guard to the remove_line function. But this indicates another problem with source maps, as this should not happen.

  void SourceMap::remove_line()
  {
    if (output_position.line > 1) {
      output_position.line -= 1;
      output_position.column = 1;
    }
  }

[am11]

👍
I am working on appveyor thing on https://github.com/darrenkopp/sassc-win. Hopefully, it will get sorted out!

[am11]

I think we should fix the source of problem.

I first built a patch for it: checking if it lies between std::numeric_limits<size_t>::max() and std::numeric_limits<size_t>::max() - 100, but that would have considered a poor hack I guess. 😃

[mgreter]

The actual source of the problem is that remove_line is called to often in output_nested.cpp. I don't think that will be an easy one to fix! I've added the guard from above to the code and it at least does no longer crash (but it indicates that the line numbers in the mappings may not be reported correctly).