Uglifyjs: bug caused by function inlining within loops

Created on 26 Dec 2017 · 18Comments · Source: mishoo/UglifyJS

Bug report or feature request? --> Bug
ES5 or ES6+ input? --> ES5
Uglify version (uglifyjs -V) --> 3.3.2
JavaScript input --> https://code.jquery.com/jquery-1.11.3.js

Hi and Merry Christmas,

We've found a bug updating to [email protected] when the behaviour of jquery changed using some selectors, particularly with the :radio pseudo-selector. Also, we've noticed that the problem only occurred on uglyfied code.

We investigated the jquery code 'vanilla' code and the uglified one side by side, and found that the problem is occurring in this part of the code:

The type variable takes the value of "radio", "checkbox", and so and returns a matcher function for an element. But the code which resulted after uglifying it is the following:

screen shot 2017-12-26 at 4 04 35 pm

Here the createInputPseudo function was inlined but the, pe variable of the for loop haven't got it's var statement. So it was declared in the global scope and worse, has the value of ":image" for all the matcher functions.

Then, immediately after the loop, the pe variable is declared. I like to think that: The uglifier recognized it's error and tried to fix it, but it was too late.

The bug is still present in 3.3.2. I've just tested the bug using just uglify version 3.3.2, minifying the jquery 1.11.3 library using uglifyjs --compress --mangle -- jquery.js > jquery.min.js and running the next code:

<html>
<head>
  <script src="/jquery.min.js"></script>
</head>
<body>
  <input id="a" type="radio"/>
  <input id="b" type="radio"/>
  <input id="log" type="text"/>
  <script>
    var log = $('#log');
    var first = $('input:radio');
    var second = $('input[type="radio"]');
    log.val( 'first: ' + first.length + ', second: ' + second.length);
  </script>
</body>
</html>

Thanks and happy holidays

bug

Source

Jbat1Jumper

👍1

All 18 comments

From your description I don't see a bug. It is valid to do var pe; after first use of pe in JavaScript.

Please provide a reproducible test case, using uglify-js only, to illustrate the issue.

alexlamsl on 26 Dec 2017

In https://github.com/angular/angular-cli/issues/8992 I sort of traced down a similar odd behaviour with [email protected] (versus 3.2.2) that as far as I could tell had something to do with function inlining... but I couldn't boil it down to a reproducible example just yet.

I saw it happening in https://github.com/DevExpress/DevExtreme/blob/17_2/js/core/class.js#L5-L15 where it got inlined inside https://github.com/DevExpress/DevExtreme/blob/17_2/js/core/class.js#L23-L37. Then somewhere in there the wrong methods were being called.

Maybe there's something about the inlined functions and usage of this? I'm going to try investigating it more tomorrow.

filipesilva on 26 Dec 2017

@filipesilva since you are comparing diffs between 3.2.2 and 3.3.2:

check if hoist_funs is the culprit, as it is disabled by default in 3.3.2
if not, you may minimise the diff between the two versions by enabling it

alexlamsl on 26 Dec 2017

@alexlamsl The problem is that a var is needed inside the scope of the loop.

In the original code of the library, there's a for loop with a variable i and a call to a function. On each call, the value of the variable is bound to the scope of the function. So if the variable i changes, the variable in the scope of the function doesn't vary.

In the uglified code, there's a for loop using the variable t and the definition of an anonymous function which borrows the variable pe from the scope of the loop. In each cycle, the value of t gets assigned to pe. But the problem is that the variable pe was never declared local, and all the anonymous functions point to the same variable. So cycle after cycle, when the value of the variable pe changes, all the functions have the last value that pe held. At the end of the loop, we end with five match function, which all compare an element's type against "image".

Tomorrow I'll try to do a small sized example reproducing the error.

Jbat1Jumper on 27 Dec 2017

Not quite sure what you mean:

$ cat test.js
for (i = 0; i < 3; i++) {
    console.log(i);
}
var i;

$ node test.js
0
1
2

Looking forward to a reproducible test case using uglify-js only.

alexlamsl on 27 Dec 2017

@alexlamsl thank you for that tip. Setting hoist_funs: true doesn't change the bug I am seeing but does massively reduce the diff between 3.2.2 and 3.3.2 (14k to 4k lines). That will help narrow it down 👍

filipesilva on 27 Dec 2017

@alexlamsl Almost, but instead of calling a function which uses the value of the variable in that moment, you can create another function which holds the references to the variable i.

$ cat test.js

var arr = [];

for (i = 0; i < 3; i++) {
    arr.push(function() {
        console.log(i)
    });
}
var i;

arr.forEach(function(fn) {
    fn();
})

$ node test.js

3
3
3

I'm still trying to reproduce the bug with uglify, but I'm new to the interns of the library.

Jbat1Jumper on 27 Dec 2017

Nailed it, here's your sample:

