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

'comparisons' compression option results in incorrect code with react-mapbox-gl #2011

Closed
davidascher opened this issue May 26, 2017 · 44 comments

Comments

@davidascher
Copy link

davidascher commented May 26, 2017

Bug report. ES5 (transpiled by babel)

Uglify version (uglifyjs -V)

2.8.27

JavaScript input

See this repo: https://github.com/davidascher/mapbox-repro-bug

The original failure is that create-react-app 1.0 production builds that use react-mapbox-gl fail, while development builds work fine (that repo is the minimal such config I could make).

Further debugging indicates that the key difference between the two configurations is the use of the WebpackUglifyJS plugin, which calls out to uglify-js.

(the ejected branch shows the commit which fixes the bug in the production configuration: davidascher/mapbox-repro-bug@b0c95d2)

I'm afraid the bundles are not minimal from an uglify-js point of view, but I'm unable to narrow the test case any further.

Broken live site: https://build-wnyrhmqiph.now.sh/
Working live site: https://build-ijvjggnled.now.sh/

Tracking this bug in CRA as: facebook/create-react-app#2376

@kzc
Copy link
Contributor

kzc commented May 26, 2017

Please post a link to a gist of the final javascript file after all the babeling but before uglify is run on it. Also provide which uglify options are to be used to build it, and identify the section of code in the final javascript input that you believe is transformed incorrectly.

@davidascher
Copy link
Author

I believe this is the JS when it does not run through uglifyjs:

https://gist.github.com/davidascher/da5ec954598cfa1a43c61d429bc4726d

Unfortunately I have no idea which part of the JS input is problematic, and I realize it's large (it contains all of react, etc.).

As to the options used to configure uglify, I don't know what https://github.com/webpack-contrib/uglifyjs-webpack-plugin does, but the webpack configuration in question does:

    new webpack.optimize.UglifyJsPlugin({
      compress: {
        warnings: false,
        reduce_vars: false
      },
      output: {
        comments: false
      },
      sourceMap: true
    }),

I realize this is not a minimal test case from uglify-js's point of view, but it's the best I can do.

@kzc
Copy link
Contributor

kzc commented May 26, 2017

The broken website link above shows this in the error console:

ReferenceError: Can't find variable: e

but with no stack trace or line number.

To aid debugging could you use the following uglify options:

      compress: {
        warnings: true,
        reduce_vars: true
      },
      mangle: false,
      output: {
        comments: false,
        beautify: true
      }

and provide the error, line number and associated code?

Your yarn.lock says it's using uglify-js@3.0.11 but I'm skeptical that an uglify-js 3.x webpack plugin exists. It's more likely this:

uglify-js@^2.6, uglify-js@^2.8.5:
  version "2.8.27"

Could you please run npm ls (or yarn equivalent) and confirm which uglify-js is being run?

@davidascher
Copy link
Author

Thanks for digging in!

For the uglify-js version question:

mapbox-repro@0.1.0 /Users/da/src/react-mapbox-repro/mapbox-repro
├─┬ html-webpack-plugin@2.28.0
│ └─┬ html-minifier@3.5.2
│   └── uglify-js@3.0.11 
├─┬ jest@20.0.3
│ └─┬ jest-cli@20.0.4
│   └─┬ istanbul-api@1.1.8
│     └─┬ istanbul-reports@1.1.0
│       └─┬ handlebars@4.0.10
│         └── uglify-js@2.8.27  deduped
├─┬ sw-precache-webpack-plugin@0.9.1
│ └── uglify-js@2.8.27 
└─┬ webpack@2.6.1
  └── uglify-js@2.8.27  deduped

For the less obfuscated code:

https://build-dxltejnpzf.now.sh is running the code w/ the options you specified.

Uncaught ReferenceError: global is not defined

The line in question is

}).call(this, _dereq_("_process"), void 0 !== global ? global : "undefined" != typeof self ? self : "undefined" != typeof window ? window : {});

which seems to be last line in this large chunk:

