mishoo / UglifyJS

JavaScript parser / mangler / compressor / beautifier toolkit
http://lisperator.net/uglifyjs/
Other
13.1k stars 1.25k forks source link

Variable name reuse with mangle off leads to code that fails in some browsers #5932

Closed alexkunin closed 1 week ago

alexkunin commented 1 week ago

Uglify version (uglifyjs -V)

I'm using it as a library, the bug was initially observed with 3.18.0, but then confirmed with 3.19.3 (tried online playground https://skalman.github.io/UglifyJS-online/ and also updated dependency in my project).

JavaScript input

(function () {

async function getReactAppElements(url) {
  return document.createElement('div');
}
const factories = new Map([
  ['name', { factory: () => void 0 }],
]);
(async function mountReactApp(name, url, shadowRoot, interopData = {}) {
    console.log('before', url);
    const elements = await getReactAppElements(url);
    shadowRoot.appendChild(elements);
    const { factory, eventTarget } = await factories.get(name);

    factory();
    console.log('after', url);
})('name', 'url', document.createElement('div'));

}());

And here is output, in any browser:

before "url"
after "url"

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

The only option is mangle: { mangle: false }

JavaScript output or error produced.

!function(){const factories=new Map([["name",{factory:()=>{}}]]);!async function(name,url,shadowRoot){console.log("before",url);var elements=await document.createElement("div"),shadowRoot=(shadowRoot.appendChild(elements),await factories.get(name)).factory;shadowRoot(),console.log("after",url)}("name","url",document.createElement("div"))}();

Same code but formatted:

!function () {
    const factories = new Map([ [ 'name', {
        factory: () => {
        },
    } ] ]);
    !async function (name, url, shadowRoot) {
        console.log('before', url);
        var elements = await document.createElement('div'),
            shadowRoot = (shadowRoot.appendChild(elements), await factories.get(name)).factory;
        shadowRoot(), console.log('after', url);
    }('name', 'url', document.createElement('div'));
}();

For most browsers output is the same, but on iOS 16.4 stock Safari outputs error:

before – "url"
< true
[Error] Unhandled Promise Rejection: TypeError: undefined is not an object (evaluating 'shadowRoot.appendChild')
    Console Evaluation (Console Evaluation 11:3:201)

In my case (i.e. less stripped down version of the code) the problem was a bit different, but the root seems to be the same: reuse of argument name as a var in async context. Apparently, it is some incompatibility of that specific Safari JS engine, here is a code to demonstrate it:

(async function (arg) {
  async function a(x) { return x; }
  console.log('before:', arg);
  var arg = await a(arg), {a: arg} = {a: arg + '-suffix'};
  console.log('after:', arg);
})('initial');

Under normal circumstances output looks like this:

before: initial
after: initial-suffix

But with that specific Safari I get the following:

before: undefined
after: "undefined-suffix"

So, Uglify produces code which generally works, but does not work in combination with certain bug (?) in Safari.

alexlamsl commented 1 week ago

Safari bugs aside (in 2024 no less), I seem to have trouble reproducing your stripped-down example:

$ cat test.js
(async function (arg) {
  async function a(x) { return x; }
  console.log('before:', arg);
  var arg = await a(arg), {a: arg} = {a: arg + '-suffix'};
  console.log('after:', arg);
})('initial');

with the reported version from aforementioned site:

$ uglify-js -v
uglify-js 3.19.0
$ uglify-js test.js --compress --beautify
!async function(arg) {
    console.log("before:", "initial");
    var arg = {
        a: await "initial" + "-suffix"
    }.a;
    console.log("after:", arg);
}();

with latest verson of uglify-js:

$ uglify-js -v
uglify-js 3.19.3
$ uglify-js test.js --compress --beautify
(async arg => {
    console.log("before:", "initial");
    arg = await "initial" + "-suffix";
    console.log("after:", arg);
})();

Note the console.log("before:", "initial"); in both cases.

alexlamsl commented 1 week ago

AFAICT a workaround for this case is to disable merge_vars for now:

{
  compress: {
    merge_vars: false
  }
}
alexlamsl commented 1 week ago

So I tried it on Safari 17.6 and the demo case for their bug can be reduced to:

(async function (a) {
  console.log('before:', a);
  var a = await (async b => b)(a);
  console.log('after:', a);
})('PASS');
before: – undefined
after: – undefined

Simply removing var before a = ... will workaround this:

(async function (a) {
  console.log('before:', a);
  a = await (async b => b)(a);
  console.log('after:', a);
})('PASS');
before: – "PASS"
after: – "PASS"

Looks like that online site has specified module: false − using that and the webkit flag on your original input code seems to do something useful:

$ uglify-js issue-5932.js --no-module --webkit --compress --beautify
!function() {
    const factories = new Map([ [ "name", {
        factory: () => {}
    } ] ]);
    !async function(name, url, shadowRoot) {
        console.log("before", url);
        var elements = await document.createElement("div"), elements = (shadowRoot.appendChild(elements), 
        await factories.get(name)).factory;
        elements(), console.log("after", url);
    }("name", "url", document.createElement("div"));
}();

Note the elements = (shadowRoot... instead of shadowRoot = (shadowRoot... which you had before.

So the solution to this problem seems to have already been implemented, i.e. { webkit: true } which covers all the known Safari quirks. If that is not a case, please holler and I will re-open this report and investigate further.

alexlamsl commented 1 week ago

Specifically, this was reported in #5032 addressed in #5033

alexkunin commented 1 week ago

Safari bugs aside (in 2024 no less)

Well, according to webkit bug tracker, that's not an unheard of. But quirk is also a good term, for sure.

I don't think is resolved, though I'd be glad to be wrong. E.g. take this snippet:

(async function (arg) {
  async function a(x) { return x; }
  console.log('before:', arg);
  var arg = await a(arg), {a: arg} = {a: arg + '-suffix'};
  console.log('after:', arg);
})('initial');

Applying mangle: false and webkit: true (this flag actually does not change output in this case) results in this code (note that 'initial' is now inlined) which works fine in both Safari and Chrome:

!async function(arg) {
    console.log("before:", "initial");
    var arg = {
        a: await "initial" + "-suffix"
    }.a;
    console.log("after:", arg);
}();

But if I force it to avoid inlining (which is what my actual code does -- the argument is not a constant; if constant is long enough, Uglify will also leave it as an argument):

(async function (arg) {
  async function a(x) { return x; }
  console.log('before:', arg);
  var arg = await a(arg), {a: arg} = {a: arg + '-suffix'};
  console.log('after:', arg);
})(window.a || 'initial');

Resulting code becomes this (and turning webkit on or off still does not affect output, as far as I can tell):

!async function(arg) {
    console.log("before:", arg);
    var arg = {
        a: await arg + "-suffix"
    }.a;
    console.log("after:", arg);
}(window.a || "initial");

And now we have same difference in outputs as before. Chrome:

before: initial
after: initial-suffix

Safari:

before: undefined
after: "undefined-suffix"

For now my workaround is to turn off Uglify for file with this specific code.

alexlamsl commented 1 week ago

Your latest example would produce the incorrect output, i.e. on Safari 17.6

(async function (arg) {
  async function a(x) { return x; }
  console.log('before:', arg);
  var arg = await a(arg), {a: arg} = {a: arg + '-suffix'};
  console.log('after:', arg);
})(window.a || 'initial');

gives:

before: – undefined
after: – "undefined-suffix"

without any processing by uglify-js, thus --webkit won't be able to fix anything 😉