UglifyJS icon indicating copy to clipboard operation
UglifyJS copied to clipboard

Parameter reassigned before `arguments`

Open harai opened this issue 1 year ago • 10 comments
trafficstars

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.

harai avatar Sep 25 '24 06:09 harai

Thanks for the report − investigating.

alexlamsl avatar Sep 28 '24 19:09 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.

Which web browser were you running the output code on?

alexlamsl avatar Sep 28 '24 19:09 alexlamsl

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

alexlamsl avatar Sep 28 '24 19:09 alexlamsl

@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"

harai avatar Sep 29 '24 01:09 harai

--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.

alexlamsl avatar Sep 29 '24 04:09 alexlamsl

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 avatar Nov 28 '24 09:11 magicds

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 avatar Nov 28 '24 09:11 magicds

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

magicds avatar Nov 28 '24 10:11 magicds

I have meet the same issue. And I try to make a minimize demo to explain this issue.

code

// demo.js
"use strict";
function add(aa, bb) {
    var cc = [aa, bb].join('.');
    console.log(cc, arguments);
}
# uglify-js 3.19.3
# uglify command
$ npx uglifyjs demo.js --compress --output demo.min.js
// demo.min.js
function add(aa,bb){aa=[aa,bb].join(".");console.log(aa,arguments)}

result

the frist function argument called aa is reassigned; and function context variable called arguments is changed. image

image

It sance not about "use strict". Hoping be fixed, my program is broken.

CntChen avatar Dec 12 '24 16:12 CntChen

--compress merge_vars=false is useful to resolve this case.

merge_vars (default: true) — combine and reuse variables.

CntChen avatar Dec 12 '24 19:12 CntChen