(function(process, global) {
                        function inspect(e, r) {
                            var t = {
                                seen: [],
                                stylize: stylizeNoColor
                            };
                            return arguments.length >= 3 && (t.depth = arguments[2]), arguments.length >= 4 && (t.colors = arguments[3]), 
                            isBoolean(r) ? t.showHidden = r : r && exports._extend(t, r), isUndefined(t.showHidden) && (t.showHidden = !1), 
                            isUndefined(t.depth) && (t.depth = 2), isUndefined(t.colors) && (t.colors = !1), 
                            isUndefined(t.customInspect) && (t.customInspect = !0), t.colors && (t.stylize = stylizeWithColor), 
                            formatValue(t, e, t.depth);
                        }
                        function stylizeWithColor(e, r) {
                            var t = inspect.styles[r];
                            return t ? "�[" + inspect.colors[t][0] + "m" + e + "�[" + inspect.colors[t][1] + "m" : e;
                        }
                        function stylizeNoColor(e, r) {
                            return e;
                        }
                        function arrayToHash(e) {
                            var r = {};
                            return e.forEach(function(e, t) {
                                r[e] = !0;
                            }), r;
                        }
                        function formatValue(e, r, t) {
                            if (e.customInspect && r && isFunction(r.inspect) && r.inspect !== exports.inspect && (!r.constructor || r.constructor.prototype !== r)) {
                                var n = r.inspect(t, e);
                                return isString(n) || (n = formatValue(e, n, t)), n;
                            }
                            var i = formatPrimitive(e, r);
                            if (i) return i;
                            var o = Object.keys(r), s = arrayToHash(o);
                            if (e.showHidden && (o = Object.getOwnPropertyNames(r)), isError(r) && (o.indexOf("message") >= 0 || o.indexOf("description") >= 0)) return formatError(r);
                            if (0 === o.length) {
                                if (isFunction(r)) {
                                    var u = r.name ? ": " + r.name : "";
                                    return e.stylize("[Function" + u + "]", "special");
                                }
                                if (isRegExp(r)) return e.stylize(RegExp.prototype.toString.call(r), "regexp");
                                if (isDate(r)) return e.stylize(Date.prototype.toString.call(r), "date");
                                if (isError(r)) return formatError(r);
                            }
                            var c = "", a = !1, l = [ "{", "}" ];
                            if (isArray(r) && (a = !0, l = [ "[", "]" ]), isFunction(r)) {
                                c = " [Function" + (r.name ? ": " + r.name : "") + "]";
                            }
                            if (isRegExp(r) && (c = " " + RegExp.prototype.toString.call(r)), isDate(r) && (c = " " + Date.prototype.toUTCString.call(r)), 
                            isError(r) && (c = " " + formatError(r)), 0 === o.length && (!a || 0 == r.length)) return l[0] + c + l[1];
                            if (t < 0) return isRegExp(r) ? e.stylize(RegExp.prototype.toString.call(r), "regexp") : e.stylize("[Object]", "special");
                            e.seen.push(r);
                            var f;
                            return f = a ? formatArray(e, r, t, s, o) : o.map(function(n) {
                                return formatProperty(e, r, t, s, n, a);
                            }), e.seen.pop(), reduceToSingleString(f, c, l);
                        }
                        function formatPrimitive(e, r) {
                            if (isUndefined(r)) return e.stylize("undefined", "undefined");
                            if (isString(r)) {
                                var t = "'" + JSON.stringify(r).replace(/^"|"$/g, "").replace(/'/g, "\\'").replace(/\\"/g, '"') + "'";
                                return e.stylize(t, "string");
                            }
                            return isNumber(r) ? e.stylize("" + r, "number") : isBoolean(r) ? e.stylize("" + r, "boolean") : isNull(r) ? e.stylize("null", "null") : void 0;
                        }
                        function formatError(e) {
                            return "[" + Error.prototype.toString.call(e) + "]";
                        }
                        function formatArray(e, r, t, n, i) {
                            for (var o = [], s = 0, u = r.length; s < u; ++s) hasOwnProperty(r, String(s)) ? o.push(formatProperty(e, r, t, n, String(s), !0)) : o.push("");
                            return i.forEach(function(i) {
                                i.match(/^\d+$/) || o.push(formatProperty(e, r, t, n, i, !0));
                            }), o;
                        }
                        function formatProperty(e, r, t, n, i, o) {
                            var s, u, c;
                            if (c = Object.getOwnPropertyDescriptor(r, i) || {
                                value: r[i]
                            }, c.get ? u = c.set ? e.stylize("[Getter/Setter]", "special") : e.stylize("[Getter]", "special") : c.set && (u = e.stylize("[Setter]", "special")), 
                            hasOwnProperty(n, i) || (s = "[" + i + "]"), u || (e.seen.indexOf(c.value) < 0 ? (u = isNull(t) ? formatValue(e, c.value, null) : formatValue(e, c.value, t - 1), 
                            u.indexOf("\n") > -1 && (u = o ? u.split("\n").map(function(e) {
                                return "  " + e;
                            }).join("\n").substr(2) : "\n" + u.split("\n").map(function(e) {
                                return "   " + e;
                            }).join("\n"))) : u = e.stylize("[Circular]", "special")), isUndefined(s)) {
                                if (o && i.match(/^\d+$/)) return u;
                                s = JSON.stringify("" + i), s.match(/^"([a-zA-Z_][a-zA-Z_0-9]*)"$/) ? (s = s.substr(1, s.length - 2), 
                                s = e.stylize(s, "name")) : (s = s.replace(/'/g, "\\'").replace(/\\"/g, '"').replace(/(^"|"$)/g, "'"), 
                                s = e.stylize(s, "string"));
                            }
                            return s + ": " + u;
                        }
                        function reduceToSingleString(e, r, t) {
                            var n = 0;
                            return e.reduce(function(e, r) {
                                return n++, r.indexOf("\n") >= 0 && n++, e + r.replace(/\u001b\[\d\d?m/g, "").length + 1;
                            }, 0) > 60 ? t[0] + ("" === r ? "" : r + "\n ") + " " + e.join(",\n  ") + " " + t[1] : t[0] + r + " " + e.join(", ") + " " + t[1];
                        }
                        function isArray(e) {
                            return Array.isArray(e);
                        }
                        function isBoolean(e) {
                            return "boolean" == typeof e;
                        }
                        function isNull(e) {
                            return null === e;
                        }
                        function isNullOrUndefined(e) {
                            return null == e;
                        }
                        function isNumber(e) {
                            return "number" == typeof e;
                        }
                        function isString(e) {
                            return "string" == typeof e;
                        }
                        function isSymbol(e) {
                            return "symbol" == typeof e;
                        }
                        function isUndefined(e) {
                            return void 0 === e;
                        }
                        function isRegExp(e) {
                            return isObject(e) && "[object RegExp]" === objectToString(e);
                        }
                        function isObject(e) {
                            return "object" == typeof e && null !== e;
                        }
                        function isDate(e) {
                            return isObject(e) && "[object Date]" === objectToString(e);
                        }
                        function isError(e) {
                            return isObject(e) && ("[object Error]" === objectToString(e) || e instanceof Error);
                        }
                        function isFunction(e) {
                            return "function" == typeof e;
                        }
                        function isPrimitive(e) {
                            return null === e || "boolean" == typeof e || "number" == typeof e || "string" == typeof e || "symbol" == typeof e || void 0 === e;
                        }
                        function objectToString(e) {
                            return Object.prototype.toString.call(e);
                        }
                        function pad(e) {
                            return e < 10 ? "0" + e.toString(10) : e.toString(10);
                        }
                        function timestamp() {
                            var e = new Date(), r = [ pad(e.getHours()), pad(e.getMinutes()), pad(e.getSeconds()) ].join(":");
                            return [ e.getDate(), months[e.getMonth()], r ].join(" ");
                        }
                        function hasOwnProperty(e, r) {
                            return Object.prototype.hasOwnProperty.call(e, r);
                        }
                        var formatRegExp = /%[sdj%]/g;
                        exports.format = function(e) {
                            if (!isString(e)) {
                                for (var r = [], t = 0; t < arguments.length; t++) r.push(inspect(arguments[t]));
                                return r.join(" ");
                            }
                            for (var t = 1, n = arguments, i = n.length, o = String(e).replace(formatRegExp, function(e) {
                                if ("%%" === e) return "%";
                                if (t >= i) return e;
                                switch (e) {
                                  case "%s":
                                    return String(n[t++]);

                                  case "%d":
                                    return Number(n[t++]);

                                  case "%j":
                                    try {
                                        return JSON.stringify(n[t++]);
                                    } catch (e) {
                                        return "[Circular]";
                                    }

                                  default:
                                    return e;
                                }
                            }), s = n[t]; t < i; s = n[++t]) o += isNull(s) || !isObject(s) ? " " + s : " " + inspect(s);
                            return o;
                        }, exports.deprecate = function(e, r) {
                            function t() {
                                if (!n) {
                                    if (process.throwDeprecation) throw new Error(r);
                                    process.traceDeprecation ? console.trace(r) : console.error(r), n = !0;
                                }
                                return e.apply(this, arguments);
                            }
                            if (isUndefined(global.process)) return function() {
                                return exports.deprecate(e, r).apply(this, arguments);
                            };
                            if (!0 === process.noDeprecation) return e;
                            var n = !1;
                            return t;
                        };
                        var debugEnviron, debugs = {};
                        exports.debuglog = function(e) {
                            if (isUndefined(debugEnviron) && (debugEnviron = process.env.NODE_DEBUG || ""), 
                            e = e.toUpperCase(), !debugs[e]) if (new RegExp("\\b" + e + "\\b", "i").test(debugEnviron)) {
                                var r = process.pid;
                                debugs[e] = function() {
                                    var t = exports.format.apply(exports, arguments);
                                    console.error("%s %d: %s", e, r, t);
                                };
                            } else debugs[e] = function() {};
                            return debugs[e];
                        }, exports.inspect = inspect, inspect.colors = {
                            bold: [ 1, 22 ],
                            italic: [ 3, 23 ],
                            underline: [ 4, 24 ],
                            inverse: [ 7, 27 ],
                            white: [ 37, 39 ],
                            grey: [ 90, 39 ],
                            black: [ 30, 39 ],
                            blue: [ 34, 39 ],
                            cyan: [ 36, 39 ],
                            green: [ 32, 39 ],
                            magenta: [ 35, 39 ],
                            red: [ 31, 39 ],
                            yellow: [ 33, 39 ]
                        }, inspect.styles = {
                            special: "cyan",
                            number: "yellow",
                            boolean: "yellow",
                            undefined: "grey",
                            null: "bold",
                            string: "green",
                            date: "magenta",
                            regexp: "red"
                        }, exports.isArray = isArray, exports.isBoolean = isBoolean, exports.isNull = isNull, 
                        exports.isNullOrUndefined = isNullOrUndefined, exports.isNumber = isNumber, exports.isString = isString, 
                        exports.isSymbol = isSymbol, exports.isUndefined = isUndefined, exports.isRegExp = isRegExp, 
                        exports.isObject = isObject, exports.isDate = isDate, exports.isError = isError, 
                        exports.isFunction = isFunction, exports.isPrimitive = isPrimitive, exports.isBuffer = _dereq_("./support/isBuffer");
                        var months = [ "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" ];
                        exports.log = function() {
                            console.log("%s - %s", timestamp(), exports.format.apply(exports, arguments));
                        }, exports.inherits = _dereq_("inherits"), exports._extend = function(e, r) {
                            if (!r || !isObject(r)) return e;
                            for (var t = Object.keys(r), n = t.length; n--; ) e[t[n]] = r[t[n]];
                            return e;
                        };
                    }).call(this, _dereq_("_process"), void 0 !== global ? global : "undefined" != typeof self ? self : "undefined" != typeof window ? window : {});

but you may prefer to look at the site for the entire stack trace & code.

@kzc
Copy link
Contributor

kzc commented May 26, 2017

I think your issue might not be related to uglify at all.

Please try these options and report back with the error:

      compress: false,
      mangle: false,
      output: {
        comments: false,
        beautify: true
      }

@davidascher
Copy link
Author

Thanks, this is super helpful.

With those options, the site works fine. As soon as I set compress: true it breaks. I did a binary search through the compress options, and it seems that the key one is:

compress: {
  comparison: true
}

which breaks the build (with false fixing it).

Here's the diff of the two JS files: https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d

I'm not sure of the wisdom of stripping the extra = in a bunch of those comparisons, but they seem benign. Some of the transformations seem more likely problematic:

https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d#file-difference-diff-L200

-                        void 0 === i && (i = 1e-6);
+                        "undefined" == typeof i && (i = 1e-6);

https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d#file-difference-diff-L209

-                    void 0 !== module && module.exports ? module.exports = isSupported : window && (window.mapboxgl = window.mapboxgl || {}, 
+                    "undefined" != typeof module && module.exports ? module.exports = isSupported : window && (window.mapboxgl = window.mapboxgl || {}, 

https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d#file-difference-diff-L227

-                    }).call(this, void 0 !== global ? global : "undefined" != typeof self ? self : "undefined" != typeof window ? window : {});
+                    }).call(this, "undefined" !== typeof global ? global : "undefined" !== typeof self ? self : "undefined" !== typeof window ? window : {});

