Uglifyjs: 2.8.1 potential bug

[SirZach]

I cannot verify with certainty yet but my app just pulled in 2.8.1 and now when it minifies, i get an Uncaught SyntaxError: Illegal break statement. If I disable minification then I don't have this issue.

Again, I'm looking through the commits to see if it is this repo but cannot confirm. Thanks.

[SirZach]

I've confirmed that it's still broken with 2.8.0 but is working with 2.7.5

[kzc]

First test with 2.8.1.

If that does not work please provide JS source code and uglify options to reproduce this issue.

[SirZach]

@kzc I found the problem with 2.8.1. Not sure I can provide the JS source code. I'm using ember-cli-uglify which pulls in broccoli-uglify-sourcemap which pulls in UglifyJS2. I've gone into broccoli-uglify-sourcemap and changed its package.json's version of uglify-js from ^2.7.0 to 2.7.5 and that works

[kzc]

Well, if you can't post a small example of javascript that breaks with uglify 2.8.x, not much we can do.

[epoberezkin]

Also fails with 2.8.x: https://travis-ci.org/epoberezkin/ajv/jobs/206165572

[alexlamsl]

@epoberezkin from the job log looks like you are using uglify-js 2.8.0 - try 2.8.1?

Either way, please provide actual code examples that would reproduce any issues.

[kzc]

@alexlamsl Found a problem with warnings and reduce_vars in [email protected]:

npm install [email protected]

# this is okay
bin/uglifyjs node_modules/ajv/dist/ajv.bundle.js -c reduce_vars=true,warnings=false

# this is okay
bin/uglifyjs node_modules/ajv/dist/ajv.bundle.js -c reduce_vars=false

# this is not
bin/uglifyjs node_modules/ajv/dist/ajv.bundle.js -c

...
WARN: Boolean && always false [node_modules/ajv/dist/ajv.bundle.js:4647,6]
undefined:190
        return props[p];
                    ^

TypeError: Cannot read property 'file' of undefined

[kzc]

self.condition.start is undefined here:

https://github.com/mishoo/UglifyJS2/blob/f5cbe19b753bf84f8b11f96e2c91a8ca49525dba/lib/compress.js#L2178

[alexlamsl]

@kzc I've plugged a few instances of make_node() not passing in start info in #1513 - mind giving it a quick spin and see if it makes any difference?

[kzc]

Reduced test case:

$ bin/uglifyjs -V
uglify-js 2.8.2

$ echo 'if (false && false) console.log("nope");' | bin/uglifyjs -c
WARN: Boolean && always false [-:1,4]
undefined:191
        return props[p];
                    ^

TypeError: Cannot read property 'file' of undefined

[kzc]

@alexlamsl It works fine with 2.7.5. Some start tokens are not being copied in 2.8.x.

$ uglifyjs -V
uglify-js 2.7.5

$ echo 'if (1 && 0) console.log("nope");' | uglifyjs -c reduce_vars,collapse_vars
WARN: Condition left of && always true [-:1,4]
WARN: Condition always false [-:1,9]
WARN: Dropping unreachable code [-:1,12]

[kzc]

@alexlamsl Should re-open this ticket.

There are other [email protected] issues with https://github.com/epoberezkin/ajv when running https://github.com/epoberezkin/ajv/blob/master/.travis.yml that do not occur when running against [email protected]. Clone the repo to see the errors for yourself.

[alexlamsl]

@kzc thanks for the info - will have a look myself after making the 2.8.3 release 😉

[alexlamsl]

npm run test-spec passes with 2.8.3 - the rest I'm still trying to make it work on Windows... 😞

[kzc]

This is successful:

# uglify-js 2.8.3
uglifyjs dist/ajv.bundle.js -c reduce_vars=false > dist/ajv.min.js
node_modules/.bin/karma start --single-run --browsers PhantomJS

This fails:

# uglify-js 2.8.3
uglifyjs dist/ajv.bundle.js -c reduce_vars=true > dist/ajv.min.js
node_modules/.bin/karma start --single-run --browsers PhantomJS

PhantomJS 2.1.1 (Mac OS X 0.0.0) Ajv compile method should compile schema and return validating function FAILED
    expected true to equal false

I hate to say it, but I think a 2.8.4 with reduce_vars disabled by default is needed until this is debugged.

[alexlamsl]

@kzc I can't even get those bash scripts to run, so if you can track that one down I'd be very grateful.

(Travis CI not loading any of the job logs for me...)

Still a bit puzzled as to where this is going wrong and the scope of this breakage, but I guess disabling reduce_vars by default is the safest course of action for now.

[kzc]

How can I disable replacing regexp literals only for reduce_vars? I think that might be it.

[alexlamsl]

@kzc try:

