Skip to content

Commit

Permalink
Fix collisions between original property and mangled property (#1063)
Browse files Browse the repository at this point in the history 
* Fix collisions between original property and mangled property

This fixes a bug when an original property (`obj.i`) shares the same
name as a mangled property (`obj.prop` -> `n.i`), and you're using a
name-cache. Because our mangled name collides, we have to ensure all
original `.i` properties are renamed to something else (`n.o`).

But, because we had a name-cache, we would reserve the mangled property
name in the mangler. That prevents us from attempting to rename the
original `.i` property to `.o`.

* Ensure we don't reuse an already mangled name from cache
  • Loading branch information
@jridgewell
jridgewell committed Sep 18, 2021
1 parent 0479921 commit 38bef40
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 25 deletions.
6 changes: 3 additions & 3 deletions lib/propmangle.js
View file
Expand Up @@ -197,9 +197,6 @@ function mangle_properties(ast, options) {
var cache;
if (options.cache) {
cache = options.cache.props;
cache.forEach(function(mangled_name) {
reserved.add(mangled_name);
});
} else {
cache = new Map();
}
Expand All @@ -217,6 +214,9 @@ function mangle_properties(ast, options) {

var names_to_mangle = new Set();
var unmangleable = new Set();
// Track each already-mangled name to prevent nth_identifier from generating
// the same name.
cache.forEach((mangled_name) => unmangleable.add(mangled_name));

var keep_quoted = !!options.keep_quoted;

Expand Down
87 changes: 65 additions & 22 deletions test/mocha/minify.js
View file
Expand Up @@ -10,9 +10,9 @@ function read(path) {

describe("minify", function() {
it("Should test basic sanity of minify with default options", async function() {
var js = 'function foo(bar) { if (bar) return 3; else return 7; var u = not_called(); }';
var js = "function foo(bar) { if (bar) return 3; else return 7; var u = not_called(); }";
var result = await minify(js);
assert.strictEqual(result.code, 'function foo(n){return n?3:7}');
assert.strictEqual(result.code, "function foo(n){return n?3:7}");
});

it("Should skip inherited keys from `files`", async function() {
Expand Down Expand Up @@ -124,14 +124,54 @@ describe("minify", function() {
assert.strictEqual(run_code(compressed), run_code(original));
});

it.skip("Should avoid mangled names in cache", async function() {
it("Should avoid mangled names in cache", async function() {
var cache = {};
var original = "";
var compressed = "";
const nth_identifier = {
get(n) {
return String.fromCharCode(n + "a".charCodeAt(0));
}
};

await for_each_async([
'"xxxyy";var i={prop1:1};',
'"xxyyy";var j={prop2:2,prop3:3},k=4;',
"console.log(i.prop1,j.prop2,j.prop3,k);",
"console.log(i.prop2 === undefined, j.prop1 === undefined);",
], async function(code) {
var result = await minify(code, {
compress: false,
mangle: {
properties: {
nth_identifier,
},
toplevel: true,
},
nameCache: cache
});
original += code;
compressed += result.code;
});
assert.strictEqual(compressed, [
'"xxxyy";var x={g:1};',
'"xxyyy";var p={h:2,i:3},r=4;',
"console.log(x.g,p.h,p.i,r);",
"console.log(x.h===undefined,p.g===undefined);"
].join(""));
assert.strictEqual(run_code(compressed), run_code(original));
});

it("Should consistently rename properties colliding with a mangled name", async function() {
var cache = {};
var original = "";
var compressed = "";

await for_each_async([
'"xxxyy";var i={s:1};',
'"xxyyy";var j={t:2,u:3},k=4;',
"console.log(i.s,j.t,j.u,k);",
"function fn1(obj) { obj.prop = 1; obj.i = 2; }",
"function fn2(obj) { obj.prop = 1; obj.i = 2; }",
"let o1 = {}, o2 = {}; fn1(o1); fn2(o2);",
"console.log(o1.prop === o2.prop, o2.prop === 1, o1.i === o2.i, o2.i === 2);",
], async function(code) {
var result = await minify(code, {
compress: false,
Expand All @@ -145,11 +185,14 @@ describe("minify", function() {
compressed += result.code;
});
assert.strictEqual(compressed, [
'"xxxyy";var x={x:1};',
'"xxyyy";var y={y:2,a:3},a=4;',
'console.log(x.x,y.y,y.a,a);',
// It's important that the `n.i` here conflicts with the original's
// `obj.i`, so `obj.i` gets consistently renamed to `n.o`.
"function n(n){n.i=1;n.o=2}",
"function c(n){n.i=1;n.o=2}",
"let f={},e={};n(f);c(e);",
"console.log(f.i===e.i,e.i===1,f.o===e.o,e.o===2);",
].join(""));
assert.strictEqual(run_code(compressed), run_code(original));
assert.equal(run_code(compressed), run_code(original));
});

it("Should not parse invalid use of reserved words", async function() {
Expand Down Expand Up @@ -186,7 +229,7 @@ describe("minify", function() {
keep_quoted_props: false,
quote_style: 3
}});
assert.strictEqual(result.code, 'var foo={x:1,y:2,z:3};');
assert.strictEqual(result.code, "var foo={x:1,y:2,z:3};");
});
});

Expand Down Expand Up @@ -237,7 +280,7 @@ describe("minify", function() {

var map = JSON.parse(result.map);

assert.equal(map.file, 'simple.min.js');
assert.equal(map.file, "simple.min.js");
assert.equal(map.sourcesContent.length, 1);
assert.equal(map.sourcesContent[0],
'let foo = x => "foo " + x;\nconsole.log(foo("bar"));');
Expand Down Expand Up @@ -271,7 +314,7 @@ describe("minify", function() {

describe("sourceMapInline", function() {
it("should append source map to output js when sourceMapInline is enabled", async function() {
var result = await minify('var a = function(foo) { return foo; };', {
var result = await minify("var a = function(foo) { return foo; };", {
sourceMap: {
url: "inline"
}
Expand All @@ -281,7 +324,7 @@ describe("minify", function() {
"//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIjAiXSwibmFtZXMiOlsiYSIsImZvbyJdLCJtYXBwaW5ncyI6IkFBQUEsSUFBSUEsRUFBSSxTQUFTQyxHQUFPLE9BQU9BIn0=");
});
it("should not append source map to output js when sourceMapInline is not enabled", async function() {
var result = await minify('var a = function(foo) { return foo; };');
var result = await minify("var a = function(foo) { return foo; };");
var code = result.code;
assert.strictEqual(code, "var a=function(n){return n};");
});
Expand Down Expand Up @@ -390,24 +433,24 @@ describe("minify", function() {
});

it("should work with compress defaults disabled", async function() {
var code = 'if (true) { console.log(1 + 2); }';
var code = "if (true) { console.log(1 + 2); }";
var options = {
compress: {
defaults: false,
}
};
assert.strictEqual((await minify(code, options)).code, 'if(true)console.log(1+2);');
assert.strictEqual((await minify(code, options)).code, "if(true)console.log(1+2);");
});

it("should work with compress defaults disabled and evaluate enabled", async function() {
var code = 'if (true) { console.log(1 + 2); }';
var code = "if (true) { console.log(1 + 2); }";
var options = {
compress: {
defaults: false,
evaluate: true,
}
};
assert.strictEqual((await minify(code, options)).code, 'if(true)console.log(3);');
assert.strictEqual((await minify(code, options)).code, "if(true)console.log(3);");
});

describe("enclose", function() {
Expand All @@ -424,7 +467,7 @@ describe("minify", function() {
it("Should work with arg", async function() {
var result = await minify(code, {
compress: false,
enclose: 'undefined',
enclose: "undefined",
mangle: false,
});
if (result.error) throw result.error;
Expand All @@ -433,7 +476,7 @@ describe("minify", function() {
it("Should work with arg:value", async function() {
var result = await minify(code, {
compress: false,
enclose: 'window,undefined:window',
enclose: "window,undefined:window",
mangle: false,
});
if (result.error) throw result.error;
Expand All @@ -442,9 +485,9 @@ describe("minify", function() {
it("Should work alongside wrap", async function() {
var result = await minify(code, {
compress: false,
enclose: 'window,undefined:window',
enclose: "window,undefined:window",
mangle: false,
wrap: 'exports',
wrap: "exports",
});
if (result.error) throw result.error;
assert.strictEqual(result.code, '(function(window,undefined){(function(exports){function enclose(){console.log("test enclose")}enclose()})(typeof exports=="undefined"?exports={}:exports)})(window);');
Expand Down

0 comments on commit 38bef40

Please sign in to comment.