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

"Enhance conditionals" change creating invalid output #3269

Closed
Jimbly opened this issue Oct 5, 2018 · 4 comments · Fixed by #3329
Closed

"Enhance conditionals" change creating invalid output #3269

Jimbly opened this issue Oct 5, 2018 · 4 comments · Fixed by #3329

Comments

@Jimbly
Copy link
Contributor

Jimbly commented Oct 5, 2018

Bug report

An optimization in v3.4.9 seems to be moving an array assignment to before the array is created.

Bisected the problem to 2bdaca1 (#3243).

Uglify version (uglifyjs -V)

uglify-js 3.4.9

JavaScript input

Input (simplified from some code in Turbulenz that trigger this):

function v3Build(r, dst) {
  var res;
  var length = arguments.length;
  if (length >= 3) {
    if (length > 3) {
      res = arguments[3];
      if (res === undefined) {
        res = new Float32Array(3);
      }
    } else {
      res = new Float32Array(3);
    }

    res[0] = arguments[0];
    res[1] = arguments[1];
    res[2] = arguments[2];
  } else {
    res = dst;
    if (res === undefined) {
      res = new Float32Array(3);
    }

    res[0] = r[0];
    res[1] = r[1];
    res[2] = r[2];
  }

  return res;
}

console.log(v3Build([1,2,3]));

Expected output: Float32Array [ 1, 2, 3 ]

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

JavaScript output or error produced.

Generated code (with --beautify for clarity):

function v3Build(r, dst) {
    var res, length = arguments.length;
    return res[2] = 3 <= length ? (3 < length ? void 0 === (res = arguments[3]) && (res = new Float32Array(3)) : res = new Float32Array(3), 
    res[0] = r, res[1] = dst, arguments[2]) : (void 0 === (res = dst) && (res = new Float32Array(3)), 
    res[0] = r[0], res[1] = r[1], r[2]), res;
}

console.log(v3Build([ 1, 2, 3 ]));

Running output:

out.js:3
    return res[2] = 3 <= length ? (3 < length ? void 0 === (res = arguments[3]) && (res = n...
               ^

TypeError: Cannot set property '2' of undefined
    at v3Build (out.js:3:19)
    at Object.<anonymous> (out.js:8:13)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:612:3
@nylen
Copy link

nylen commented Oct 9, 2018

Confirmed working in 3.4.8 and broken in 3.4.9 for me too, @alexlamsl it looks like there is a major bug in 3.4.9.

Input:

var modelClassName,
	routeName  = '',
	parentName = '',
	routeEnd   = '';
if ( parentName === 'x' ) {
	parentName = '';
}
if ( 'me' === routeEnd ) {
	routeName = 'me';
}
if ( '' !== parentName && parentName !== routeName ) {
	modelClassName = parentName + routeName;
	modelClassName = mapping.models[ modelClassName ] || modelClassName;
	loadingObjects.models[ modelClassName ] = thing();
} else {
	modelClassName = routeName;
	modelClassName = mapping.models[ modelClassName ] || modelClassName;
	loadingObjects.models[ modelClassName ] = thing();
}

Output with -c -m -b options (note that loadingObjects.models[modelClassName] is executed before modelClassName is assigned):

var modelClassName, routeName = "", parentName = "", routeEnd = "";

"x" === parentName && (parentName = ""), "me" === routeEnd && (routeName = "me"),
loadingObjects.models[modelClassName] = (modelClassName = (modelClassName = "" !== parentName && parentName !== routeName ? parentName + routeName : routeName,
mapping.models[modelClassName] || modelClassName), thing());

@nylen
Copy link

nylen commented Nov 9, 2018

Releasing a new version and then disappearing is bad form. Please follow up with your community after the release.

If you don't have time to do that, better not to do the release in the first place.

@panost
Copy link

panost commented Nov 9, 2018

Someone at least, remove v3.4.9 from here and npm. It is a dangerous version

@oliverwoodings
Copy link

Very surprised to see that this still has not been rolled back. We unfortunately did not catch the issue until it entered production code (it did not manifest itself as a code exception, but instead as data mutation), by which point it had triggered a major incident for us.

edmundoa pushed a commit to Graylog2/graylog2-server that referenced this issue Jan 16, 2019
Due to a bug in uglify-js 3.4.9 compressing conditionals during asset
minization is broken, leading to #5450.

This commit disables conditionals compression until we can test it is fixed
in a future uglify-js release.

Here is a similar issue reported in upstream project:
mishoo/UglifyJS#3269

Fixes #5450
edmundoa pushed a commit to Graylog2/graylog2-server that referenced this issue Jan 16, 2019
Due to a bug in uglify-js 3.4.9, compressing conditionals during asset
minimization is broken, leading to #5450.

This commit disables conditionals compression until we can test it is fixed
in a future uglify-js release.

There are several reports in the upstream project, one of them is:
mishoo/UglifyJS#3269

Fixes #5450
bernd pushed a commit to Graylog2/graylog2-server that referenced this issue Jan 16, 2019
Due to a bug in uglify-js 3.4.9, compressing conditionals during asset
minimization is broken, leading to #5450.

This commit disables conditionals compression until we can test it is fixed
in a future uglify-js release.

There are several reports in the upstream project, one of them is:
mishoo/UglifyJS#3269

Fixes #5450
edmundoa added a commit to Graylog2/graylog2-server that referenced this issue Jan 17, 2019
Due to a bug in uglify-js 3.4.9, compressing conditionals during asset
minimization is broken, leading to #5450.

This commit disables conditionals compression until we can test it is fixed
in a future uglify-js release.

There are several reports in the upstream project, one of them is:
mishoo/UglifyJS#3269

Fixes #5450

(cherry picked from commit 28993f9)
bernd pushed a commit to Graylog2/graylog2-server that referenced this issue Jan 17, 2019
Due to a bug in uglify-js 3.4.9, compressing conditionals during asset
minimization is broken, leading to #5450.

This commit disables conditionals compression until we can test it is fixed
in a future uglify-js release.

There are several reports in the upstream project, one of them is:
mishoo/UglifyJS#3269

Fixes #5450

(cherry picked from commit 28993f9)
alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Mar 12, 2019
alexlamsl added a commit that referenced this issue Mar 12, 2019
fixes #3245
fixes #3257
fixes #3260
fixes #3269
fixes #3271
fixes #3278
fixes #3309
fixes #3319
fixes #3321
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

Successfully merging a pull request may close this issue.

4 participants