https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d#file-difference-diff-L245

-                    }).call(this, _dereq_("_process"), void 0 !== global ? global : "undefined" != typeof self ? self : "undefined" != typeof window ? window : {});
+                    }).call(this, _dereq_("_process"), "undefined" !== typeof global ? global : "undefined" !== typeof self ? self : "undefined" !== typeof window ? window : {});

with the last one looking a lot like the original problematic line in the traceback.

@davidascher davidascher changed the title React-mapbox-gl in CRA 1.0 shows bug traceable to uglifyjs 'comparisons' compression option results in incorrect code with react-mapbox-gl May 27, 2017
@alexlamsl
Copy link
Collaborator

alexlamsl commented May 27, 2017

So this is against uglify-js@2.8.27 not 3.0 - html-minifier only compress JavaScript embedded within HTML.

@alexlamsl
Copy link
Collaborator

I don't see anything immediately wrong around the four instances of typeof global:

  • first one is eliminiated as dead code as it follows if(true){module.exports=f()}
  • the next two is encapsulated within function(global) { ... }
  • the last one is encapsulated by another function(global) { ... }

@kzc
Copy link
Contributor

kzc commented May 27, 2017

If https://build-dxltejnpzf.now.sh/ is debugged in Chrome you will see that the main thread handles this line fine:

                    }).call(this, _dereq_("_process"), void 0 !== global ? global : "undefined" != typeof self ? self : "undefined" != typeof window ? window : {});

