Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mangling named functions in strict mode can cause syntax errors in Safari #179

Closed
markmevans opened this issue Apr 1, 2013 · 25 comments
Closed

Comments

@markmevans
Copy link

In strict mode, Safari 6.0.3 on OS X 10.8.3 raises an exception if a named function's name is the same as one of the function's argument names (at least in some scenarios). For example:

$ cat <<EOF | ./bin/uglifyjs -  --mangle
> "use strict";
> 
> function f1() {
>   return function f2(a,b,c) {
>   }
> }
> EOF

results in the following code:

"use strict";function f1(){return function n(n,t,u){}}

Safari parses the original code without errors, but the generated code causes the following error:

SyntaxError: Cannot declare a parameter named 'n'  in strict mode

Manually converting the inner function to be anonymous or renaming to something not matching an argument's name resolves the problem.

Also, the generated code runs fine in Chrome and Firefox and Safari does not raise a syntax error for functions declared globally.

@mishoo
Copy link
Owner

mishoo commented Apr 1, 2013

Looks like a bug in Safari, isn't it?

@markmevans
Copy link
Author

@mishoo Seems like it. I haven't found anything in the ECMA-262 5.1 that justifies Safari's behavior, but I am a JavaScript noob so I thought maybe it was a matter of interpretation of the spec. Anyway, I wasn't sure and I thought it might be an issue that others would run into. I ran into this "minimizing" an AngularJS project. The code that's tripping up Safari is in AngularJS so I can't modify it. But it looks like I can work around this by using angular.min.js.

In case you're interested, that file is generated using Google's closure compiler which apparently converts the nested function into an anonymous function since there are no references to the function's name. Something like:

"use strict";function f1(){return function(n,t,u){}}

Maybe that's a worthwhile enhancement?

@mishoo
Copy link
Owner

mishoo commented Apr 2, 2013

It does that if you pass -c (compress).

@mishoo mishoo closed this as completed Apr 2, 2013
@markmevans
Copy link
Author

Facepalm. Thanks.

@markmevans
Copy link
Author

Enabling mangle and compression worked around the Safari bug for me.

uglify: {
  options: {
    mangle: true,
    compress : true
  }
}

On Apr 9, 2013, at 2:31 AM, Eugene Bogomolny notifications@github.com wrote:

Hello!

I use grunt task manager to build the project. I use the latest version of grunt-contrib-uglify. Project on AngularJS. I describe task in grunt:

uglify: {
options: {
compress : false
}
}

but it seems that it does not change anything. And this problem in Safari still exists. Any advise?


Reply to this email directly or view it on GitHub.

@mgcrea
Copy link

mgcrea commented May 7, 2013

Encountering this as well, options: { mangle: true, compress : true } did not work for me, I still get a function r() that breaks safari 6.

This can break many minified scripts, shouldn't this be handled by uglify in some way (through an option maybe), even it the behavior is safari-only?

How can I minify & avoid name collision breaking safari otherwise?

@ashclarke
Copy link