(function () {
    var pseudos = {};
    function createFn(i) {
        return function() {
            console.log(i);
        }
    }
    for (i in {a: true, b: true, c: true}) {
        pseudos[i] = createFn(i);
    }
    for (i in pseudos) {
        pseudos[i]();
    }
})()

Running $ node test.js and $ uglifyjs --compress --mangle -- test.js > test.min.js; node test.min.js; results in different outputs.

It seems that if the function is not in global or public scope, and is used only in one place, uglify tries to inline it.

Jbat1Jumper on 27 Dec 2017

@alexlamsl I've been digging into this and think I've found the function that breaks in my examples.

The options I use are the following:

    ecma: 5,
    ie8: false,
    mangle: {
      safari10: true,
      keep_fnames: true,
    },
    compress: {
      hoist_funs: true,
      keep_fnames: true,
      // this is the option that seems to break stuff
      // inline: false,
    },
    output: {
      beautify: true,
      ascii_only: true,
      comments: false,
      webkit: true,
    },

In 3.2.2 I have these functions and the app works:

function listenToElementOutputs(e, t, n, r) {
  for (var o = 0; o < n.outputs.length; o++) {
    var i = n.outputs[o], a = function renderEventHandlerClosure(e, t, n) {
      return function (r) {
        return dispatchEvent(e, t, n, r);
      };
    }(e, n.nodeIndex, elementEventFullName(i.target, i.eventName)), s = i.target, u = e;
    "component" === i.target && (s = null, u = t);
    var c = u.renderer.listen(s || r, i.eventName, a);
    e.disposables[n.outputIndex + o] = c;
  }
}
function elementEventFullName(e, t) {
  return e ? e + ":" + t : t;
}

In 3.3.2 the same function also inlines elementEventFullName and the app stops working:

function listenToElementOutputs(e, t, n, r) {
  for (var o = 0; o < n.outputs.length; o++) {
    var i = n.outputs[o], a = (d = e, l = n.nodeIndex, p = i.target, h = i.eventName,
      f = p ? p + ":" + h : h, function (e) {
        return dispatchEvent(d, l, f, e);
      }), s = i.target, u = e;
    "component" === i.target && (s = null, u = t);
    var c = u.renderer.listen(s || r, i.eventName, a);
    e.disposables[n.outputIndex + o] = c;
  }
  var d, l, f, p, h;
}

In 3.3.2 if I uncomment inline: false, the app will work again:

function listenToElementOutputs(e, t, n, r) {
  for (var o = 0; o < n.outputs.length; o++) {
    var i = n.outputs[o], a = renderEventHandlerClosure(e, n.nodeIndex, elementEventFullName(i.target, i.eventName)), s = i.target, u = e;
    "component" === i.target && (s = null, u = t);
    var c = u.renderer.listen(s || r, i.eventName, a);
    e.disposables[n.outputIndex + o] = c;
  }
}
function renderEventHandlerClosure(e, t, n) {
  return function (r) {
    return dispatchEvent(e, t, n, r);
  };
}
function elementEventFullName(e, t) {
  return e ? e + ":" + t : t;
}

In 3.3.2 if I set mangle: false, (and keep inline: false, commented) the app will work again:

function listenToElementOutputs(view, compView, def, el) {
  for (var i = 0; i < def.outputs.length; i++) {
    var output = def.outputs[i], handleEventClosure = renderEventHandlerClosure(view, def.nodeIndex, (target = output.target,
      name = output.eventName, target ? target + ":" + name : name)), listenTarget = output.target, listenerView = view;
    "component" === output.target && (listenTarget = null, listenerView = compView);
    var disposable = listenerView.renderer.listen(listenTarget || el, output.eventName, handleEventClosure);
    view.disposables[def.outputIndex + i] = disposable;
  }
  var target, name;
}
function renderEventHandlerClosure(view, index, eventName) {
  return function (event) {
    return dispatchEvent(view, index, eventName, event);
  };
}

I'm still trying to deconstruct this into a simpler example/test case, but it seems like there's a bad interfaction between mangle and compress.inline, maybe only when two functions are being inlined.

filipesilva on 27 Dec 2017

I've come up with a repro that's very similar to what @Jbat1Jumper got:

// test.js
(function () {
  var outputs = [
    { type: 0, target: null, eventName: "ngSubmit", propName: null },
    { type: 0, target: null, eventName: "submit", propName: null },
    { type: 0, target: null, eventName: "reset", propName: null },
  ];

  function listenToElementOutputs(outputs) {
    var handlers = [];
    for (var i = 0; i < outputs.length; i++) {
      var output = outputs[i];
      var handleEventClosure = renderEventHandlerClosure(output.eventName);
      handlers.push(handleEventClosure)
    }
    var target, name;
    return handlers;
  }

  function renderEventHandlerClosure(eventName) {
    return function () {
      return console.log(eventName);
    };
  }

  listenToElementOutputs(outputs).forEach(handler => handler());
})()

The test command is uglifyjs --compress --mangle -- test.js > test.min.js && node test.min.js.

