Uglifyjs: 3.4.9 breaks an execution order

[kawanet]

msgpack-lite wip branch's test fails when minified with Uglify 3.4.9.
It also passes when minified with Uglify 3.4.8 however.
It looks 3.4.9 breaks something about the order to execute.

JavaScript input

The following function is found at https://github.com/kawanet/msgpack-lite/blob/master/lib/buffer-lite.js

function write(string, offset) {
  var buffer = this;
  var index = offset || (offset |= 0);
  var length = string.length;
  var chr = 0;
  var i = 0;
  while (i < length) {
    chr = string.charCodeAt(i++);

    if (chr < 128) {
      buffer[index++] = chr;
    } else if (chr < 0x800) {
      // 2 bytes
      buffer[index++] = 0xC0 | (chr >>> 6);
      buffer[index++] = 0x80 | (chr & 0x3F);
    } else if (chr < 0xD800 || chr > 0xDFFF) {
      // 3 bytes
      buffer[index++] = 0xE0 | (chr  >>> 12);
      buffer[index++] = 0x80 | ((chr >>> 6)  & 0x3F);
      buffer[index++] = 0x80 | (chr          & 0x3F);
    } else {
      // 4 bytes - surrogate pair
      chr = (((chr - 0xD800) << 10) | (string.charCodeAt(i++) - 0xDC00)) + 0x10000;
      buffer[index++] = 0xF0 | (chr >>> 18);
      buffer[index++] = 0x80 | ((chr >>> 12) & 0x3F);
      buffer[index++] = 0x80 | ((chr >>> 6)  & 0x3F);
      buffer[index++] = 0x80 | (chr          & 0x3F);
    }
  }
  return index - offset;
}

The uglifyjs CLI command

uglifyjs -c -m -o min.js buffer-lite.js

Uglify 3.4.8 🆗

function write(r, t) {
  for (var e = this, h = t || (t |= 0), n = r.length, o = 0, a = 0; a < n;)
    (o = r.charCodeAt(a++)) < 128 ? e[h++] = o
      : (o < 2048 ? e[h++] = 192 | o >>> 6
      : (o < 55296 || 57343 < o ? e[h++] = 224 | o >>> 12
        : (o = 65536 + (o - 55296 << 10 | r.charCodeAt(a++) - 56320), e[h++] = 240 | o >>> 18, e[h++] = 128 | o >>> 12 & 63), e[h++] = 128 | o >>> 6 & 63), e[h++] = 128 | 63 & o);
  return h - t
}

Uglify 3.4.9 🆖

function write(r, t) {
  for (var e = this, h = t || (t |= 0), n = r.length, o = 0, a = 0; a < n;)
    o = r.charCodeAt(a++), e[h++] = o < 128 ? o
      : (e[h++] = o < 2048 ? 192 | o >>> 6
        : (e[h++] = o < 55296 || 57343 < o ? 224 | o >>> 12
          : (o = 65536 + (o - 55296 << 10 | r.charCodeAt(a++) - 56320), e[h++] = 240 | o >>> 18, 128 | o >>> 12 & 63), 128 | o >>> 6 & 63), 128 | 63 & o);
  return h - t
}

It looks e[h++] = o is moved to wrong positions.

[kawanet]

duplicated #3271

[oliversalzburg]

I noticed the same issue with SheetJS/js-xlsx

All UTF8 characters will have their bytes reversed when exporting files with an uglified bundle.

[oliversalzburg]

@alexlamsl Is there anything you can do for us here?

[kawanet]

I confirmed the pull request "enhance conditionals (#3243)" made this bug happen.

[pedrofurtado]

@alexlamsl Is there any plans to release a minor version fixing that? That is bug with high priority to solve. And, thanks @kawanet for posting the scenario where this issues occurs! Here we are having the same problem.

[oliversalzburg]

@pedrofurtado Given the age of this issue, I doubt there is any interest at all to do anything

[pedrofurtado]