@mgcrea: On a side note (sorry it's not contributing anything):

Thanks for finding this! I found an issue with my own code whereby I was encountering a strict mode exception because I had the following:

Row { // Object 
    show: function show(show) { //code }
}

The parameter name show was conflicting with the named function, but only in Safari (My version happens to be 5.1.7 on Windows).

This bug report saved me a lot of searching time!

@mgcrea
Copy link

mgcrea commented May 18, 2013

@ashclarke Thanks! Found that this error was indeed triggered by this code:

var CountdownFactory = function CountdownFactory(options) {

rvanvelzen added a commit to rvanvelzen/UglifyJS2 that referenced this issue Oct 25, 2013
Fixes mishoo#326 and mishoo#179

It isn't a really clean solution, but it works. The idea is to
"virtually" reference the function name inside the scope. But to be able
know it isn't *really* referenced, we add a `virtual` flag to
`AST_SymbolRef.reference`.
@steeeveb
Copy link

I think I have the same issue using uglify-js 2.4.16 via grunt-contrib-uglify 0.7.0. In my case, the error is on something like

c.prototype.key=function k(k){var a=this._baseState;return f(null===a.key),a.key=k,this}

tested on safari 7 and 8

@steeeveb
Copy link

ok, I found the code that produce this result:

Node.prototype.key = function key(key) {
      var state = this._baseState;

        assert(state.key === null);
          state.key = key;

            return this;
};

It's part of the library added by browserify to my node modules.
Am I doing something wrong?

@steeeveb
Copy link

I tried reserving the word 'key', but the result is the same, because key is also a reserved word in ES5.
So I don't really know if the issue has to resolved with Uglifyjs.
Can I map reserved words with mangle?

@avdg
Copy link
Contributor

avdg commented Jan 27, 2015

I know it's safari bug, but lets try to get the total perspective of ecmascript's keywords

@SteeveB where did you get the claim of key being a reserved word in es5?

Looking up the ecmascript standard: (lets assume the input is always an identifier)

Identifier ::
    IdentifierName **but not** ReservedWord

ReservedWord ::
    Keyword
    FutureReservedWord
    NullLiteral
    BooleanLiteral

Keyword :: **one of**
    break        do          instanceof  typeof
    case         else        new         var
    catch        finally     return      void
    continue     for         switch      while
    debugger     function    this        with
    default      if          throw
    delete       in          try

FutureReservedWord :: **one of**
    class   enum    extends  super
    const   export  import

NullLiteral ::
    **null**

BooleanLiteral ::
    **true**
    **false**

(see http://www.ecma-international.org/ecma-262/5.1/#sec-A.1)

My current guess is that the safari implementation believes that key is a reserved keyword and by that does not agree with what the ecmascript 5 standard says.

Though I will not rule out that previous versions of ecmascript may had it as keyword, but I don't have time yet to find it out.

@ashclarke
Copy link

@steeeveb The problem is not the word "key" as such. As per my comment, #179 (comment), the problem is that your key function has a parameter named key also.

If you had something like Node.prototype.key = function key(theKey) { instead then it would work.

@steeeveb
Copy link

Yes, I confirm. Now I'm online with the minified version modified by hand in that point(a little bit ugly :) )
After looking better, I found that the function "key" is imported by Mathjs(I think inside a dependency)
So searching for a solution with Uglifyjs options is not the right thing. I'll try elsewhere!

thank you!

@ajhsu
Copy link

ajhsu commented Sep 17, 2015

Hi all,

I just tried all of approaches to make my codebase to avoid from Safari's issue.

But i got fail..

I'm currently using gulp-uglify 1.4.1( which uses uglify-js 2.4.24 as dep) to compress with my browserifed codebase.

Althrough I passed compress attribute into options, it still output some named anonymous function with same parameter's name, which will cause Safari error in strict mode.

Here's what i passed into gulp-uglify

{
    mangle: true,
    compress: true
}

And i still can find some named anonymous function with same parameter's name, like:
n.prototype.key=function d(d){
r.prototype.error=function M(e,M){

And I'm thinking reasons may be:

  1. These functions were referenced by name, from somewhere else, so it won't be mangled
  2. These function names were as same as parameter's name BEFORE they were uglified, that will cause uglify output the same name at the end?

Is there anybody suffering the same issue?

@avdg
Copy link
Contributor

avdg commented Sep 17, 2015

Probably time for a --screw-safari option :/

@mhenry1384
Copy link

This was my solution, removing use strict.

function minifyjs(js) {
var ast = jsp.parse(js);
ast = pro.ast_mangle(ast);
ast = pro.ast_squeeze(ast);
var minJs = pro.gen_code(ast);
minJs = minJs.replace(new RegExp('"use strict";', 'g'), '');
return minJs;
}

@avdg
Copy link
Contributor

avdg commented Feb 13, 2016

Hmm... so the problem was caused because safari wanted to pass the whole es 5 spec stuff literally... The restriction of functions to be limited in global functions and thus disabling that restriction is the es 6 way of doing this... There are tests in the test262 es5 suite to test that so that's how I know about this issue. I can lookup for details if really needed (so please ask) but otherwise you have to stick with the information in this comment :-)

In short:

  • es5 doesn't allow functions in functions, es 6 lifted that restriction.
  • safari says ok, I'll do that es5 check thingy (what seems later to be breaking es 6 stuff)
  • other browsers didn't implement the functions in function check and comply to es 6 (maybe they never implemented that check anyway)

@mgol
Copy link
Contributor

mgol commented Apr 23, 2016

A related issue (#1052) has just broken jQuery minification in current (!) Firefox, Safari as well as IE 10.

@adamreisnz
Copy link

I experienced a similar problem due to mistakenly having the following in an AngularJS project:

.factory('CustomerStore', function $instanceStore($q, $log, $instanceStore) {

Which should have simply been:

.factory('CustomerStore', function($q, $log, $instanceStore) {

Hope this helps someone with a similar issue.

@kzc
Copy link
Contributor

kzc commented Aug 25, 2016

This Safari mangling issue was fixed recently: eb63fec

Upgrade to latest uglify-js. (2.7.3 at time of this post).

@PezCoder
Copy link

PezCoder commented Feb 8, 2017

I have upgraded to latest version of gulp-uglify i.e 2.0.1 but I'm still getting the same issue,
SyntaxError: Cannot declare a parameter named 'e' as it shadows the name of a strict mode function.
On minification, this is on the top of my bundled page

!function e(t, o, n) {
    function r(i, s) {
        if (!o[i]) {
            if (!t[i]) {
                var c = "function" == typeof require && require;
                if (!s && c)
                    return c(i, !0);
                if (a)
                    return a(i, !0);
                var l = new Error("Cannot find module '" + i + "'");
                throw l.code = "MODULE_NOT_FOUND", l
            }
            var u = o[i] = {
                exports: {}
            };
            t[i][0].call(u.exports, function(e) {
                var o = t[i][1][e];
                return r(o ? o : e)
            }, u, u.exports, e, t, o, n)
        }
        return o[i].exports
    }
    for (var a = "function" == typeof require && require, i = 0; i < n.length; i++)
        r(n[i]);
    return r
}

How can i fix this, or atleast bypass this ?

@avdg
Copy link
Contributor

avdg commented Feb 8, 2017

@PezCoder you might want to open a separate issue for that as there is no guarantee you are talking about the same issue?

@PezCoder
Copy link

PezCoder commented Feb 8, 2017

@avdg I'm confused on what side this issue persist.. gulp-uglify repo or the uglify itself ?
Thanks for any help

@avdg
Copy link
Contributor

avdg commented Feb 8, 2017

Seems to be uglify

cyjake added a commit to cyjake/UglifyJS2 that referenced this issue Aug 7, 2018
Normally any collision on these names won't be an issue but the Safari on iOS 7/8/9 will yield with a SyntaxError. The problem was discuessed and (partially) fixed in mishoo#179 but function definition mangling still breaks:

```js
$ echo 'function aaa(){function foo(bar){};foo.name=2;return foo}' | ./node_modules/.bin/uglifyjs --compress --mangle
function aaa(){function n(n){}return n.name=2,n}
```

Since symbol names are now [mangled from within](mishoo#2948 (comment)), the mangled argument names can now be appended to `names` cache to make sure the later mangled function name won't collide with them.
cyjake added a commit to cyjake/UglifyJS2 that referenced this issue Aug 8, 2018
Normally any collision on these names won't be an issue but the Safari on iOS 7/8/9 will yield with a SyntaxError. The problem was discuessed and (partially) fixed in mishoo#179 but function definition mangling still breaks:

```js
$ echo 'function aaa(){function foo(bar){};foo.name=2;return foo}' | ./node_modules/.bin/uglifyjs --compress --mangle
function aaa(){function n(n){}return n.name=2,n}
```

Since symbol names are now [mangled from within](mishoo#2948 (comment)), the mangled argument names can now be appended to `names` cache to make sure the later mangled function name won't collide with them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests