Bug: Uglified babel polyfill with reduce_vars option causes string.split to misbehave in chrome 51

[bdwain]

Bug report or feature request?
Bug report

ES5 or ES6+ input?
Es5

Uglify version (uglifyjs -V)
2.8.26

JavaScript output or error produced.
The native string split method seems to no longer work correctly in the console (or my app) when i load my app. This only happens in chrome 51.

expected behavior (this is in the console after loading my app. uglify is not processing this code directly):

let str = 'fooBar'
str.split(/(?=[A-Z])/)

> ["foo", "Bar"]

actual behavior:

let str = 'fooBar'
str.split(/(?=[A-Z])/)

> ["fooBar"]

How to reproduce
I ran create-react-app with the canary build of react-scripts (because it needs webpack 2 to reproduce the issue) and the only change i had to make was importing babel-polyfill in the entry point to break it. Removing react and all of the css did not fix it. It does not happen in development mode builds. Only production builds. The issue goes away if the build is not minified. Here is the code to reproduce if you just want to clone it. https://github.com/bdwain/uglify-bug.

the full source is just

import 'babel-polyfill';
let foo = 123;
console.log(foo);

The issue seems to have started due to the suggestion in this comment on an uglify thread

The suggestion was to replace ast = ast.transform(compress) with ast = compress.compress(ast);

I do not believe this is due to create-react-app's usage of the webpack plugin specifically because it happens in my own app which is not based on create-react-app. I was able to reproduce it with no options passed to the plugin.

It seems to also be a bug in chrome 51 (because no matter what we do in javascript, native methods should not break), but because it only is exposed when running uglify, it seems relevant. And while I realize that the issue is only appearing when running the webpack uglify plugin, the reason I did not file the issue with them is that it seems to have started when they made a change that was suggested here (same comment as above)

UPDATE: I removed create-react-app and webpack from the example. Now, the example is just uglifying the babel-polyfill with the reduce_vars option enabled (which seems to be the source of the issue). Including that minfied result in index.html causes the issue.

[gaearon]

In particular the culprit seems to be enabling reduce_vars.

[kzc]

The example above is ES6 code using let. [email protected] only works on ES5 code.

I cannot reproduce the issue using ES5 code:

$ node_modules/.bin/uglifyjs -V
uglify-js 2.8.26

$ echo 'var str = "fooBar"; console.log( str.split(/(?=[A-Z])/) );' | node
[ 'foo', 'Bar' ]

$ echo 'var str = "fooBar"; console.log( str.split(/(?=[A-Z])/) );' | node_modules/.bin/uglifyjs -c
var str="fooBar";console.log(str.split(/(?=[A-Z])/));

$ echo 'var str = "fooBar"; console.log( str.split(/(?=[A-Z])/) );' | node_modules/.bin/uglifyjs -c | node
[ 'foo', 'Bar' ]

It would be helpful if someone would post a standalone example of breaking ES5 code with no dependencies other than uglify-js.

[bdwain]

@kzc sorry i think my original comment was a little misleading. the expected and actual outputs are not being run through uglify. they are just run in the console directly.

uglify is run on the app itself (after running through babel and webpack to convert to es5). loading the app causes the global String.split prototype method to misbehave once loading is done (so in the console or if the app uses string.split)

[kzc]

I get that the ES6 is transpiled to ES5. It's just that we don't have the time to learn third party tools and foreign applications for a bug report. If you can generate the ES5 code without uglify and post it in a gist that would be helpful.

[bdwain]

oh sorry yea here's the gist.

https://gist.github.com/bdwain/aa93e842a2fd4d23f0e710eee68ddd98

(edited to use a new gist without react or anything in it)

[bdwain]

for comparison, this gist works

https://gist.github.com/bdwain/d252d19b2ea933abdcc281f6664b8c07

the only difference is that i removed the import of babel-polyfill

(edited to use a new gist without react or anything in it)

[bdwain]

@kzc i edited the examples and gists to remove react from them because it was irrelevant. the full source code is now just

import 'babel-polyfill';
let foo = 123;
console.log(foo);

[gaearon]