A workaround, for now, is to disable "conditionals" optimization when using UglifyJS in version 3.4.9. Suggestion from: https://github.com/mishoo/UglifyJS2/issues/3288#issuecomment-433457397

[nylen]

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.

[neelance]

This bug has caused us a lot of headache. It makes special characters have their bytes reversed when saving them to Firebase, but only if Firebase falls back to long polling, which made this bug happen only sometimes for our users. We couldn't reproduce it ourselves and it took a long time to track this down.

I knew that it was due to some updated modules and I updated them a few more times in the hope that a fix would have been released in the meantime, but to no avail. Now I see that this serious bug has already been filed for two months! (#3245) I have to say that I am disappointed that such a widely used module lets this happen, especially on a patch release. At least a quick revert of the breaking commit would have been in order.

[kzc]

Github users have an inflated sense of entitlement. Don't blame the people who generously volunteer their time producing and maintaining open source software. No one can be expected to support software for free forever.

Ultimately it's up to each project to test their applications. There is no guarantee. All software has bugs. Projects come and go. You have the freedom to disable any or all minification options, lock in any version, fork and maintain this or any other project (license permitting), or switch to an alternative.

If anything this exercise points to a deficiency in the node package ecosystem. There is no way to flag issues with certain versions of packages other than to file a bug report with its author. There ought to be a way in npm and yarn to upvote/downvote package versions and a warning could accompany package installation once warnings as a percentage of installations of a particular version reaches a certain threshold. The user could then take appropriate action if they see fit.

[oliversalzburg]

@kzc While I agree with you in most parts, when the community finds an issue in your software, at least a response like "Thanks for letting me know, but I'm busy for the rest of the year. Sorry about that" or "Please submit a PR and I will merge it" would help in understanding why there is no movement in this issue. I don't think anyone expects to be served freely with fixes for the remainder of their live, but no response at all frustrates people.

We actually implemented a version locking approach into our own dependency management tool to allow us to better handle situations like this in the future.

[kzc]

OSS is a thankless task. I don't fault any package maintainer for walking away without explanation. Instead, I appreciate their effort. "Community" is an overrated term. People only used this software because it was better than the alternatives at the time. There are software producers and free downloaders. If users took a minute to read the uglify issues they could have seen and easily avoided the problems and used any of the workarounds listed above.

[nylen]

Github users have an inflated sense of entitlement.

Hi, and welcome to GitHub!

Don't blame the people who generously volunteer their time producing and maintaining open source software.

When software maintainers, open source or not, don't act in a professional manner, I think it is appropriate to take the following steps, in the following order:

1. Report bugs and try to fix them. (I and others have already done this. In this case there is obviously no one around to accept PRs and release a new version, otherwise we would not be still having this issue.)
2. Call the maintainers out on the problem, with specific suggestions and recommendations for improvement.
3. Look for alternatives, explain why you are doing so and recommend that others do the same.

No one can be expected to support software for free forever. [...] OSS is a thankless task. I don't fault any package maintainer for walking away without explanation.

This is mostly true. I have been an OSS maintainer of various projects for many years. However, I don't agree that it's a thankless task.

When we release software, we take on various responsibilities. Among them: ensuring that the software works as intended for its users, and generally being a good steward of our communities.

Interpretations of these responsibilities vary. However, this much is clear: if professional users depend on a software product that is being released and maintained in an unprofessional manner, then this either needs to be corrected by improving the release process, or the users need to find alternative software.

And again, I can't speak for others, but when I no longer have time to contribute to a project, I do my best to leave it in the hands of someone capable who will continue to look after its community.

At this point I'm going to be moving away from uglify-js to https://github.com/terser-js/terser and I recommend that any others who are frustrated with this issue do the same.

Instead, I appreciate [package maintainers'] effort.

I appreciate results more than misplaced effort. We're having this conversation today because the maintainers of uglify-js screwed up, and it's not the first time.

This is not entitlement, it is a simple statement of cause and effect as it relates to my livelihood as a professional software developer.

[kzc]

I appreciate results more than misplaced effort. We're having this conversation today because the maintainers of uglify-js screwed up, and it's not the first time.

Comments like that drive people away from wanting to contribute to OSS. You are using this software for free and are owed nothing.

@alexlamsl did an outstanding job as maintainer. He redesigned and simplified the minify API for uglify-js@3, implemented a raft of advanced optimizations to further reduce code size, improved the test suite with real world code, implemented and deployed an ES5 fuzzer which ran 24/7, and often fixed bugs the same day with weekly releases for almost two years.

At this point I'm going to be moving away from uglify-js to https://github.com/terser-js/terser and I recommend that any others who are frustrated with this issue do the same.

Wow, you really went out on a limb there recommending an uglify-js fork maintained by a couple of the same core contributors, of which I am one. uglify-es and its successor terser would not exist without the great work of the uglify-js' maintainers.

[nylen]

Thanks for all your contributions, both here and on terser.

[oliversalzburg]

And thanks for your insightful comments too!

[neelance]

@kzc I'm the author of some popular open source projects, too. I don't have the time to maintain them all, so I got in touch with the community and took care of finding new primary maintainers for the projects. If there would be a change that breaks a project for many of its users, then I would at least put in 5 minutes to revert the change. That's all it takes. I wouldn't have to actually fix the broken change. In my opinion you do have at least some responsibility for the lost time of other people that you cause by breaking your open source project. They put trust in you and you should be responsible with this trust. This does not contradict with me being thankful for the open source work that other people do.

Btw: We switched to terser and it works great.

[neelance]

If users took a minute to read the uglify issues they could have seen and easily avoided the problems and used any of the workarounds listed above.

This is not true in our case. The way the bug happened to us and wasn't reproducible had me suspect other components first. I suspected Vue.js and Firebase as likely culprits and only when a coworker of mine could reproduce the problem because of an internet hickup I was able to see that it was caused by reverse code order in production.

[kzc]

@neelance We'll have to agree to disagree. Users are responsible for reviewing the issues of any package before upgrading and to continue monitoring the issues once they've upgraded. All users would be well advised to lock down their package versions in yarn and npm. All software has bugs - whether the project is maintained or not. Users of this or any other OSS project are owed nothing and are not forced to use the software. They are responsible for testing their own applications after upgrading any of their packages. The LICENSE file is there for a reason.

[neelance]

I'm currently thinking a lot about this and I am somewhat confused... I don't want to say that the author of a project owes something. But on the other hand it doesn't feel right to me to just say every behavior is equal. Are you saying that there should be zero trust? Even if someone does a great job like you said @alexlamsl did, I should still not trust him? How can we build advanced projects without such trust? Especially in the NPM world it is quite infeasible to track all issues of all transitive dependencies.

[kzc]

Are you saying that there should be zero trust?

I wouldn't phrase it exactly that way, but yes, people should not blindly trust any package or package upgrade. There's always been bugs in this and every other OSS project. You are ultimately responsible for testing your own applications. A million or so people per day downloaded uglify-js because they considered it the most reliable and convenient option at the time. If people are uncomfortable with software maintained by just one or two individuals they should choose an alternative - assuming that one exists at all. The only benefit that a large "community" of users imparts is that bugs are reported more quickly and workarounds are more readily shared.

The introduction of this bug was not intentional. It was created in an effort to add a new optimization. All new features have the possibility of introducing a bug. Uglify is highly customizable and offers granularity to enable or disable any optimization according to each user's risk tolerance.

Especially in the NPM world it is quite infeasible to track all issues of all transitive dependencies.

It's feasible. I do it for my own projects. You have to ability to lock down any package version in the hierarchy or even swap them out with custom versions using yarn resolutions.

[puzrin]

I confirm regression in 3.4.8 -> 3.4.9

https://github.com/nodeca/pako/issues/161#issuecomment-468420555

The same problem with execution order.