Sourcemap offset fix (improved)

[gthb]

Improved version of #268, working for my codebase.

[ben-ng]

+1 for getting this merged!

[ben-ng]

Bump for merge!

[kurttheviking]

👍

[mishoo]

It's a pretty big patch and I don't understand what it fixes. Can you detail?

[ben-ng]

First one fixes an incorrect offset when quotes are used around the keys of object literals.

Next few commits escape various characters that otherwise lead to broken source-maps.

[ben-ng]

Many of these were discovered minifying popular libraries like jquery and lodash, stepping through the maps (chrome debugger), and through trial and error, realizing what characters led to unusable source maps.

[mishoo]

So you're saying that for { "foo": ... }, with your fix, the mapping will point foo to the index of the f letter?

[gthb]

Yep, the last commit does that (instead of pointing into the middle of the token). Without it, the offset sequence was getting disrupted on multi-character adjustments, causing invalid source maps.

[mishoo]

Sorry, I'm still not convinced. Can you show an example that leads to a broken source map?

Also, I feel it's not correct in the example I gave to have the foo point to the index of f, because that's inside the string.

Your last commit fixes something introduced by an earlier commit in this pull request — but I'm asking why is this pull request necessary at all. How is UglifyJS currently broken?

[gthb]

Here's a boiled-down example:

jQuery(function ($,_) {
  // CSV/XLS exports based on posting CSV to server
  function csv_export ( sep, ext ) {
    var csv = get_CSV( sep, "\n", '"' );
    var fileformat = { ';':'scsv', ',':'csv', '\t':'tsv' }[ sep ] || 'csv';
    // ...
  }
});

When I uglify using the current tip of master:

uglifyjs --source-map uglify-303.map uglify-303.js > uglify-303.min.js

the result fails the source map validator: http://sourcemap-validator.herokuapp.com/validate?url=http%3A%2F%2Foneoff.datamarket.com%2Fuglify-303.min.js

and also Sentry fails to use it (so it is unlikely to be just a glitch in the validator).

When I uglify using a checkout of this pull request:

uglifyjs --source-map uglify-303.patched.map uglify-303.js > uglify-303.patched.min.js

the result passes the source map validator: http://sourcemap-validator.herokuapp.com/validate?url=http%3A%2F%2Foneoff.datamarket.com%2Fuglify-303.patched.min.js

and in the real-life scenario from which this was boiled down, Sentry successfully applies the source map.

[ben-ng]

My real-life example: browserify source maps don't work if the source files contain maps of lodash, jquery from uglify because the offsets are wrong (new lines when there are none, etc).

[mishoo]

Maybe the problem is that I pass a name for the object properties in the source map, when perhaps there shouldn't be any?

Again I must stress that it feels wrong for the mapping of an element to point inside a token (which in this case is a string). It's the first time I ever hear about Sentry and I couldn't care less about source map validation when the spec itself is pretty unclear, so please allow me to be cautious about “fixing” something I consider a non-issue… Still open to discuss though.

[gthb]

The last commit was indeed a fix to the first one; I've posted a squashed version of this pull request in case you prefer that.

[gthb]

Again I must stress that it feels wrong for the mapping of an element to point inside a token

Yeah, probably. (Though {foo: 3} is equivalent to {"foo": 3}, so that's some kind of argument for pointing to foo for whichever representation — but not a very strong argument.)

I really don't feel strongly either way — I didn't introduce that offset, I just corrected it from @ben-ng's original remainder/2, which corrupted the offset sequence on multi-character keys. And in any case, that offset is not a necessary part of the fix; I just posted #341 which is the same patch except without that remainder and offset stuff, and that works just as well.

... something I consider a non-issue

The spec is indeed unclear, but I am pretty sure this is an issue — because it doesn't just corrupt the offset to that one token, it corrupts the whole offset sequence for the remainder of the source map.

The proof is in the pudding of using the source map; for me that's Sentry, but you probably have some other consumer of source maps in mind. Does that tool handle http://oneoff.datamarket.com/uglify-303.min.js with its source map correctly? (You probably need a less stripped-down test case to verify that — you ought to be able to reproduce this problem in any codebase you're already testing source maps on, by just sticking an object literal in the code somewhere, containing keys with escaped characters in them, like the one in uglify-303.js)

[mishoo]

because it doesn't just corrupt the offset to that one token, it corrupts the whole offset sequence for the remainder of the source map.

That would be a serious bug indeed, but it doesn't seem to be the case. (it's actually not possible, by design).

Check this demo: http://lisperator.net/uglifyjs/#demo (use Chrome or FF, I'm not sure it works with other browsers)

Paste this code on the left side:

(function(){
    var something = {
        "foo": 1
    };
    var anotherthing = "test";
    console.log(something);
    console.log(anotherthing);
})();

then click "Uglify!". The left side will turn into a syntax-highlighting editor, and you'll get the minified code on the right side, which looks like this:

!function(){var o={foo:1},t="test"
console.log(o),console.log(t)}()

If you now click tokens on the right side, the left side editor will focus/highlight the corresponding token in the original source. Try clicking on:

• o — it'll highlight something on the left side.
• foo — it'll highlight "fo. Granted, this looks broken (but it's not)
• t or console or log — and it'll highlight the correct place on the left side.

This proves a couple of things:

1. although the mapping for "foo" might look broken, it's actually not. It points to the start of the string, which is the quote character. The fact that the highlighting seems wrong is because it highlights as many characters as the string has (3), but that's a minor issue and not actually a problem in the source map itself—it's rather a bug in the client application.
2. because the mappings for t (→ anotherthing) and console are correct, it means the offsets are not broken after the quoted object property. So what am I missing?

[gthb]

because it doesn't just corrupt the offset to that one token, it corrupts the whole offset sequence for the remainder of the source map.

That would be a serious bug indeed, but it doesn't seem to be the case. (it's actually not possible, by design).

Indeed I can't reproduce it now (the corruption of the whole subsequent offset sequence), so I can't claim with any conviction that it wasn't just me making a mistake in testing, earlier. Sorry about that!

I hadn't tried out the Chrome dev tools built-in support; that indeed parses my source maps without problems, built with or without this patch. So it's starting to look like the problem really is in sourcemap-validator.herokuapp.com and in raven-js (or Chrome itself) failing to pass on the source-mapped location to Sentry (see getsentry/sentry#1030).

[rvanvelzen]

I'm closing this for now. If the issue persists, please let us know!