In fact, the debugger shows that global is defined in the main thread - it appears to be a reference to window. This particular variable global is not the global variable global but a captured parent function argument and it must contain a value of some kind - even if it is the value undefined.

However, the same line throws a ReferenceError for global when run as a web service worker thread. In that thread global is not defined for the line in question.

I've never used a web service worker. Are the laws of javascript variable resolution different for them compared to the main thread?

@alexlamsl
Copy link
Collaborator

Web Worker does not have access to window, the equivalent global namespace of which is self.

@alexlamsl
Copy link
Collaborator

Otherwise variable resolution is the same on main or web worker threads. One issue that could have happened is if the main thread sends the code to a worker thread via Function.prototype.toString() and then reconstructing the function on the other side. Now they would have a reference to global in this example, but escaped the function(global){ ... } encapsulation because it's sending the code of the inner function as plain text. But if that's the case, it's out of scope for uglify-js.

@kzc
Copy link
Contributor

kzc commented May 27, 2017

Because the variable global for the line in question was mangled to e in a different incarnation of this program it further demonstrates that it's not the global variable global. Global variables are not mangled by uglify.

Is a web service worker thread effectively forked from the main thread (in the UNIX process sense) and any references to window are uncerimoniously broken leaving them not defined rather than setting them to undefined?

