Incorrect mangling of local variables with try {} catch(e) {}

[laino]

I had a few lines in my codebase that looked like this:

try {
    var part = file.slice(0, 2);
    reader.readAsArrayBuffer(part);
} catch (e) {
    onUnreadable();
}

And "e is not a function" got thrown at "onUnreadable();".
I temporarily fixed it by rewriting my code like this:

try {
    var part = file.slice(0, 2);
    reader.readAsArrayBuffer(part);
} catch (wat) {
    onUnreadable();
}

The issue should be obvious here.

I would love you guys if you fixed this, because I'm kinda worried that those bugs exists elsewhere in my code...

[laino]

This is still happening.

[rvanvelzen]

Is this all the code you're uglifying? I don't see anything wrong with it, or with the uglified result:

try{var part=file.slice(0,2);reader.readAsArrayBuffer(part)}catch(e){onUnreadable()}

[laino]

No. It's part of a pretty big codebase.

[mishoo]

I suspect this is because of UglifyJS's default to be compliant with IE8 bugs and leaks the catch variable in the outer scope. Can you try passing the --screw-ie8 flag, see if that fixes it?

Anyhow, a complete repro case would be welcome.

[laino]

I'm already using --screw-ie8

[laino]

And I can't really reproduce it right now. The bug comes and goes as I change code somewhere else. I suspect that for some reason the compressor fails to honour all possible local variables when creating the shorter "aliases". No idea.

[mishoo]

Could be a serious bug then, I'd appreciate it if you can post some minimal code which demonstrates the issue.

[laino]

I'll post it here once I manage to condense it to a minimal example.

[jareware]

This looks like something I'm seeing as well, though curiously enough only happening on IE8.

[sardonicpresence]

Minimal example:

bad = function (e) {
  return function(error) {
    try {
      e();
    } catch (e) {
      error(e);
    }
  };
};

In version 2.4.5, with or without --screw-ie8, the compressor would eliminate the e argument as unused and the mangler would rename the call to e to match the argument to catch instead of the function argument.

As of 2.4.6 this behaviour is fixed with --screw-ie8.

As of 2.5.0 this case still mangles incorrectly without --screw-ie8 (though the compressor no longer considers the argument unused).

[kzc]

Just showing the output for ticket viewers.

original

$ cat tc.js 
bad = function(e) {
    return function(error) {
        try {
            e();              /* <== */
        } catch (e) {
            error(e);
        }
    };
};

with uglifyjs v2.50 -m --screw-ie8 -b

$ uglifyjs -m --screw-ie8 -b < tc.js 
bad = function(n) {
    return function(t) {
        try {
            n();              /* <== */
        } catch (n) {
            t(n);
        }
    };
};

with uglifyjs v2.50 -m -b

$ uglifyjs -m -b < tc.js 
bad = function(n) {
    return function(n) {
        try {
            t();              /* <== */
        } catch (t) {
            n(t);
        }
    };
};

[rvanvelzen]

Thanks. I'll take a look at it right now.

[rvanvelzen]

Relevant bit: https://github.com/mishoo/UglifyJS2/blob/593677d/lib/scope.js#L98-L106

I'm going to have to test this carefully to ensure no regression springs up.

[kzc]

I'm going to have to test this carefully to ensure no regression springs up.

Yeah, the uglify scope code is pretty tricky/subtle - due to the complexity of ecmascript no doubt.

The test case in question tests my limited javascript knowledge...

bad = function(e) {
    return function(error) {
        try {
            e();
        } catch (e) {
            error(e);
        }
    };
};

Regarding the variable e used in:

        } catch (e) {
            error(e);
        }

by the ecmascript standard is it supposed to replace the e argument variable of the outer function or is it considered to be an independent variable confined to the catch block scope?

[kzc]

@rvanvelzen Do we try to make a fix for this issue or just enable the screw_ie8 option by default?

[rvanvelzen]

I'd rather do both. I don't feel like we should ship a feature that may break code.

If we are to enable screw_ie8 by default, I'd rather remove it completely and screw IE8 altogether.

[kzc]

@rvanvelzen Have you made any progress on this issue?

I don't know how ECMAscript is supposed to behave regarding a catch var shadowing a var of same name in outer scope. I'm surprised that such code is common in the wild.

[mishoo]

There are more things under the --screw-ie8 umbrella, for instance when this flag is passed, UgliyfJS will convert foo["case"] into foo.case (which is valid by the standard, but will produce a syntax error in IE<=8; that's bad because the script won't even be parsed).

I'd like to stay on the safe side here, so this probably asks for some additional flag. I would, however, turn on by default interpreting the catch clause by the standard (that is, the catch block creates a new variable binding which isn't visible outside). Please give me a few more days.

[rvanvelzen]

I don't know how ECMAscript is supposed to behave regarding a catch var shadowing a var of same name in outer scope. I'm surprised that such code is common in the wild.

