Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Preserve name indices for duplicate names #72

Merged
merged 3 commits into from

3 participants

@evanw

ArraySet assumes the "names" array contains unique names, but this is not in the spec and the TypeScript compiler sometimes generates duplicate names. In this case, SourceMapConsumer crashes when it tries to access a name index out of bounds. This change allows duplicates in ArraySet and just returns the index of the first duplicate from indexOf(), which prevents the crash and allows the source maps to be loaded.

@evanw evanw Preserve name indices for duplicate names
Without this fix, ArraySet objects only added the first copy of a name
if the "names" array contained duplicates. This messed up indices into
the names array because the array was shorter than it should be. The
TypeScript compiler sometimes generates source maps with duplicate
names, causing a crash in SourceMapConsumer.
be06c70
@sokra

I think it's better to fix this behavior in the TypeScript compiler and change the spec in a way not allowing duplicate entries in names (and sources). This arrays should compress (deduplicate) references to names and sources in the mappings.

@fitzgen
Owner

I agree we should change the spec (see my email over there), however we should probably fix this now in a way that we can consume source maps that contain duplicates, but generate source maps that don't.

Haven't looked at the code yet, will get to it soon.

@fitzgen
Owner

@evanw we can't modify add to always push the name because we will get way too many duplicates. If a variable referenced 1000 times in a file being minified, and there is a mapping for each time it is referenced, we've now added 1000 items to the names array instead of one. But this is true for every variable that is re-used, not just this particular one. This is going to propagate through to the source maps that we generate, inflating their size. We can't do that.

I think we could add a boolean parameter to ArraySet.fromArray and ArraySet.prototype.add which allows duplicates. Then, SourceMapConsumer would use this new parameter while SourceMapGenerator would remain the same as it is now.

If anyone can think of a better solution, I'm all ears.

For a patch with this functionality to land, I would also like to see tests that:

  • we don't generate source maps with duplicates in the names array,

  • and we don't barf when consuming source maps that have duplicates in the names array

@sokra

Instead of accessing internals, it may be easier to operate on the generated source map:

var mapJson = map.toJSON();
mapJson.sources[1] = 'source1.js'
assert.doesNotThrow(function () {
  map = new SourceMapConsumer(mapJson);
});
@fitzgen fitzgen merged commit fc0104f into mozilla:master

1 check passed

Details default The Travis CI build passed
@fitzgen
Owner

@evanw thank you for the pull request! Sorry I missed that you updated it based on my feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 30, 2013
  1. @evanw

    Preserve name indices for duplicate names

    evanw authored
    Without this fix, ArraySet objects only added the first copy of a name
    if the "names" array contained duplicates. This messed up indices into
    the names array because the array was shorter than it should be. The
    TypeScript compiler sometimes generates source maps with duplicate
    names, causing a crash in SourceMapConsumer.
Commits on Jul 31, 2013
  1. @evanw
  2. @evanw