I guess it would be nice to trim it down to specific code in Babel polyfill (since that's just JS too).
It is possible that this repros only in specific ES environment/tooling.

[bdwain]

i updated https://github.com/bdwain/uglify-bug to remove create-react-app and webpack from the example. I was able to reproduce it just by running uglify on the babel polyfill and including that in index.html.

[kzc]

CLI repro:

$ mkdir repro
$ cd repro
$ npm install [email protected] [email protected] [email protected]

Create this test.js file:

$ cat test.js
require("babel-polyfill");
var str = "fooBar";
console.log( str.split(/(?=[A-Z])/) );

$ node_modules/.bin/browserify test.js > bundle.js

$ node bundle.js
[ 'foo', 'Bar' ]

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true | node
[ 'fooBar' ]

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=false | node
[ 'foo', 'Bar' ]

Regression introduced in 16cd5d5

[kzc]

@alexlamsl Just a hunch - can you disable replacement of all RegExp literals in reduce_vars and see what happens?

[alexlamsl]

@kzc thanks for the CLI repo - I was just scanning through the diff between reduce_vars=0 and default options, and one thing that hits me is the use of:

var lt = "<", gt = ">";
... lt + "script" + gt ...

I wonder if that defeats some mechanism 😅

(Read: nothing confirmed yet, about to find out...)

[alexlamsl]

Just a hunch - can you disable replacement of all RegExp literals in reduce_vars and see what happens?

I'll give that a go as well in a moment 😉

[alexlamsl]

@kzc would you mind pointing out what I did wrong? 😅

> type test.js
require("babel-polyfill");
var str = "fooBar";
console.log( str.split(/(?=[A-Z])/) );

> nvs use node
PATH = node\7.10.0\x64

> npm install [email protected] [email protected] [email protected]
> node_modules\.bin\browserify test.js > bundle.js
> type bundle.js | node
[ 'foo', 'Bar' ]

> node_modules\.bin\uglifyjs bundle.js -c | node
[ 'foo', 'Bar' ]

> node_modules\.bin\uglifyjs bundle.js -c reduce_vars | node
[ 'foo', 'Bar' ]

> node_modules\.bin\uglifyjs bundle.js -c reduce_vars=true | node
[ 'foo', 'Bar' ]

> node_modules\.bin\uglifyjs bundle.js -c reduce_vars=false | node
[ 'foo', 'Bar' ]

[kzc]

That should work. Try this instead:

$ node_modules/.bin/browserify test.js --node > bundle.js

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=false | node
[ 'foo', 'Bar' ]

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true | node
[ 'fooBar' ]

Barring that, try it on Mac or Linux. Or bundle the program using webpack instead of browserify. I don't know how to use webpack.

[alexlamsl]

Ugh ❗

> nvs use node/0.10
> node_modules\.bin\browserify test.js > bundle.js
> type bundle.js | node
[ 'foo', 'Bar' ]

> node_modules\.bin\uglifyjs bundle.js -c | node
[ 'foo', 'Bar' ]

> nvs use node/4
> node_modules\.bin\browserify test.js > bundle.js
> type bundle.js | node
[ 'foo', 'Bar' ]

> node_modules\.bin\uglifyjs bundle.js -c | node
[ 'foo', 'Bar' ]

> nvs use node/6
> node_modules\.bin\browserify test.js > bundle.js
> type bundle.js | node
[ 'foo', 'Bar' ]

> node_modules\.bin\uglifyjs bundle.js -c | node
[ 'fooBar' ]

> nvs use node/7
> node_modules\.bin\browserify test.js > bundle.js
> type bundle.js | node
[ 'foo', 'Bar' ]

> node_modules\.bin\uglifyjs bundle.js -c | node
[ 'foo', 'Bar' ]

... so the issue is specific to Node.js 6 somehow?

[kzc]

The bug is related to node 6:

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true | node0_10_41 
[ 'foo', 'Bar' ]

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true | node0_12_9 
[ 'foo', 'Bar' ]

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true | node421 
[ 'foo', 'Bar' ]

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true | node591
[ 'foo', 'Bar' ]

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true | node690 
[ 'fooBar' ]

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true | node773
[ 'foo', 'Bar' ]

[alexlamsl]

Lordy! 🙈

I guess I can run through replacing bits of no_reduce_vars.js onto reduce_vars.js until it works on node690 to find out where the culprit is.

(I need to head out for a meeting now. I'll be back to analyse this later today if nobody beats me to it.)

[kzc]

It makes no sense - the file bundle.min.js is the same in all cases:

$ node_modules/.bin/uglifyjs bundle.js -c reduce_vars=true > bundle.min.js

$ node421 bundle.min.js 
[ 'foo', 'Bar' ]

$ node591 bundle.min.js 
[ 'foo', 'Bar' ]

$ node690 bundle.min.js 
[ 'fooBar' ]

$ node773 bundle.min.js 
[ 'foo', 'Bar' ]

The polyfill code must have some obscure runtime assumption of some kind.

[alexlamsl]

Before we go further down the rabbit hole, @bdwain can you confirm if your original issue is run on Node.js 6, and that it works on Node.js 4 or 7?

[bdwain]

@alexlamsl i haven't run on 6. my examples fail for node 7.9.

[kzc]

The version of node used to run uglify does not matter.

What matters is the browser or node engine that runs the minified code.

[bdwain]

oh sorry. i've only run this in chrome 51. i haven't run it in node. i can try to set it up, but chrome 51 is the issue for me. every other version is fine.

what's confusing to me is what in the browser could mess with the global string prototype, which is a native method. I checked when the bug happened, and the split method had not been replaced by some other function.

[kzc]

Can you run the minified bundle in a non-Chrome browser (FF, Safari, IE11) and report your results?

[bdwain]

it worked in all 3 of those. though it also works in the latest version of chrome, so there may be bugs in older versions of those browsers as well. I know it works as far back as chrome 30 with no issues though, except for 51.

[bdwain]

also, for the record i'm on a mac (Sierra) though i actually first noticed this issue in android. it also happens in windows chrome 51 as well

[kzc]

@bmeurer We would greatly appreciate your opinion on this matter: #1964 (comment)

[bmeurer]

@schuay will take a look (cc @fhinkel for Node). Note that Chrome M51 is really outdated.

[alexlamsl]

The usual use case of var r = /a/g; ... r.exec(s) ... is covered, i.e. reduce_vars will not replace r in those cases.

In this case I think we are hitting this:

$ cat test.js
[
    42,
    NaN,
    true,
    null,
    "string",
    undefined,
    /regexp/ig,
].forEach(function(value) {
    try {
        value.field = true;
        if (value.field) {
            console.log(value);
        }
    } catch (e) {}
});

$ node test.js
{ /regexp/gi field: true }

[alexlamsl]

@kzc do you think putting replacement of RegExp behind unsafe is a potential solution here?

According to http://caniuse.com/usage-table Chrome 51 isn't quite as popular. It thinks most people are either stuck on 49 or move onto 56+.

[gaearon]

@kzc Thanks for the deep dive.

[schuay]

Thanks for the cli repro, very useful indeed.

There's two issues here. You're running into a bug (fixed here) in the V8's slow-path implementation of RegExp.p.[Symbol.split]. That problem is introduced by V8 5.1 and fixed in 5.2. I guess there's not much you can do about that other than update Chrome/Node or not use reduce_vars.

More problematic though may be that reduce_vars is moving all regexps onto the slow path by modifying RegExp.prototype.constructor (see the slow-path check here). That should probably be fixed as the performance difference is quite significant (think 40x for split).

And FYI: in recent V8 versions, slow path checks have become even more restrictive and now trigger whenever a property is modified or added to RegExp or RegExp.prototype.

[alexlamsl]

More problematic though may be that reduce_vars is moving all regexps onto the slow path by modifying RegExp.prototype.constructor

@schuay would you mind elaborating on how reduce_vars would cause this to happen? I can't see immediately how turning var r = /a/g; f(r); into f(/a/g); modifies constructor?

[schuay]

The relevant part is actually proto.constructor=$RegExp.

I didn't dig deeper, but RegExp.prototype.constructor is modified when reduce_vars is true, and unmodified when false.

[alexlamsl]

@schuay thanks for the pointer 👍

[kzc]

@schuay Thanks for the detailed analysis and confirmation of the V8 RegExp bug in Chrome 51 and Node 6.

In light of these findings, I suggest that either this RegExp literal substitution be removed altogether or to put it behind a new compress option unsafe_regexp, defaulting to false. It is not appropriate for this functionality to be behind the unsafe option in my opinion due to the deoptimization implications of RegExp literal substitution, and it could benefit from more configuration granularity.

@bdwain Thanks for the detailed bug report for this very obscure bug!

I wonder why babel-polyfill tries to replace String.prototype.split in the first place. Should it be doing that?

[bdwain]

No problem. Thanks for jumping on the issue so quickly.

I'm totally confused why this would happen also. Though from @schuay 's comments I thought the RegExp prototype was actually the one being changed.

In my examples, i was able to do something like this.

window.splitProto = String.prototype.split //before index loads

String.prototype.split = window.splitProto//after index
//try string.split

and it still failed.

though i guess if the prototype was changed and not replaced, that would still. make sense

[kzc]

Though from @schuay 's comments I thought the RegExp prototype was actually the one being changed

Ah okay, I missed that. Same question, different function.

[alexlamsl]

@kzc I thought @schuay meant modifying RegExp.prototype, then call "str".split(regexp) will result in slow-down. I suspect that leakage via proto = $RegExp.prototype, and the difference in value of CORRECT_NEW in the two scripts may subsequently enabled other code paths not shown in your diff which modifies proto thus trigger the slowdown.

As for unsafe vs. unsafe_regexp, aside from whether RegExp operations takes up significant runtime of JavaScript in general, I am surprised that var r = /a/; "aaa".split(r); is that much faster than "aaa".split(/a/); - it's not without precedence I guess, as we have strange performance differences that created wrap_iife and others were suggested as well IIRC.

But I agree with improved granularity - we already have things like unsafe_comps and unsafe_math, so having one more is probably just fine.

[bdwain]

@alexlamsl so just to make sure I understand, is the reduce_vars option safe to use again? and the issue will only reappear if i also enable unsafe_regexp?

[bdwain]

also this fix will not be made in uglify 2 right? only v3?

[alexlamsl]

so just to make sure I understand, is the reduce_vars option safe to use again? and the issue will only reappear if i also enable unsafe_regexp?

Yup, in the upcoming release.

also this fix will not be made in uglify 2 right? only v3?

It's a simple patch, so backporting to v2 shouldn't be too much trouble. And I doubt anybody relies on this breaking change to function before (Famous Last Words:tm:)

[bdwain]

cool. yea while it is a breaking change, it's breaking in that it fixes something that was broken by default, so i think it's fine to backport.

thanks for fixing this issue so quickly! i really appreciate it.

[schuay]

I think there's still some confusion about why regexps can take the slow path, let me try to clear that up.

When running any regexp function (exec, Symbol.match, Symbol.search, test, Symbol.replace, Symbol.split), V8 checks whether the regexp instance is in a known, unmodified shape. If so, it allows us to take certain shortcuts in the implementation that are much faster than if we need to handle the generic case.

The current conditions for the fast path are:

• the regexp object must be unmodified (adding, deleting or modifying any properties switches to slow path)
• the regexp object's prototype must be unmodified
• and regexp.lastIndex must be a simple (small) integer - not an object, not a getter.

Modifying RegExp.prototype is a particularly bad case, as it will move every RegExp in the entire page onto the slow path:

const re_ctor = RegExp.prototype.constructor;
const x = /./;

// All fast.
x.test("");
/./.test("");

RegExp.prototype.constructor = function(a,b) {
  Reflect.construct(re_ctor, a, b);
};

// All slow.
x.test("");  // Even this one, constructed before the RE.prototype modification.
/./.test("");
RegExp(".").test("");
new RegExp(".").test("");

Regardless of how much runtime regexps take up on the web in general, it's probably a bad idea to slow them all down.

I am surprised that var r = /a/; "aaa".split(r); is that much faster than "aaa".split(/a/);

There shouldn't be a difference between the two. As far as I understand, RE.prototype modification is the only issue here :)

[alexlamsl]

@schuay thanks for the clarifications 👍

[kzc]

There's also the issue of object identity which makes replacement of RegExp literal variables inherently unsafe:

$ bin/uglifyjs -V
uglify-js 3.0.9

$ cat test.js
function f(x){return x} var re1 = /a/g; console.log(f(re1) === re1);

$ cat test.js | node
true

$ cat test.js | bin/uglifyjs -c toplevel | node
true

$ cat test.js | bin/uglifyjs -c toplevel,unsafe_regexp=false | node
true

$ cat test.js | bin/uglifyjs -c toplevel,unsafe_regexp=true | node
false

In any case, RegExp literal substitution is disabled in Uglify by default as of [email protected] and [email protected] thanks to @alexlamsl.