Page MenuHomePhabricator

Possible bug in minifier: exponents can be incorrectly broken across lines
Closed, ResolvedPublic

Description

When I visit the above url I get:
load.php:682 Uncaught SyntaxError: Unexpected token -

I don't get an error with debug=true


Version: 1.20.x
Severity: normal
URL: https://translatewiki.net/wiki/Map_of_translators

Details

Reference
bz32548

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:04 AM
bzimport set Reference to bz32548.
brion added a comment.Nov 21 2011, 6:51 PM

Error I see in Firefox is:

missing exponent
https://translatewiki.net/w/load.php?debug=false&lang=en&modules=ext.maps.common%2Copenlayers&skin=vector&version=20111120T223129Z&*
Line 681

original source (https://translatewiki.net/w/extensions/Maps/includes/services/OpenLayers/OpenLayers/OpenLayers.js line 1483):

OpenLayers.Layer.MultiMap=OpenLayers.Class(OpenLayers.Layer.EventPane,OpenLayers.Layer.FixedZoomLevels,{MIN_ZOOM_LEVEL:1,MAX_ZOOM_LEVEL:17,RESOLUTIONS:[9,1.40625,0.703125,0.3515625,0.17578125,0.087890625,0.0439453125,0.02197265625,0.010986328125,0.0054931640625,0.00274658203125,0.001373291015625,6.866455078125E-4,3.4332275390625E-4,1.71661376953125E-4,8.58306884765625E-5,4.291534423828125E-5],type:null,initialize:function(a,b){OpenLayers.Layer.EventPane.prototype.initialize.apply(this,arguments);

Minified broken (https://translatewiki.net/w/load.php?debug=false&lang=en&modules=ext.maps.common%2Copenlayers&skin=vector&version=20111120T223129Z&* line 681-682):

"Scale = 1 : ${scaleDenom}":"Escala = 1 : ${scaleDenom}",W:"O",E:"E",N:"N",S:"S",reprojectDeprecated:"Est\u00e1 usando a op\u00e7\u00e3o 'reproject' na camada ${layerName}. Esta op\u00e7\u00e3o \u00e9 obsoleta: foi concebida para permitir a apresenta\u00e7\u00e3o de dados sobre mapas-base comerciais, mas esta funcionalidade \u00e9 agora suportada pelo Mercator Esf\u00e9rico. Mais informa\u00e7\u00e3o est\u00e1 dispon\u00edvel em http://trac.openlayers.org/wiki/SphericalMercator.",methodDeprecated:"Este m\u00e9todo foi declarado obsoleto e ser\u00e1 removido na vers\u00e3o 3.0. Por favor, use ${newMethod} em vez disso."});OpenLayers.Layer.MultiMap=OpenLayers.Class(OpenLayers.Layer.EventPane,OpenLayers.Layer.FixedZoomLevels,{MIN_ZOOM_LEVEL:1,MAX_ZOOM_LEVEL:17,RESOLUTIONS:[9,1.40625,0.703125,0.3515625,0.17578125,0.087890625,0.0439453125,0.02197265625,0.010986328125,0.0054931640625,0.00274658203125,0.001373291015625,6.866455078125E-4,3.4332275390625E-4,1.71661376953125E-4,8.58306884765625E
-5,4.291534423828125E-5],type:null,initialize:function(a,b)

^ note the line break between "8.58306884765625E" and "-5".

brion added a comment.Nov 21 2011, 7:25 PM

I suspect it's hitting this line-wrapping bit:

} elseif( $maxLineLength > 0 && $lineLength + $end - $pos > $maxLineLength &&

		!isset( $semicolon[$state][$type] ) && $type !== self::TYPE_INCR_OP )

{
This line would get too long if we added $token, so add a newline first.
Only do this if it won't trigger semicolon insertion and if it won't
// put a postfix increment operator on its own line, which is illegal in js.
$out .= "\n";
$lineLength = 0;

which probably indicates that the tokenizer stage is incorrectly splitting numbers with exponents into two or three tokens, where the number part of the exponent is separate and ends up getting pushed to the next line.

The entire number, including the E, sign, and exponent, should come out as single token.

Well isn't this suspicious!

Numeric literal. Search for the end of it, but don't care about [+-]exponent
at the end, as the results of "numeric [+-] numeric" and "numeric" are
// identical to our state machine.

Working on a test case...

Added PHPUnit test cases in 103846. This will trigger 2 test failures until the bug is resolved.

Created attachment 9518
Provisional patch to JavaScriptMinifier.php

Passes the PHPUnit test cases; want to test it on actual code though :D

attachment work ignored as obsolete

Created attachment 9519
Provisional patch to JavaScriptMinifier.php, fixed

removed stray edits to other files

Attached:

Ok tested it with the same file that broke on translatewiki (from Maps extension).

http://stormcloud.local/trunk/load.php?debug=false&lang=en&modules=ext.maps.common%2Copenlayers&skin=vector&version=20111120T223129Z

With the original code I see at line 681/682:

"Scale = 1 : ${scaleDenom}":"Escala = 1 : ${scaleDenom}",W:"O",E:"E",N:"N",S:"S",reprojectDeprecated:"Est\u00e1 usando a op\u00e7\u00e3o 'reproject' na camada ${layerName}. Esta op\u00e7\u00e3o \u00e9 obsoleta: foi concebida para permitir a apresenta\u00e7\u00e3o de dados sobre mapas-base comerciais, mas esta funcionalidade \u00e9 agora suportada pelo Mercator Esf\u00e9rico. Mais informa\u00e7\u00e3o est\u00e1 dispon\u00edvel em http://trac.openlayers.org/wiki/SphericalMercator.",methodDeprecated:"Este m\u00e9todo foi declarado obsoleto e ser\u00e1 removido na vers\u00e3o 3.0. Por favor, use ${newMethod} em vez disso."});OpenLayers.Layer.MultiMap=OpenLayers.Class(OpenLayers.Layer.EventPane,OpenLayers.Layer.FixedZoomLevels,{MIN_ZOOM_LEVEL:1,MAX_ZOOM_LEVEL:17,RESOLUTIONS:[9,1.40625,0.703125,0.3515625,0.17578125,0.087890625,0.0439453125,0.02197265625,0.010986328125,0.0054931640625,0.00274658203125,0.001373291015625,6.866455078125E-4,3.4332275390625E-4,1.71661376953125E-4,8.58306884765625E
-5,4.291534423828125E-5],type:null,initialize:function(a,b){OpenLayers.Layer.EventPane.prototype.initialize.apply(this,arguments);OpenLayers.Layer.FixedZoomLevels.prototype.initialize.apply(this,arguments);this.sphericalMercator&&(OpenLayers.Util.extend(this,OpenLayers.Layer.SphericalMercator),this.initMercatorParameters(),this.RESOLUTIONS.unshift(10))},loadMapObject:function(){try{this.mapObject=new MultimapViewer(this.div)}catch(a){}},getWarningHTML:function(){return OpenLayers.i18n("getLayerWarning",{layerType:"MM",layerLib:"MultiMap"})},setMapObjectCenter:function(a,b){this.mapObject.goToPosition(a,b)},getMapObjectCenter:function(){return this.mapObject.getCurrentPosition()},getMapObjectZoom:function(){return this.mapObject.getZoomFactor()},getMapObjectLonLatFromMapObjectPixel:function(a){a.x-=this.map.getSize().w/2;a.y-=this.map.getSize().h/2;return this.mapObject.getMapPositionAt(a)},getMapObjectPixelFromMapObjectLonLat:function(a){return this.mapObject.geoPosToContainerPixels(a)

^ that's split between the 'E' and the '-5', as we saw before.

With the patch in & memcached cleared, reload & we get:

"Scale = 1 : ${scaleDenom}":"Escala = 1 : ${scaleDenom}",W:"O",E:"E",N:"N",S:"S",reprojectDeprecated:"Est\u00e1 usando a op\u00e7\u00e3o 'reproject' na camada ${layerName}. Esta op\u00e7\u00e3o \u00e9 obsoleta: foi concebida para permitir a apresenta\u00e7\u00e3o de dados sobre mapas-base comerciais, mas esta funcionalidade \u00e9 agora suportada pelo Mercator Esf\u00e9rico. Mais informa\u00e7\u00e3o est\u00e1 dispon\u00edvel em http://trac.openlayers.org/wiki/SphericalMercator.",methodDeprecated:"Este m\u00e9todo foi declarado obsoleto e ser\u00e1 removido na vers\u00e3o 3.0. Por favor, use ${newMethod} em vez disso."});OpenLayers.Layer.MultiMap=OpenLayers.Class(OpenLayers.Layer.EventPane,OpenLayers.Layer.FixedZoomLevels,{MIN_ZOOM_LEVEL:1,MAX_ZOOM_LEVEL:17,RESOLUTIONS:[9,1.40625,0.703125,0.3515625,0.17578125,0.087890625,0.0439453125,0.02197265625,0.010986328125,0.0054931640625,0.00274658203125,0.001373291015625,6.866455078125E-4,3.4332275390625E-4,1.71661376953125E-4,
8.58306884765625E-5,4.291534423828125E-5],type:null,initialize:function(a,b){OpenLayers.Layer.EventPane.prototype.initialize.apply(this,arguments);OpenLayers.Layer.FixedZoomLevels.prototype.initialize.apply(this,arguments);this.sphericalMercator&&(OpenLayers.Util.extend(this,OpenLayers.Layer.SphericalMercator),this.initMercatorParameters(),this.RESOLUTIONS.unshift(10))},loadMapObject:function(){try{this.mapObject=new MultimapViewer(this.div)}catch(a){}},getWarningHTML:function(){return OpenLayers.i18n("getLayerWarning",{layerType:"MM",layerLib:"MultiMap"})},setMapObjectCenter:function(a,b){this.mapObject.goToPosition(a,b)},getMapObjectCenter:function(){return this.mapObject.getCurrentPosition()},getMapObjectZoom:function(){return this.mapObject.getZoomFactor()},getMapObjectLonLatFromMapObjectPixel:function(a){a.x-=this.map.getSize().w/2;a.y-=this.map.getSize().h/2;return this.mapObject.getMapPositionAt(a)},getMapObjectPixelFromMapObjectLonLat:function(a){return this.mapObject.

^ this looks correct. Yay!

Committed fix as r103865. I wouldn't mind some more test cases though; the minifier's tokenizer doesn't report parse errors so feels really skeezy to me. :)

  • Bug 34048 has been marked as a duplicate of this bug. ***