This page is out of date. Refresh to see the latest.
View
19 lib/source-map/array-set.js
@@ -25,10 +25,10 @@ define(function (require, exports, module) {
/**
* Static method for creating ArraySet instances from an existing array.
*/
- ArraySet.fromArray = function ArraySet_fromArray(aArray) {
+ ArraySet.fromArray = function ArraySet_fromArray(aArray, allowDuplicates) {
var set = new ArraySet();
for (var i = 0, len = aArray.length; i < len; i++) {
- set.add(aArray[i]);
+ set.add(aArray[i], allowDuplicates);
}
return set;
};
@@ -38,14 +38,15 @@ define(function (require, exports, module) {
*
* @param String aStr
*/
- ArraySet.prototype.add = function ArraySet_add(aStr) {
- if (this.has(aStr)) {
- // Already a member; nothing to do.
- return;
- }
+ ArraySet.prototype.add = function ArraySet_add(aStr, allowDuplicates) {
+ var isDuplicate = this.has(aStr);
var idx = this._array.length;
- this._array.push(aStr);
- this._set[util.toSetString(aStr)] = idx;
+ if (!isDuplicate || allowDuplicates) {
+ this._array.push(aStr);
+ }
+ if (!isDuplicate) {
+ this._set[util.toSetString(aStr)] = idx;
+ }
};
/**
View
7 lib/source-map/source-map-consumer.js
@@ -62,8 +62,11 @@ define(function (require, exports, module) {
throw new Error('Unsupported version: ' + version);
}
- this._names = ArraySet.fromArray(names);
- this._sources = ArraySet.fromArray(sources);
+ // Pass "true" below to allow duplicate names and sources. While source maps
+ // are intended to be compressed and deduplicated, the TypeScript compiler
+ // sometimes generates source maps with duplicates in them.
+ this._names = ArraySet.fromArray(names, true);
+ this._sources = ArraySet.fromArray(sources, true);
this.sourceRoot = sourceRoot;
this.sourcesContent = sourcesContent;
this.file = file;
View
33 test/source-map/test-array-set.js
@@ -68,4 +68,37 @@ define(function (require, exports, module) {
assert.strictEqual(set.indexOf('__proto__'), 0);
};
+ exports['test .fromArray() with duplicates'] = function (assert, util) {
+ var set = ArraySet.fromArray(['foo', 'foo']);
+ assert.ok(set.has('foo'));
+ assert.strictEqual(set.at(0), 'foo');
+ assert.strictEqual(set.indexOf('foo'), 0);
+ assert.strictEqual(set.toArray().length, 1);
+
+ set = ArraySet.fromArray(['foo', 'foo'], true);
+ assert.ok(set.has('foo'));
+ assert.strictEqual(set.at(0), 'foo');
+ assert.strictEqual(set.at(1), 'foo');
+ assert.strictEqual(set.indexOf('foo'), 0);
+ assert.strictEqual(set.toArray().length, 2);
+ };
+
+ exports['test .add() with duplicates'] = function (assert, util) {
+ var set = new ArraySet();
+ set.add('foo');
+
+ set.add('foo');
+ assert.ok(set.has('foo'));
+ assert.strictEqual(set.at(0), 'foo');
+ assert.strictEqual(set.indexOf('foo'), 0);
+ assert.strictEqual(set.toArray().length, 1);
+
+ set.add('foo', true);
+ assert.ok(set.has('foo'));
+ assert.strictEqual(set.at(0), 'foo');
+ assert.strictEqual(set.at(1), 'foo');
+ assert.strictEqual(set.indexOf('foo'), 0);
+ assert.strictEqual(set.toArray().length, 2);
+ };
+
});
View
70 test/source-map/test-source-map-consumer.js
@@ -318,4 +318,74 @@ define(function (require, exports, module) {
assert.equal(map.sourceContentFor(s), "foo");
};
+ exports['test github issue #72, duplicate sources'] = function (assert, util) {
+ var map = new SourceMapConsumer({
+ "version": 3,
+ "file": "foo.js",
+ "sources": ["source1.js", "source1.js", "source3.js"],
+ "names": [],
+ "mappings": ";EAAC;;IAEE;;MEEE",
+ "sourceRoot": "http://example.com"
+ });
+
+ var pos = map.originalPositionFor({
+ line: 2,
+ column: 2
+ });
+ assert.equal(pos.source, 'http://example.com/source1.js');
+ assert.equal(pos.line, 1);
+ assert.equal(pos.column, 1);
+
+ var pos = map.originalPositionFor({
+ line: 4,
+ column: 4
+ });
+ assert.equal(pos.source, 'http://example.com/source1.js');
+ assert.equal(pos.line, 3);
+ assert.equal(pos.column, 3);
+
+ var pos = map.originalPositionFor({
+ line: 6,
+ column: 6
+ });
+ assert.equal(pos.source, 'http://example.com/source3.js');
+ assert.equal(pos.line, 5);
+ assert.equal(pos.column, 5);
+ };
+
+ exports['test github issue #72, duplicate names'] = function (assert, util) {
+ var map = new SourceMapConsumer({
+ "version": 3,
+ "file": "foo.js",
+ "sources": ["source.js"],
+ "names": ["name1", "name1", "name3"],
+ "mappings": ";EAACA;;IAEEA;;MAEEE",
+ "sourceRoot": "http://example.com"
+ });
+
+ var pos = map.originalPositionFor({
+ line: 2,
+ column: 2
+ });
+ assert.equal(pos.name, 'name1');
+ assert.equal(pos.line, 1);
+ assert.equal(pos.column, 1);
+
+ var pos = map.originalPositionFor({
+ line: 4,
+ column: 4
+ });
+ assert.equal(pos.name, 'name1');
+ assert.equal(pos.line, 3);
+ assert.equal(pos.column, 3);
+
+ var pos = map.originalPositionFor({
+ line: 6,
+ column: 6
+ });
+ assert.equal(pos.name, 'name3');
+ assert.equal(pos.line, 5);
+ assert.equal(pos.column, 5);
+ };
+
});
View
26 test/source-map/test-source-map-generator.js
@@ -388,4 +388,30 @@ define(function (require, exports, module) {
util.assertEqualMaps(assert, map1.toJSON(), map2.toJSON());
};
+
+ exports['test github issue #72, check for duplicate names or sources'] = function (assert, util) {
+ var map = new SourceMapGenerator({
+ file: 'test.js'
+ });
+ map.addMapping({
+ generated: { line: 1, column: 1 },
+ original: { line: 2, column: 2 },
+ source: 'a.js',
+ name: 'foo'
+ });
+ map.addMapping({
+ generated: { line: 3, column: 3 },
+ original: { line: 4, column: 4 },
+ source: 'a.js',
+ name: 'foo'
+ });
+ util.assertEqualMaps(assert, map.toJSON(), {
+ version: 3,
+ file: 'test.js',
+ sources: ['a.js'],
+ names: ['foo'],
+ mappings: 'CACEA;;GAEEA'
+ });
+ };
+
});
Something went wrong with that request. Please try again.