diff --git a/lib/compress.js b/lib/compress.js
index 32e7a64..d6bf78a 100644
--- a/lib/compress.js
+++ b/lib/compress.js
@@ -205,7 +205,7 @@ merge(Compressor.prototype, {
         this.walk(tw);

         function reset_def(def) {
-            def.fixed = true;
+            def.fixed = def.init && !(def.init instanceof AST_RegExp);
             def.references = [];
             def.should_replace = undefined;
             if (unsafe && def.init) {

Before

$ echo 'var foobar=/b/;console.log(foobar);' | uglifyjs -c
var foobar=/b/;console.log(/b/);

After

$ echo 'var foobar=/b/;console.log(foobar);' | uglifyjs -c
var foobar=/b/;console.log(foobar);

[kzc]

My hunch about regexp literals and reduce_vars was wrong. Still looking.

[alexlamsl]

@kzc would you mind sending me dist/ajv.bundle.js so I can play around with it over here? I should be able to produce those two .min files and can compare them line by line...

[kzc]

@alexlamsl See: https://github.com/mishoo/UglifyJS2/issues/1518#issuecomment-283255833

[alexlamsl]

This might be it:

...
                                if (...) {
                                    var $typeChecked = !0;
...
                                }
                            }
                        }
                    }
                }
                if (... && !$typeChecked && ...) {
...

[kzc]

You isolated it:

$ bin/uglifyjs -V
uglify-js 2.8.3

expected:

$ echo 'function f(){return 0} if (f()) { var t = 1 } if (!t) console.log(t)' | node
undefined

actual:

$ echo 'function f(){return 0} if (f()) { var t = 1 } if (!t) console.log(t)' | bin/uglifyjs -c
WARN: Condition always false [-:1,50]
WARN: Dropping unreachable code [-:1,54]
function f(){return 0}if(f())var t=1;

$ echo 'function f(){return 0} if (f()) { var t = 1 } if (!t) console.log(t)' | bin/uglifyjs -c | node
WARN: Condition always false [-:1,50]
WARN: Dropping unreachable code [-:1,54]

[alexlamsl]

@kzc ninja-ed by your example 🤣

Anyway, may I fix this up instead of disabling reduce_vars altogether?

[kzc]

Your call. How quickly can you fix it?

[alexlamsl]

@kzc working on it as we speak - if somehow this is taking longer than I expected, I'll push go for the disabling route instead.

[kzc]

Good luck! I'll try out your fix tomorrow. Hopefully there's not yet another issue in this source file.

[kzc]

@alexlamsl #1524 allows https://github.com/epoberezkin/ajv to run npm test successfully.

However when diffing the uglify output of reduce_vars=false with reduce_vars=true there is some questionable code:

uglifyjs dist/ajv.bundle.js -c reduce_vars=false -b > dist/ajv.min.js-rvf
uglifyjs dist/ajv.bundle.js -c reduce_vars=true  -b > dist/ajv.min.js-rvt
diff -u dist/ajv.min.js-rvf dist/ajv.min.js-rvt

...

@@ -2966,10 +2966,10 @@
                 var result;
                 return text = source, at = 0, ch = " ", result = value(), white(), ch && error("Syntax error"), 
                 "function" == typeof reviver ? function walk(holder, key) {
-                    var k, v, value = holder[key];
+                    var k, v, value = holder[""];
                     if (value && "object" == typeof value) for (k in value) Object.prototype.hasOwnProperty.call(value, k) && (v = walk(value, k), 
                     void 0 !== v ? value[k] = v : delete value[k]);
-                    return reviver.call(holder, key, value);
+                    return reviver.call(holder, "", value);
                 }({
                     "": result
                 }, "") : result;

Can you confirm that this code is incorrect and hand inspect the entire diff?

[alexlamsl]

@kzc I did inspect the combination in your example (with the exception of -b bracketize)

Your quoted section is correct, because function walk(holder, key){...}({...}, "") is an IIFE and key is assigned with "" as a result.

There are a few other places where this happens as well in that code, and lots of $errorKeyword substitutions.

[alexlamsl]

(Part of) the idea for that optimisation rule is that with keep_fargs=false the now unused function argument key would then be eliminated.

(What hasn't been done is discarding the corresponding unused invocation parameter. But let's fix bugs first.)

[kzc]

Your quoted section is correct, because function walk(holder, key){...}({...}, "") is an IIFE and key is assigned with "" as a result.

I see. I did not realize that reduce_vars follows parameters into function calls. Impressive!

If that's the case then #1524 looks good for a 2.8.4 release.

[alexlamsl]

You were surprised by that once already 👻

https://github.com/mishoo/UglifyJS2/pull/1473#issuecomment-278349553

Anyway, just passed the battery of test/Jetstream.js, and I ran through the same set of options on test/benchmark.js for good measure (can only verify uglify-js does not crash for the latter set of tests). Will go ahead and do all the release stuff now.

[kzc]

You were surprised by that once already

I vaguely recall that now that you mention it. Thanks for the embarrassing link.
;-)

[SirZach]

I will gladly test with 2.8.4 and see if it addresses my original issue. Thanks for the quick response times!

[kzc]

@alexlamsl > (Part of) the idea for that optimisation rule is that with keep_fargs=false the now unused function argument key would then be eliminated.

Are you doing this for IIFEs only?

Any thought to eliminate single-use functions without side-effects altogether? i.e.:

$ echo 'console.log( function(x){return 2*x}(3) );' | bin/uglifyjs -c passes=3
console.log(function(x){return 6}(3));

could be reduced to:

console.log(6);

[alexlamsl]

@kzc I did wonder about that, yes. I think the rule is just turning function(...){return non_scope_dependent_node}(...) into non_scope_dependent_node, after other optimisation rules soak up the slack.

[alexlamsl]

@SirZach thanks for the offer - uglify-js 2.8.4 should now be available on npm

Would be good to know if it works for you 😉

[kzc]

I think the rule is just turning function(...){return non_scope_dependent_node}(...) into non_scope_dependent_node, after other optimisation rules soak up the slack.

For IIFE constant return values, it's a no brainer - regardless of number of arguments.

Have to ponder the consequences of non_scope_dependent_node variables. It could be fine.

[kzc]

@epoberezkin Thanks for the bug report. Could you please test against [email protected]?

[alexlamsl]

@kzc looks like greenkeeper is happy with 2.8.4 on ajv

https://travis-ci.org/epoberezkin/ajv/builds/206671000

[epoberezkin]

@kzc @alexlamsl thank you! sorry couldn't provide more help than serving as the testing grounds :)

[SirZach]

@alexlamsl unfortunately 2.8.4 is still problematic for me. I figured out a way to lock down to 2.7.5 so I'm not in desperation mode anymore. I'll periodically check back with later versions and see if they're working or not. Thanks!