The catch parameter is only available inside the catch block. Note that other variables declared inside the catch block are available outside. IE8 does this incorrectly, which is why it's so hard to fix.

Relevant bit of the spec: http://es5.github.io/#x12.14

[lee-houghton]

I've also noticed this. It resulted in a button in our product mysteriously not working despite the code looking correct. It seems like it is definitely the same issue, but I'll post my version anyway, just in case it's different in some way that I don't understand.

Thankfully there is a workaround, but it is easy to slip up and accidentally trigger this behaviour.

In my case it was compiled using Babel:

class MyComponent {
    async onSomeButtonClick(e) {
        e.preventDefault();

        try {
            this.doSomething();
        } catch(e) {
            alert(e.message);
        }
    }
}

When this is compiled to ES5, this becomes something like (it's a little simplified):

MyComponent.prototype.onSomeButtonClick = function(e) {
    return regenerator.async(function f$(context$2$0) {
        e.preventDefault();
        try {
            this.doSomething();
        } catch(e) {
            alert(e.message);
        }
    });
}

The onSomeButtonClick function is mangled differently depending on screw_ie8:

// with figure_out_scope({screw_ie8:true})
// This one seems correct.
MyComponent.prototype.onSomeButtonClick=function(t){return regenerator.async(function(e){t.preventDefault();try{this.doSomething()}catch(t){alert(t.message)}})};

// with figure_out_scope({screw_ie8:false})
MyComponent.prototype.onSomeButtonClick=function(t){return regenerator.async(function(t){e.preventDefault();try{this.doSomething()}catch(e){alert(e.message)}})};

https://tonicdev.com/lee-houghton/570d2d5f10fc0a1100bb7b13

[rvanvelzen]

Maybe we should just warn on shadowed names in catch blocks. It should be obvious (by now) that it's a Bad Thing™ to do.

[lee-houghton]

It may be obvious after reading this thread, sure. But I think this behaviour is very unexpected beforehand; it certainly took me by surprise.

I think a warning would probably suffice for cases like mine, as long as the warning gives some sort of idea of what problem might result from it. If it were just something like "warning: catch block variable shadows variable from enclosing scope", that wouldn't give me any idea that it's going to compile valid code into something that doesn't work at all.

[mislav]

We were just bitten by this at GitHub. Minimal example:

function a(b) {
  try {
  } catch (undefined) {}
  return b === undefined
}

I would expect undefined to be untouched so that the comparison with undefined works. However, undefined gets mangled to c (or a similar generated variable name) and all occurrences of undefined within the a() function scope get rewritten too. The code ultimately fails at runtime because it references a c variable that hasn't been defined.

[kzc]

No doubt it's a bug. undefined is pretty screwed up in javascript in general with it being an identifier not a built-in. I would recommend to stay away from this idiom as far as uglify goes. Best case scenario even if uglify were to leave undefined unmangled would be a larger program compared to catch(x).

[kzc]

Should point out that catch (undefined) works correctly with the --screw-ie8 flag. undefined is just another identifier local to the catch block.

$ cat g.js 
function a(b) {
  try {
  } catch (undefined) {}
  return b === undefined
}

$ bin/uglifyjs -m -- g.js 
function a(t){try{}catch(c){}return t===c}

$ bin/uglifyjs -m --screw-ie8 -- g.js 
function a(n){try{}catch(n){}return n===undefined}

$ cat catch_undefined.js 
"use strict";
(function a(b){
    try {
        throw 'Stuff';
    } catch (undefined) {
        console.log('caught: ' + undefined);
    }
    console.log('undefined is ' + undefined);
    return b === undefined;
})();

This is the desired output:

$ node catch_undefined.js 
caught: Stuff
undefined is undefined

Notice that --support-ie8 (a.k.a. screw_ie8=false) generates bad code:

$ uglifyjs -m --support-ie8 -- catch_undefined.js | node
caught: Stuff
[stdin]:1
"use strict";(function t(o){try{throw"Stuff"}catch(c){console.log("caught: "+c)}console.log("undefined is "+c);return o===c})();
                                                                                                            ^
ReferenceError: c is not defined

Notice that --screw-ie8 generates correct code:

$ uglifyjs -m --screw-ie8 -- catch_undefined.js | node
caught: Stuff
undefined is undefined

[mislav]

I would recommend to stay away from this idiom as far as uglify goes.

Sure, that's what we did when we discovered the bug. The reason we have this idiom in the codebase is that CoffeeScript generates it:

$ echo try a | coffee -bcs
// Generated by CoffeeScript 1.10.0
try {
  a;
} catch (undefined) {}

I can confirm that --screw-ie8 works around this, since use of undefined gets replaced with void 0. We could switch to that flag, since we don't support IE8 on GitHub.com (nor IE9 for that matter). But, it's important for UglifyJS in general to not have bugs that mangle working code into one that fails.

[kzc]

See fix: #1179

Don't support ie8 by default. catch argument is mangled correctly for ES5 standards-compliant JS engines.

[kzc]

This issue can be closed.