@alexlamsl
Copy link
Collaborator

alexlamsl commented May 27, 2017

Web Worker is a completely isolated execution environment. They share nothing and has to postMessage() between main thread, which objects are copied via structured cloning (except for Transferable Objects which does not require a copy, but can only be accessed by the receiving party after postMessage())

@alexlamsl
Copy link
Collaborator

What I meant in the second half of #2011 (comment) is that in order to "share" a function say function f(){ ... }, the main thread can:

worker.postMessage(f.toString());

... then on the worker end:

addEventListener("message", function(event) {
    eval(event.data);
});

But obviously that would break any scoping that the original function was in.

@alexlamsl
Copy link
Collaborator

The official way to execute code on Web Worker is to pass in the script's URL:

new Worker("path/to/worker.js");

Anything else is just hacking around this limitation. (Blob URLs is a way to "embed" code, but is not supported on all major web browsers.)

@kzc
Copy link
Contributor

kzc commented May 27, 2017

They share nothing ... objects are copied via structured cloned

Now we're getting somewhere - references to window are evidently not copied during that clone process. So the variable global for the line in question in the web worker clone would not be set to undefined, rather it would not be defined, producing the ReferenceError we see above.

Could we separate out the compress optimization for typeof x === "undefined" and put it behind a new compress flag? That way it could be disabled independently of the compress comparisons option.