With 3.3.2 the output is:

reset
reset
reset

// test.min.js
!function(){(function(e){for(var n=[],t=0;t<e.length;t++){var r=function(e){return function(){return console.log(e)}}(e[t].eventName);n.push(r)}return n})([{type:0,target:null,eventName:"ngSubmit",propName:null},{type:0,target:null,eventName:"submit",propName:null},{type:0,target:null,eventName:"reset",propName:null}]).forEach(e=>e())}();



md5-f0585e5d661ade1a9fa2a3e3747a7576



ngSubmit
submit
reset



md5-98882b285c5b04cabff86f7ba08e6efe



// test.min.js
!function(){(function(e){for(var t=[],n=0;n<e.length;n++){var r=e[n],l=(a=r.eventName,function(){return console.log(a)});t.push(l)}var a;return t})([{type:0,target:null,eventName:"ngSubmit",propName:null},{type:0,target:null,eventName:"submit",propName:null},{type:0,target:null,eventName:"reset",propName:null}]).forEach(e=>e())}();

The way by which the function is inlined is not respecting the proper scoping of eventName, with 3.3.2 using what is saved in the closure variable instead of a scoped one.

filipesilva on 27 Dec 2017

You can find a repo at https://github.com/filipesilva/uglify-app.

git clone https://github.com/filipesilva/uglify-app
cd uglify-app
npm i
npm run uglify
npm run use-uglify-3.2.2
npm run uglify

filipesilva on 27 Dec 2017

@filipesilva's test case in ES5 form against master f30790b11bb9e162a19d7769ab54d8bb3f61cc27:

$ cat test.js
(function () {
  var outputs = [
    { type: 0, target: null, eventName: "ngSubmit", propName: null },
    { type: 0, target: null, eventName: "submit", propName: null },
    { type: 0, target: null, eventName: "reset", propName: null },
  ];
  function listenToElementOutputs(outputs) {
    var handlers = [];
    for (var i = 0; i < outputs.length; i++) {
      var output = outputs[i];
      var handleEventClosure = renderEventHandlerClosure(output.eventName);
      handlers.push(handleEventClosure)
    }
    var target, name;
    return handlers;
  }
  function renderEventHandlerClosure(eventName) {
    return function () {
      return console.log(eventName);
    };
  }
  listenToElementOutputs(outputs).forEach(function(handler) {
    return handler()
  });
})();

$ cat test.js | node
ngSubmit
submit
reset



md5-98882b285c5b04cabff86f7ba08e6efe



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

kzc on 27 Dec 2017

👍1

@Jbat1Jumper's test case might be a different bug judging by the options:

$ cat t2663.js
(function () {
    var pseudos = {};
    function createFn(i) {
        return function() {
            console.log(i);
        }
    }
    for (i in {a: true, b: true, c: true}) {
        pseudos[i] = createFn(i);
    }
    for (i in pseudos) {
        pseudos[i]();
    }
})();

$ cat t2663.js | node
a
b
c



md5-98882b285c5b04cabff86f7ba08e6efe



$ cat t2663.js | bin/uglifyjs -c | node
a
b
c



md5-98882b285c5b04cabff86f7ba08e6efe



$ cat t2663.js | bin/uglifyjs -mc | node
c
c
c



md5-98882b285c5b04cabff86f7ba08e6efe



$ cat t2663.js | bin/uglifyjs -mc --no-rename | node
a
b
c

kzc on 27 Dec 2017

@filipesilva @Jbat1Jumper @kzc thank you very much for the test cases & efforts.

Just got home from badminton, will get onto these after settling down :+1:

alexlamsl on 27 Dec 2017

👍1

https://github.com/mishoo/UglifyJS2/issues/2663#issuecomment-354129256 without rename:

(function(i) {
    var pseudos = {};
    function createFn(j) {
        return function() {
            console.log(j);
        }
    }
    for (i in {a: true, b: true, c: true}) {
        pseudos[i] = createFn(i);
    }
    for (i in pseudos) {
        pseudos[i]();
    }
})();

$ cat test.js | node
a
b
c

$ uglifyjs test.js -c | node
c
c
c

alexlamsl on 27 Dec 2017

@alexlamsl Can you give this issue a suitable title?

kzc on 27 Dec 2017

Thanks for fixing this so promptly @alexlamsl @kzc 👍

filipesilva on 27 Dec 2017

Thanks to everyone 😬 @alexlamsl @kzc @filipesilva

Jbat1Jumper on 28 Dec 2017

Was this page helpful?

0 / 5 - 0 ratings

Related issues

Bug in 'mangle` with nested catch blocks

alexlamsl · 5Comments

Properties not resolved with compress option

chrismanley · 5Comments

How to keep ES6 class name property

PinkyJie · 3Comments

DefaultsError: `warnings` is not a supported option

lhtdesignde · 3Comments

ERROR: Unexpected token: keyword (const)

utdrmac · 4Comments