Last modified: 2011-03-14 14:26:09 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 T29054, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 27054 - JS minification bug in ResourceLoader: ambiguous implied end of statement at end of file can break parsing of subsequent file
JS minification bug in ResourceLoader: ambiguous implied end of statement at ...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
1.18.x
All All
: Normal normal (vote)
: ---
Assigned To: Trevor Parscal
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-31 00:42 UTC by Brion Vibber
Modified: 2011-03-14 14:26 UTC (History)
1 user (show)

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


Attachments
Proposed patch: Add a semicolon to each script before combining (486 bytes, patch)
2011-02-21 18:26 UTC, P.Copp
Details

Description Brion Vibber 2011-01-31 00:42:17 UTC
Noticed this structure while cleaning up SVGEdit; like bug 27046 this involves code that works fine with debug=false, but can end up breaking with minification. Unlike that bug, the whitespace is being preserved ok -- but the breakage occurs nonetheless.

JS allows the end of a statement to be implied in many circumstances, leaving out the final semicolon. This is IMO sloppy, but is standard and is pretty frequent.

I found that the way we're concatenating scripts together can make this break:

Script one:

    var barf = {
    }

Script two:
    (function() {
      console.log('inside');
    })();

Concatenated script:
    var barf = {
    }
    (function() {
      console.log('inside');
    })();

This concatenated script actually gets parsed & run the same as:

    var barf = {
    }(function() {
      console.log('inside');
    })();

that is to say, the wonderful world of JS tries to make a function call on the result of the object literal "{}". This naturally fails.

(Note that the whitespace stripping actually doesn't make a difference here -- you can have as many or few spaces, line breaks, or comments as you like here.

Recommended fix: minification system needs to know when concatenation changes how statement boundaries are parsed and add an explicit semicolon, or else throw an explicit and clear error message.

Workaround: add an explicit semicolon on ambiguous statements.



I've also found that the error from this happening seems to be hidden from the JS console, but I'm not sure if this is just my other wonky stuff going on or not. But it is devilishly hard to track down!

Haven't double-checked with the JSMin system to see if this was already done, but didn't come across this issue before.
Comment 1 Brion Vibber 2011-01-31 00:42:41 UTC
I mean "works fine with debug=true" blahhhhh :D
Comment 2 P.Copp 2011-02-21 18:26:23 UTC
Created attachment 8187 [details]
Proposed patch: Add a semicolon to each script before combining

The fix is quite simple: Every script has to end in a statement anyway, so just add a semicolon after each file, which is in the worst case superfluous, but only adds 1 byte per module to the output.
Comment 3 Roan Kattouw 2011-03-14 14:26:09 UTC
Patch applied in r83897. Thanks!

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


Navigation
Links