Page MenuHomePhabricator

Minify v2.2.0 incorrectly minifies vue.js (faulty line break resulting in early return)
Closed, ResolvedPublic

Description

Minify v2.2.0 minifies the vue ResourceLoader module incorrectly. Where the previous version had

...function kn(e,t){return Array
.isArray(e)...

v2.2.0 produces

...function kn(e,t){return
Array.isArray(e)...

which means the minified function returns early and the rest of its body doesn’t run.

See T277061: Many WikibaseLexeme tests suddenly failing in quibble-vendor-mysql-php72-{selenium,noselenium}-docker for more details.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Krinkle triaged this task as Unbreak Now! priority.
Krinkle added a project: Vue.js Migration.

I'm unable to reproduce this in isolation:

mediawiki/libs/Minify
-   private static $maxLineLength = 1000;
+   private static $maxLineLength = 1;
shell
$ echo 'function kn(e,t){return Array.isArray(e)?e.indexOf(t)>-1:"string"==typeof e?e.split(",").indexOf(t)>-1:(n=e,"[object RegExp]"===a.call(n)&&e.test(t));var n}' | bin/minify js

function
kn
(
e
,
t
)
{
return Array # Correct
.
isArray
(
# […]

[mediawiki/vendor@master] Revert "Update wikimedia/minify to v2.2.0"
https://gerrit.wikimedia.org/r/670790

[mediawiki/core@master] Revert "Update wikimedia/minify to v2.2.0"
https://gerrit.wikimedia.org/r/670789

This happens because the Vue code contains code like e.class, and the minifier code is not aware that it's legal to use class as a property name. Instead, it thinks it's a class definition, and misinterprets all the code that follows it. This doesn't usually result in functions not being recognized, but in this case it does. It's necessary for the e.class property access to be in a parenthesized expression, which is followed later by an object literal, which is then followed by a function declaration without an intervening semicolon. Minimal test case:

(x && y.class); let obj = {} // note: no semicolon
function g() {
    return // note: this line break should be preserved, because of semicolon insertion
        42;
}

This incorrectly minifies to (x&&y.class);let obj={}function g(){return 42;}. The failure to preserve the line break indicates that the minifier does not recognize that the return keyword is in a function body. Consequently, running the same code but with return 42; on one line through the line break test shows that the minifier is willing to break that line, which is what's happening in the Vue code.

This led me to discover another bug that has been in the minifier all along (even before ES6 support was added). ES5 reserved words can also be used as property names, e.g. x.var. The minifier doesn't know this, and treats var as a keyword rather than a property name, which leads to bugs such as:

x.var
++y

being minified to x.var++y, which is a syntax error. If we replace "var" with "foo" in the test case above, the minifier correctly preserves the newline.

Change 671371 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/libs/Minify@master] Fix handling of keywords used as object properties

https://gerrit.wikimedia.org/r/671371

Change 671371 merged by jenkins-bot:
[mediawiki/libs/Minify@master] JavaScriptMinifier: Fix handling of keywords used as object properties

https://gerrit.wikimedia.org/r/671371

Change 672487 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Update wikimedia/minify to 2.2.1

https://gerrit.wikimedia.org/r/672487

Removing this as a train blocker, since the revert Timo did on Thursday unblocked the train.

Change 672487 merged by jenkins-bot:
[mediawiki/core@master] Update wikimedia/minify to 2.2.1

https://gerrit.wikimedia.org/r/672487