Last modified: 2012-04-16 09:15:37 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T29046, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 27046 - JS minification regression in ResourceLoader: comment+linebreak as implied semicolon lost
JS minification regression in ResourceLoader: comment+linebreak as implied se...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
unspecified
All All
: Normal blocker (vote)
: ---
Assigned To: Trevor Parscal
:
Depends on:
Blocks: 26611
  Show dependency treegraph
 
Reported: 2011-01-30 17:46 UTC by Brion Vibber
Modified: 2012-04-16 09:15 UTC (History)
2 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Brion Vibber 2011-01-30 17:46:57 UTC
Found this while tidying up work on SVGEdit; ext.svgedit.embedapi.js get incorrectly minified by the new JSDistiller stuff.

This file is a short bit of (rather messy and comment-filled ;) third-party JS which defines functions for communicated with an embedded iframe. I found I got a syntax error at this point in the original file:

        var t = this //new callback
        for(var g = 0, args = []; g < arguments.length; g++){
          args.push(arguments[g]);
        }

which gets minified to:

        var t=thisfor(var g=0,args=[];g<arguments.length;g++){
        args.push(arguments[g]);
        }

The first line ('var t = this') allows the statement terminator to be implied rather than using an explicit semicolon -- a poor practice, but legitimate in JS. It also uses a double-slash single-line comment on the end.

The combination of these two seems to confuse the new minifier, and it ends up slapping them together with neither a semicolon nor any whitespace at all!

It needs to either keep the line feed intact as a statement terminator, or add a semicolon.

I'll work around it for now by adjusting the formatting in this particular file.

Adding to MW 1.17 blockers (unless we won't be shipping this version of the minifier -- the old code did fine)
Comment 1 Platonides 2011-01-30 22:09:22 UTC
We need minimifier tests.
Comment 2 Roan Kattouw 2011-01-31 12:48:56 UTC
(In reply to comment #0)
> The combination of these two seems to confuse the new minifier, and it ends up
> slapping them together with neither a semicolon nor any whitespace at all!
> 
The real bug here is that the newline after a C++-style // comment is stripped unconditionally, even though the minifier keeps each statement on its own line unless $wgResourceLoaderMinifyJSVerticalSpace is set to true. I have changed it to unconditionally *preserve* such newlines in r81246, and confirmed that this fixes your test case.

> Adding to MW 1.17 blockers (unless we won't be shipping this version of the
> minifier -- the old code did fine)
The fix has been tagged for 1.17 merging.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links