mishoo / UglifyJS

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

Parameter reassigned before `arguments` #5939

Open harai opened 2 months ago

harai commented 2 months ago

Uglify version (uglifyjs -V)

uglify-js 3.19.3

JavaScript input

((e) => {
  const t = e.pushState,
    r = e.replaceState;
  (e.pushState = function (r) {
    const n = new CustomEvent("pushstate", { state: r });
    return window.dispatchEvent(n), t.apply(e, arguments);
  }),
    (e.replaceState = function (t) {
      const n = new CustomEvent("replacestate", { state: t });
      return window.dispatchEvent(n), r.apply(e, arguments);
    }),
    (window.wcNavigation.historyPatched = !0);
})(window.history);

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

$ uglifyjs work/test.js --compress annotations=false,arguments=false,arrows=false,assignments=false,awaits=false,booleans=false,collapse_vars=false,comparisons=false,conditionals=false,dead_code=false,default_values=false,directives=false,evaluate=false,functions=false,hoist_exports=false,hoist_props=false,if_return=false,imports=false,inline=false,join_vars=false,loops=false,merge_vars=true,negate_iife=false,objects=false,properties=false,pure_getters=false,reduce_funcs=false,reduce_vars=true,rests=false,sequences=false,side_effects=false,spreads=false,strings=false,switches=false,templates=false,typeofs=false,unused=true,varify=true,yields=false --keep-fnames -b --output work/test.min.js

Disable most compress options and enable merge_vars, reduce_vars, unused, and varify.

JavaScript output or error produced.

(e => {
    let t = e.pushState, r = e.replaceState;
    e.pushState = function(r) {
        r = new CustomEvent("pushstate", {
            state: r
        });
        return window.dispatchEvent(r), t.apply(e, arguments);
    }, e.replaceState = function(t) {
        t = new CustomEvent("replacestate", {
            state: t
        });
        return window.dispatchEvent(t), r.apply(e, arguments);
    }, window.wcNavigation.historyPatched = !0;
})(window.history);

t is reassigned just before getting its value with arguments keyword, which results in Error: Failed to execute 'replaceState' on 'History': CustomEvent object could not be cloned. error.

alexlamsl commented 2 months ago

Thanks for the report − investigating.

alexlamsl commented 2 months ago

So it turns out to be a strict-mode bug/feature of Node.js / V8, i.e. if you add "use strict"; to the uglified output above, it will produce the correct output.

Which web browser were you running the output code on?

alexlamsl commented 2 months ago

For a quick workaround in your use case, add --no-module to the command line:

$ uglifyjs work/test.js --no-module --compress --keep-fnames -b --output work/test.min.js
harai commented 2 months ago

@alexlamsl

So it turns out to be a strict-mode bug/feature of Node.js / V8, i.e. if you add "use strict"; to the uglified output above, it will produce the correct output.

This page says the same thing:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

Which web browser were you running the output code on?

I was using the latest version of Chrome.

--no-module option seems to be used to support old JS runtime.

The code above is part of WooCommerce 9.3.2. Download the zip file and you will find the code in woocommerce/assets/client/admin/navigation/index.js.

The file was automatically re-minified with UglifyJS during optimization process on my server and caused the issue. The real command used for the auto-minification process is:

uglifyjs "$INPUT_JS" --compress --mangle --keep-fnames --source-map "$SOURCEMAP_CONFIG" --output "$OUTPUT_JS"
alexlamsl commented 2 months ago

--no-module option seems to be used to support old JS runtime.

With latest (& greatest?) ES module "strict" mode is supposed to be implicit:

Also, note that you might get different behavior from sections of script defined inside modules as opposed to in standard scripts. This is because modules use strict mode automatically.

So I guess the reality is that somehow Chrome still runs your code in "sloppy mode", hence the need for --no-module in this case.

magicds commented 2 days ago

Thank you so much for your hard work and for taking the time to respond! I’ve encountered the same issue and understand that it’s related to the behavior of "use strict". However, I believe this could still be considered a minor flaw.

Here’s the original source code:

window.mini_regClass = window.mini.regClass = function (clazz, className) {
    var args = [clazz];
    var exts = mini.getEpointExt(className);
    if ($.isArray(exts) && exts.length) {
        $.each(exts, function (i, ext) {
            args.push(ext);
        });

        mini.overwrite.apply(mini, args);
    }

    return old_regClass.apply(this, arguments);
};

After:

window.mini_regClass = window.mini.regClass = function(t, e) {
    var i = [t],
       t = mini.getEpointExt(e);
    return n.isArray(t) && t.length && (n.each(t, function(t, e) {
        i.push(e)
    }),
    mini.overwrite.apply(mini, i)),
    s.apply(this, arguments)
}

I’d like to suggest a possible improvement to avoid this behavior. Specifically, local variables could use names that don’t overlap with parameter names.

For example, in version 3.13.5(I'm not sure what the correct last minor version is), the result was correct and looked like this:

window.mini_regClass = window.mini.regClass = function (t, e) {
    var i = [t],
        n = mini.getEpointExt(e);
    return (
        aI.isArray(n) &&
            n.length &&
            (aI.each(n, function (t, e) {
                i.push(e);
            }),
            mini.overwrite.apply(mini, i)),
        bI.apply(this, arguments)
    );
};
magicds commented 2 days ago

My source code has a long history—maybe two to three times longer than my own work experience—so I’m not entirely sure how "use strict" might affect it.

I trust that uglify-js is continuously improving, and as long as there are no breaking issues, I’m willing to keep updating it within minor versions.

magicds commented 2 days ago

I didn't notice, maybe I can use {module: false} to avoid it