From 0a71129dc12af0d18c1188aebccb8310a128dbf7 Mon Sep 17 00:00:00 2001 From: Ger Hobbelt Date: Tue, 18 Jun 2019 19:55:02 +0200 Subject: [PATCH 1/6] fix crash in url util function due to undefined root value: `sourceRoot` is checked everywhere using `!= null` except in 1(one) spot, which caused a crash in my test set, while debugging indexed map issues with the origin API. --- lib/source-map-generator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/source-map-generator.js b/lib/source-map-generator.js index 8111e061..5f51a6c5 100644 --- a/lib/source-map-generator.js +++ b/lib/source-map-generator.js @@ -71,7 +71,7 @@ class SourceMapGenerator { }); aSourceMapConsumer.sources.forEach(function(sourceFile) { let sourceRelative = sourceFile; - if (sourceRoot !== null) { + if (sourceRoot != null) { sourceRelative = util.relative(sourceRoot, sourceFile); } From a7ebc2f9fe8537369926f6aab9b77062dcf4b8d9 Mon Sep 17 00:00:00 2001 From: Ger Hobbelt Date: Tue, 18 Jun 2019 20:16:20 +0200 Subject: [PATCH 2/6] when `sourceContentFor(aSource, nullOnMissing=false)` shouldn't this situation also throw an exception? In the other situation, where this API might also produce a NULL return, an exception *is* thrown instead, so this looks to me like the more consistent behaviour. --- lib/source-map-consumer.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/source-map-consumer.js b/lib/source-map-consumer.js index 9b68e393..63069a1c 100644 --- a/lib/source-map-consumer.js +++ b/lib/source-map-consumer.js @@ -527,7 +527,11 @@ class BasicSourceMapConsumer extends SourceMapConsumer { */ sourceContentFor(aSource, nullOnMissing) { if (!this.sourcesContent) { - return null; + if (nullOnMissing) { + return null; + } + + throw new Error('"' + aSource + '" is not in the SourceMap.'); } const index = this._findSourceIndex(aSource); From 88efe6141cbf73c11170eba9e8696c7931554f6f Mon Sep 17 00:00:00 2001 From: Ger Hobbelt Date: Tue, 18 Jun 2019 20:27:38 +0200 Subject: [PATCH 3/6] fixing `originalPositionFor()` API for indexed maps + added test adapted from the 0.6.x dev tree: it turned out that due an off-by-1 error in the column value, half of the mapping nodes weren't hit by the `originalPositionFor()` internal search code, while it *should* find those. The added tests are augmented with extra logging for diagnosis: run those tests with & without the given patch to observe the bug & change. --- lib/source-map-consumer.js | 3 +- test/test-source-map-consumer.js | 107 ++++++++++++++++++++++++++++++- test/util.js | 23 +++++++ 3 files changed, 128 insertions(+), 5 deletions(-) diff --git a/lib/source-map-consumer.js b/lib/source-map-consumer.js index 9b68e393..e87373e0 100644 --- a/lib/source-map-consumer.js +++ b/lib/source-map-consumer.js @@ -785,8 +785,7 @@ class IndexedSourceMapConsumer extends SourceMapConsumer { return cmp; } - return (aNeedle.generatedColumn - - section.generatedOffset.generatedColumn); + return aNeedle.generatedColumn - (section.generatedOffset.generatedColumn - 1); }); const section = this._sections[sectionIndex]; diff --git a/test/test-source-map-consumer.js b/test/test-source-map-consumer.js index 3d86b39e..8f327fa6 100644 --- a/test/test-source-map-consumer.js +++ b/test/test-source-map-consumer.js @@ -95,7 +95,6 @@ exports["test that the source root is reflected in a mapping's source field"] = assert.equal(mapping.source, "/the/root/one.js"); map.destroy(); - map = await new SourceMapConsumer(util.testMapNoSourceRoot); mapping = map.originalPositionFor({ @@ -111,7 +110,6 @@ exports["test that the source root is reflected in a mapping's source field"] = assert.equal(mapping.source, "one.js"); map.destroy(); - map = await new SourceMapConsumer(util.testMapEmptySourceRoot); mapping = map.originalPositionFor({ @@ -583,7 +581,6 @@ exports["test sourceRoot + generatedPositionFor"] = async function(assert) { source: "bang.coffee" }); - map = await new SourceMapConsumer(map.toString(), "http://example.com/"); // Should handle without sourceRoot. @@ -692,6 +689,110 @@ exports["test index map + generatedPositionFor"] = async function(assert) { map.destroy(); }; +exports["test index map + originalPositionFor"] = async function(assert) { + console.warn("================================="); + var map = await new SourceMapConsumer( + util.indexedTestMapWithMappingsAtSectionStart + ); + assert.ok(map instanceof IndexedSourceMapConsumer); + + map.eachMapping(function(m) { + console.warn("mapping:", m); + }); + + map.sources.forEach(function(s, d) { + console.warn("section:", d, s); + }); + + for (let l = 0; l <= 2; l++) { + for (let c = 0; c < 6; c++) { + let pos = map.originalPositionFor({ + line: l, + column: c + }); + console.warn("originalPositionFor:", { + line: l, + column: c, + rv: pos + }); + } + } + + var gen = SourceMapGenerator.fromSourceMap(map); + var smStr = JSON.stringify(gen.toJSON(), null, 2); + console.warn("generated sourcemap:", smStr); + map.destroy(); + + console.warn("-----------------------------------"); + map = await new SourceMapConsumer(smStr); + assert.ok(!(map instanceof IndexedSourceMapConsumer)); + + map.eachMapping(function(m) { + console.warn("mapping:", m); + }); + + map.sources.forEach(function(s, d) { + console.warn("section:", d, s); + }); + + for (let l = 1; l <= 2; l++) { + for (let c = 0; c < 6; c++) { + let pos = map.originalPositionFor({ + line: l, + column: c + }); + console.warn("originalPositionFor:", { + line: l, + column: c, + rv: pos + }); + } + } + + + var pos = map.originalPositionFor({ + line: 1, + column: 0 + }); + + // assert.equal(pos.line, 1); + // assert.equal(pos.column, 0); + // assert.equal(pos.source, "foo.js"); + // assert.equal(pos.name, "first"); + + pos = map.originalPositionFor({ + line: 1, + column: 1 + }); + + assert.equal(pos.line, 2); + assert.equal(pos.column, 1); + assert.equal(pos.source, "bar.js"); + assert.equal(pos.name, "second"); + + pos = map.originalPositionFor({ + line: 1, + column: 2 + }); + + // assert.equal(pos.line, 1); + // assert.equal(pos.column, 0); + // assert.equal(pos.source, "baz.js"); + // assert.equal(pos.name, "third"); + + pos = map.originalPositionFor({ + line: 1, + column: 3 + }); + + assert.equal(pos.line, 2); + assert.equal(pos.column, 1); + assert.equal(pos.source, "quux.js"); + assert.equal(pos.name, "fourth"); + + map.destroy(); +}; + exports["test allGeneratedPositionsFor for line"] = async function(assert) { let map = new SourceMapGenerator({ file: "generated.js" diff --git a/test/util.js b/test/util.js index 3f2c25c9..93b59d4d 100644 --- a/test/util.js +++ b/test/util.js @@ -265,6 +265,29 @@ exports.indexedTestMapColumnOffset = { } ] }; +exports.indexedTestMapWithMappingsAtSectionStart = { + version: 3, + sections: [ + { + offset: { line: 0, column: 0 }, + map: { + version: 3, + names: ["first", "second"], + sources: ["foo.js", "bar.js"], + mappings: "AAAAA,CCCCC" + } + }, + { + offset: { line: 0, column: 2 }, + map: { + version: 3, + names: ["third", "fourth"], + sources: ["baz.js", "quux.js"], + mappings: "AAAAA,CCCCC" + } + } + ] +}; exports.testMapWithSourcesContent = { version: 3, file: "min.js", From b974ff23309d316ffda169f11b684e9cc7468af0 Mon Sep 17 00:00:00 2001 From: Ger Hobbelt Date: Tue, 18 Jun 2019 20:37:54 +0200 Subject: [PATCH 4/6] allow the sourceMapGenerator code to also enjoy the `nullOrMissing` argument/option for the `consumer.sourceContentFor()` API. --- lib/source-map-generator.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/source-map-generator.js b/lib/source-map-generator.js index 8111e061..6851701b 100644 --- a/lib/source-map-generator.js +++ b/lib/source-map-generator.js @@ -79,7 +79,7 @@ class SourceMapGenerator { generator._sources.add(sourceRelative); } - const content = aSourceMapConsumer.sourceContentFor(sourceFile); + const content = aSourceMapConsumer.sourceContentFor(sourceFile, true); if (content != null) { generator.setSourceContent(sourceFile, content); } @@ -238,7 +238,7 @@ class SourceMapGenerator { // Copy sourcesContents of applied map. aSourceMapConsumer.sources.forEach(function(srcFile) { - const content = aSourceMapConsumer.sourceContentFor(srcFile); + const content = aSourceMapConsumer.sourceContentFor(srcFile, true); if (content != null) { if (aSourceMapPath != null) { srcFile = util.join(aSourceMapPath, srcFile); From d2c33d1c9a38f69e4448bdae4e9448cb00c7a3eb Mon Sep 17 00:00:00 2001 From: Ger Hobbelt Date: Tue, 18 Jun 2019 20:48:15 +0200 Subject: [PATCH 5/6] also use the `nullOrNothing` option/argument in the lib/source-node.js code to restore intended behaviour. All tests pass. --- lib/source-node.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/source-node.js b/lib/source-node.js index 8a7a157e..8f92c0a1 100644 --- a/lib/source-node.js +++ b/lib/source-node.js @@ -137,7 +137,7 @@ class SourceNode { // Copy sourcesContent into SourceNode aSourceMapConsumer.sources.forEach(function(sourceFile) { - const content = aSourceMapConsumer.sourceContentFor(sourceFile); + const content = aSourceMapConsumer.sourceContentFor(sourceFile, true); if (content != null) { if (aRelativePath != null) { sourceFile = util.join(aRelativePath, sourceFile); From cb653e459c8a75c91c829c057c833f466b88aabe Mon Sep 17 00:00:00 2001 From: Ger Hobbelt Date: Tue, 18 Jun 2019 20:53:51 +0200 Subject: [PATCH 6/6] remove console diagnostics from the tests. All tests pass, once again. --- test/test-source-map-consumer.js | 71 ++++---------------------------- 1 file changed, 8 insertions(+), 63 deletions(-) diff --git a/test/test-source-map-consumer.js b/test/test-source-map-consumer.js index 8f327fa6..162e665f 100644 --- a/test/test-source-map-consumer.js +++ b/test/test-source-map-consumer.js @@ -690,75 +690,20 @@ exports["test index map + generatedPositionFor"] = async function(assert) { }; exports["test index map + originalPositionFor"] = async function(assert) { - console.warn("================================="); var map = await new SourceMapConsumer( util.indexedTestMapWithMappingsAtSectionStart ); assert.ok(map instanceof IndexedSourceMapConsumer); - map.eachMapping(function(m) { - console.warn("mapping:", m); - }); - - map.sources.forEach(function(s, d) { - console.warn("section:", d, s); - }); - - for (let l = 0; l <= 2; l++) { - for (let c = 0; c < 6; c++) { - let pos = map.originalPositionFor({ - line: l, - column: c - }); - console.warn("originalPositionFor:", { - line: l, - column: c, - rv: pos - }); - } - } - - var gen = SourceMapGenerator.fromSourceMap(map); - var smStr = JSON.stringify(gen.toJSON(), null, 2); - console.warn("generated sourcemap:", smStr); - map.destroy(); - - console.warn("-----------------------------------"); - map = await new SourceMapConsumer(smStr); - assert.ok(!(map instanceof IndexedSourceMapConsumer)); - - map.eachMapping(function(m) { - console.warn("mapping:", m); - }); - - map.sources.forEach(function(s, d) { - console.warn("section:", d, s); - }); - - for (let l = 1; l <= 2; l++) { - for (let c = 0; c < 6; c++) { - let pos = map.originalPositionFor({ - line: l, - column: c - }); - console.warn("originalPositionFor:", { - line: l, - column: c, - rv: pos - }); - } - } - - var pos = map.originalPositionFor({ line: 1, column: 0 }); - // assert.equal(pos.line, 1); - // assert.equal(pos.column, 0); - // assert.equal(pos.source, "foo.js"); - // assert.equal(pos.name, "first"); + assert.equal(pos.line, 1); + assert.equal(pos.column, 0); + assert.equal(pos.source, "foo.js"); + assert.equal(pos.name, "first"); pos = map.originalPositionFor({ line: 1, @@ -775,10 +720,10 @@ exports["test index map + originalPositionFor"] = async function(assert) { column: 2 }); - // assert.equal(pos.line, 1); - // assert.equal(pos.column, 0); - // assert.equal(pos.source, "baz.js"); - // assert.equal(pos.name, "third"); + assert.equal(pos.line, 1); + assert.equal(pos.column, 0); + assert.equal(pos.source, "baz.js"); + assert.equal(pos.name, "third"); pos = map.originalPositionFor({ line: 1,