Last modified: 2011-02-08 21:57:00 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 T28931, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 26931 - JavaScript minification breaks production when line starts with slash in block comments
JavaScript minification breaks production when line starts with slash in bloc...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
1.18.x
All All
: Normal critical (vote)
: ---
Assigned To: Trevor Parscal
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-25 14:00 UTC by Krinkle
Modified: 2011-02-08 21:57 UTC (History)
3 users (show)

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


Attachments

Description Krinkle 2011-01-25 14:00:24 UTC
The following code
<pre>
/**
 * Foobar:
 * {
 * 'ltr' : {
 * //Multiple rules with configurable operators
 * 'iphone' : false
 * }
 * 
 * @return Boolean true lorem ipsum
 */
</pre>
Is minified as:
<pre>
/**
*Foobar:
*{
*'ltr':{
*//Multiple rules with configurable operators
*'iphone':false
*}
*
*@return Boolean true lorem ipsum
*/
</pre>

The following line specifically is where it all breaks:
<pre>
*//Multiple rules with configurable operators
</pre>

Because it is considered as the closure of the block comment " */ " and the (unexpected) start of a regex.

"Error: unterminated regular expression literal"
Source File: http://translatewiki.net/w/load.php?debug=false&lang=fi&modules=ext.liquidThreads%7Cjquery.autoEllipsis%7Cjquery.checkboxShiftClick%7Cjquery.client%7Cjquery.collapsibleTabs%7Cjquery.cookie%7Cjquery.delayedBind%7Cjquery.highlightText%7Cjquery.makeCollapsible%7Cjquery.placeholder%7Cjquery.suggestions%7Cjquery.tabIndex%7Cjquery.ui.button%7Cjquery.ui.core%7Cjquery.ui.dialog%7Cjquery.ui.draggable%7Cjquery.ui.mouse%7Cjquery.ui.position%7C

Thanks to TranslateWiki Nikerabbit for reporting this on IRC.

I'm confused though that this is production output, why are there comments in production output ?
That empty lines were insterted in place of comments (before the minifier switch by Trevor[0]) to keep track of the line number (although that only applies to the first file of all loaded modules) was one, but keeping all comments ? We might as well disable the minifier then.

--
Krinkle

[0] http://lists.wikimedia.org/pipermail/wikitech-l/2011-January/051308.html
Comment 1 Trevor Parscal 2011-01-25 18:16:18 UTC
This output does not appear to reflex the latest version of the minifier. I tested running the raw versions of these modules through the latest version and no such errors are present.

I think the old minified versions are still in cache, and need to be purged.
Comment 2 Krinkle 2011-01-25 18:44:57 UTC
TWN's sandwiki has been updated to the lastest trunk 2 minutes ago:

http://translatewiki.net/sandwiki/load.php?debug=false&lang=fi&modules=ext.liquidThreads%7Cjquery.client

Search for "*//"
It is still removing the space between * and //, causing the comment block to end early and a regex to be started in a wrong place:

<pre>
/**
*Checks the current browser against a support map object to determine if the browser has been black-listed or
*not.If the browser was not configured specifically it is assumed to work.It is assumed that the body
*element is classified as either"ltr"or"rtl".If neither is set,"ltr"is assumed.
*
*A browser map is in the following format:
*{
*'ltr':{
*//Multiple rules with configurable operators
*'msie':[['>=',7],['!=',9]],
*//Blocked entirely
*'iphone':false
*},
*'rtl':{
*//Test against a string
*'msie':[['!==','8.1.2.3']],
*//RTL rules do not fall through to LTR rules,you must explicity set each of them
*'iphone':false
*}
*}
*
*@param map Object of browser support map
*
*@return Boolean true if browser known or assumed to be supported,false if blacklisted
*/
</pre>

If it is going to keep comments in production mode, it should not touch touch those lines at all.
Comment 3 Trevor Parscal 2011-01-25 18:47:16 UTC
Yeah, I actually am working on a patch - I found a way to reproduce..
Comment 4 Trevor Parscal 2011-01-25 19:01:10 UTC
This should be resolved in r80979 - can someone please update tw.n and verify?
Comment 5 Krinkle 2011-01-25 19:03:30 UTC
Looks fine now. Thx.
Should this be fixed upstream by the way ?
Comment 6 Trevor Parscal 2011-01-25 19:08:12 UTC
Yes, in a few days, if this stablizes a bit, I will push back upstream our code - which the author should be able to make some use of in his JavaScriptPacker.

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


Navigation
Links