@kzc
Copy link
Contributor

kzc commented May 27, 2017

worker.postMessage(f.toString());

I knew what you meant. jxcore essentially did that for its thread implementation.

@alexlamsl
Copy link
Collaborator

I don't understand what you mean here - what is being postMessage() in this case that could lead to a variable not being declared at all?

I thought ReferenceError is about undeclared variables, not just undefined.

@alexlamsl
Copy link
Collaborator

references to window are evidently not copied

postMessage() will fail with DataCloneError if you pass DOM object as part of its message parameter - it won't get silently ignored.

@kzc
Copy link
Contributor

kzc commented May 27, 2017

The official way to execute code on Web Worker is to pass in the script's URL:new Worker("path/to/worker.js");

The broken web page was doing something similar to that when I examined the stack. Of course now I can't find that section of code.

@kzc
Copy link
Contributor

kzc commented May 27, 2017

I don't understand what you mean here - what is being postMessage() in this case that could lead to a variable not being declared at all?

I don't think postMessage() of the Function.prototype.toString() is what's happening here.

I thought ReferenceError is about undeclared variables, not just undefined.

Undeclared variables and variables not being defined are the same thing. The value undefined is something altogether different.

@alexlamsl
Copy link
Collaborator

Undeclared variables and variables not being defined are the same thing. The value undefined is something altogether different.

Agreed - was just trying to make sure we are on the same page without confusing terminologies.

@alexlamsl
Copy link
Collaborator

alexlamsl commented May 27, 2017

The broken web page was doing something similar to that when I examined the stack. Of course now I can't find that section of code.

If you can extract the code that new Worker(url) is pointing to, I can have a quick look and see if I can spot anything.

@kzc
Copy link
Contributor

kzc commented May 27, 2017

I think it was a new Worker("path/to/worker") scenario, although not necessarily that exact syntax. I noticed it when stepping through the code earlier. Unable to find when stepping through code now. In Chrome you will see a web worker's URL when it raises the exception.

You can fire up Chrome and take a look at https://build-dxltejnpzf.now.sh

@alexlamsl
Copy link
Collaborator

I went with IE's Developer Tools and got the worker's code copied out, which seems to be similar to https://gist.github.com/davidascher/da5ec954598cfa1a43c61d429bc4726d but somehow some variables are mangled but not others.

worker.js.txt

@kzc
Copy link
Contributor

kzc commented May 27, 2017

