Bug: collapse_vars break zone.js (var missing) #2437

michael42 · 2017-11-05T18:04:37Z

Hi,

it seems that uglify-js (3.1.7) breaks zone.js (0.8.18, ES5 input), when using collapse_vars. (Which is only enabled by default when using the JS API, but not on the command line?)

Unfortunately, I didn't manage to get a nice minimal reproduction snippet, as zone.js is pretty large and the issue disappears when I'm trying a the innermost 2 functions.

JavaScript input

npm install zone.js@0.8.18
HTML: <!DOCTYPE html><html><script type="text/javascript" src="zone.min.js"></script></html>

The uglifyjs CLI command executed or minify() options used.

ReferenceError: uglifyjs --beautify -c collapse_vars=true node_modules/zone.js/dist/zone.js -o zone.min.js
No ReferenceError: uglifyjs --beautify -c collapse_vars=false node_modules/zone.js/dist/zone.js -o zone.min.js

JavaScript output or error produced.

ReferenceError: req is not defined
    at zone.min.js:1140
    at zone.min.js:1157
    [...]

zone.min.js, lines 1128 to 1157:

if (function() {
        if ((isBrowser || isMix) && !Object.getOwnPropertyDescriptor(HTMLElement.prototype, "onclick") && "undefined" != typeof Element) {
            var desc = Object.getOwnPropertyDescriptor(Element.prototype, "onclick");
            if (desc && !desc.configurable) return !1;
        }
        var xhrDesc = Object.getOwnPropertyDescriptor(XMLHttpRequest.prototype, "onreadystatechange");
        if (xhrDesc) return Object.defineProperty(XMLHttpRequest.prototype, "onreadystatechange", {
            enumerable: !0,
            configurable: !0,
            get: function() {
                return !0;
            }
        }), result = !!(req = new XMLHttpRequest()).onreadystatechange, Object.defineProperty(XMLHttpRequest.prototype, "onreadystatechange", xhrDesc || {}), // <-- line 1140
            result;
        var SYMBOL_FAKE_ONREADYSTATECHANGE_1 = zoneSymbol("fakeonreadystatechange");
        Object.defineProperty(XMLHttpRequest.prototype, "onreadystatechange", {
            enumerable: !0,
            configurable: !0,
            get: function() {
                return this[SYMBOL_FAKE_ONREADYSTATECHANGE_1];
            },
            set: function(value) {
                this[SYMBOL_FAKE_ONREADYSTATECHANGE_1] = value;
            }
        });
        var detectFunc = function() {};
        (req = new XMLHttpRequest()).onreadystatechange = detectFunc;
        var result = req[SYMBOL_FAKE_ONREADYSTATECHANGE_1] === detectFunc;
        return req.onreadystatechange = null, result;
    }()) {

The text was updated successfully, but these errors were encountered:

alexlamsl · 2017-11-05T18:31:38Z

Thanks for the report - investigating.

kzc · 2017-11-05T18:43:57Z

repro:

$ cat cv.js 
function foo() {
    bar();
}
function bar() {
    if (xhrDesc) {
        var req = new XMLHttpRequest();
        var result = !!req.onreadystatechange;
        Object.defineProperty(XMLHttpRequest.prototype, 'onreadystatechange', xhrDesc || {});
        return result;
    }
    else {
        var req = new XMLHttpRequest();
        var detectFunc = function () { };
        req.onreadystatechange = detectFunc;
        var result = req[SYMBOL_FAKE_ONREADYSTATECHANGE_1] === detectFunc;
        req.onreadystatechange = null;
        return result;
    }
}
foo();

$ bin/uglifyjs cv.js -c toplevel -b
!function() {
    !function() {
        if (xhrDesc) return result = !!(req = new XMLHttpRequest()).onreadystatechange, 
        Object.defineProperty(XMLHttpRequest.prototype, "onreadystatechange", xhrDesc || {}), 
        result;
        var detectFunc = function() {};
        (req = new XMLHttpRequest()).onreadystatechange = detectFunc;
        var result = req[SYMBOL_FAKE_ONREADYSTATECHANGE_1] === detectFunc;
        req.onreadystatechange = null;
    }();
}();

- avoid issues with identity reference due to deep cloning fixes mishoo#2437

alexlamsl · 2017-11-05T19:00:27Z

@kzc Thanks a lot! 💯

I was actually cleaning up this region of code in #2439 so finding the fix was easy, but then I was pulling my hair out trying to make a minimal test case for it 😓

kzc · 2017-11-05T20:50:13Z

@alexlamsl If function bar precedes function foo then the result is different:

$ bin/uglifyjs -V
uglify-js 3.1.7

$ cat cv2.js 
function bar() {
    if (xhrDesc) {
        var req = new XMLHttpRequest();
        var result = !!req.onreadystatechange;
        Object.defineProperty(XMLHttpRequest.prototype, 'onreadystatechange', xhrDesc || {});
        return result;
    }
    else {
        var req = new XMLHttpRequest();
        var detectFunc = function () { };
        req.onreadystatechange = detectFunc;
        var result = req[SYMBOL_FAKE_ONREADYSTATECHANGE_1] === detectFunc;
        req.onreadystatechange = null;
        return result;
    }
}
function foo() {
    bar();
}
foo();

$ bin/uglifyjs cv2.js -c toplevel -b
!function() {
    !function() {
        if (xhrDesc) return result = !!(req = new XMLHttpRequest()).onreadystatechange, 
        Object.defineProperty(XMLHttpRequest.prototype, "onreadystatechange", xhrDesc || {}), 
        result;
        var req = new XMLHttpRequest(), detectFunc = function() {};
        req.onreadystatechange = detectFunc;
        var result = req[SYMBOL_FAKE_ONREADYSTATECHANGE_1] === detectFunc;
        req.onreadystatechange = null;
    }();
}();

kzc · 2017-11-06T00:12:41Z

To temporarily work around this bug in uglify-js@3.1.7 and uglify-es@3.1.7 set the following minify option:

compress: {
    reduce_vars: false
}

or from the command line:

-c reduce_vars=false

or lock in an earlier version of uglify in your project.

alexlamsl · 2017-11-06T05:13:22Z

@kzc when you swap the order of foo() and bar() to put the latter on top, collapse_vars will now act on the original copy of bar() instead of its cloned & inlined version.

- avoid issues with identity reference due to deep cloning fixes mishoo#2437

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Nov 5, 2017

private accounting for collapse_vars

cc22dfe

- avoid issues with identity reference due to deep cloning fixes mishoo#2437

alexlamsl added the bug label Nov 5, 2017

clydin mentioned this issue Nov 5, 2017

Uncaught ReferenceError: req is not defined with cli@1.5 angular/angular-cli#8359

Closed

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Nov 6, 2017

private accounting for collapse_vars

532f047

- avoid issues with identity reference due to deep cloning fixes mishoo#2437

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Nov 6, 2017

private accounting for collapse_vars

f0cef8d

- avoid issues with identity reference due to deep cloning fixes mishoo#2437

alexlamsl closed this as completed in 6c45101 Nov 6, 2017

kzc mentioned this issue Nov 6, 2017

"major issues discovered" deprecation on npm #2447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: collapse_vars break zone.js (var missing) #2437

Bug: collapse_vars break zone.js (var missing) #2437

michael42 commented Nov 5, 2017

alexlamsl commented Nov 5, 2017

kzc commented Nov 5, 2017

alexlamsl commented Nov 5, 2017

kzc commented Nov 5, 2017

kzc commented Nov 6, 2017

alexlamsl commented Nov 6, 2017

Bug: collapse_vars break zone.js (var missing) #2437

Bug: collapse_vars break zone.js (var missing) #2437

Comments

michael42 commented Nov 5, 2017

alexlamsl commented Nov 5, 2017

kzc commented Nov 5, 2017

alexlamsl commented Nov 5, 2017

kzc commented Nov 5, 2017

kzc commented Nov 6, 2017

alexlamsl commented Nov 6, 2017