Here's where the failing web worker was spawned:

                196: [ function(_dereq_, module, exports) {
                    "use strict";
                    var WebWorkify = _dereq_("webworkify"), window = _dereq_("../window"), workerURL = window.URL.createObjectURL(new WebWorkify(_dereq_("../../source/worker"), {
                        bare: !0
                    }));
                    module.exports = function() {
                        return new window.Worker(workerURL);
                    };
                }, {

@alexlamsl
Copy link
Collaborator

That doesn't look odd, so it's probably fine.

The worker.js I've attached lost its function(global) { ... } encapsulations, so it's definitely going to cause an error. Now the trouble is, reading that file I'm not convinced it's being processed as a whole by uglify-js. It looks like lots of code snippets being thrown at uglify-js, then take pieces of it out and stitch them together as one large file as we see here.

@kzc
Copy link
Contributor

kzc commented May 27, 2017

I still think #2011 (comment) is the explanation for the broken global variable in the web worker.

@alexlamsl
Copy link
Collaborator

I think the current comparisons is a good enough workaround for this invalid assumption that 41 makes - it's got nothing to do with Web Worker.

(Full disclosure: I use Web Workers extensively in one of my projects and I never had any violations with uglify-js, even though I have to postMessag() to reconstruct functions and even parts of the global namespace from the main thread.)

@kzc
Copy link
Contributor

kzc commented May 27, 2017

So I believe I've found the code which creates Blob URLs for new Worker(url):

That also explains the URL of the failing web worker:

blob:https://build-dxltejnpzf.now.sh/d68dc425-c612-4c86-87f0-6db864e89eff

@davidascher
Copy link
Author

Thanks @alexlamsl and @kzc for digging into this, much appreciated!

I believe I've traced the snippet 41 back through mapbox-gl, mapbox-gl-js (https://github.com/mapbox/mapbox-gl-js/blob/b9e10b939c6a3fe5d7ecac209f751b4871970ede/src/util/browser/web_worker.js) to this code in webworkify:

https://github.com/substack/webworkify/blob/master/index.js

I realize this isn't an uglify issue at this point, but can either of you point out what incorrect assumption that code is making, so I can make a useful bug report there?

@alexlamsl
Copy link
Collaborator

@davidascher here's an abstract example of what happened:

var cache = function(G) {
    return {
        m1: function() {
            return void 0 === G;
        }
    };
}({});

// works fine
cache.m1();

// throws error
eval("(" + cache.m1.toString() + ")()");

That last line is what happens when you wrap the code as Blob URL then send it to new Worker(url)

@kzc
Copy link
Contributor

kzc commented May 27, 2017

A simple workaround is to add a "use asm" directive to disable all uglify compress optimizations within the contentious function:

$ cat stringified_asm.js 
var cache = function(G) {
    return {
        m1: function() {
            console.log(1 + 2); // prove that uglify compress is enabled here
            return typeof G === "undefined";
        },
        m2: function() {
            "use asm";
            console.log(1 + 2); // prove that uglify compress is disabled here
            return typeof G === "undefined";
        }
    };
}({});

eval("(" + cache.m2.toString() + ")()");
console.log("SUCCESS");
$ bin/uglifyjs stringified_asm.js -c -b
var cache = function(G) {
    return {
        m1: function() {
            return console.log(3), void 0 === G;
        },
        m2: function() {
            "use asm";
            console.log(1 + 2);
            return typeof G === "undefined";
        }
    };
}({});

eval("(" + cache.m2.toString() + ")()"), console.log("SUCCESS");
$ bin/uglifyjs stringified_asm.js -c -b | node
3
SUCCESS

@davidascher
Copy link
Author

That sounds great, but I'm not sure what the guilty function is.

@kzc
Copy link
Contributor

kzc commented May 28, 2017

On second thought, "use asm" is not a great workaround. Although the code above apparently works in Chrome, Safari and Firefox, it will produce warnings in the Firefox error console. The JavaScript subset for asm.js is not suited for this task.

Perhaps we could consider an uglify specific directive or comment annotation to serve the same purpose.

Edit: But it'd be better to rewrite the code in question to avoid stringifying functions with references to variables outside the scope of the function.

joedeveloper added a commit to joedeveloper/minifier-js that referenced this issue Jun 18, 2017
Problematic issue what would cause people to disable minification entirely.
relates issues in other projects
facebook/create-react-app#2379
sleepycat/old_usesthis@523c872
mapbox/mapbox-gl-js#4359
mishoo/UglifyJS#2011 (comment)
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

3 participants