Page MenuHomePhabricator

Error "A non well formed numeric value encountered" (from ImageMap)
Closed, ResolvedPublic

Description

Error

Request ID: XHMiBQpAADkAADl36DUAAAAH

message
[{exception_id}] {exception_url}   ErrorException from line 220 of /srv/mediawiki/php-1.33.0-wmf.18/extensions/ImageMap/includes/ImageMap.php: PHP Notice: A non well formed numeric value encountered
trace
#0 /srv/mediawiki/php-1.33.0-wmf.18/extensions/ImageMap/includes/ImageMap.php(220)
#1 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(3987): ImageMap::render(string, array, Parser, PPTemplateFrame_DOM)
#2 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/CoreParserFunctions.php(1098): Parser->extensionSubstitution(array, PPTemplateFrame_DOM)
#3 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(3515): CoreParserFunctions::tagObj(Parser, PPTemplateFrame_DOM, array)
#4 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(3222): Parser->callParserFunction(PPTemplateFrame_DOM, string, array)
#5 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Preprocessor_DOM.php(1279): Parser->braceSubstitution(array, PPTemplateFrame_DOM)
#6 /srv/mediawiki/php-1.33.0-wmf.18/extensions/ParserFunctions/includes/ExtParserFunctions.php(127): PPFrame_DOM->expand(DOMElement)
#7 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(3515): ExtParserFunctions::ifeqObj(Parser, PPTemplateFrame_DOM, array)
#8 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(3222): Parser->callParserFunction(PPTemplateFrame_DOM, string, array)
#9 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Preprocessor_DOM.php(1279): Parser->braceSubstitution(array, PPTemplateFrame_DOM)
#10 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(3396): PPFrame_DOM->expand(DOMElement)
#11 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Preprocessor_DOM.php(1279): Parser->braceSubstitution(array, PPTemplateFrame_DOM)
#12 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Preprocessor_DOM.php(1729): PPFrame_DOM->expand(DOMElement, integer)
#13 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(3393): PPTemplateFrame_DOM->cachedExpand(string, PPNode_DOM)
#14 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Preprocessor_DOM.php(1279): Parser->braceSubstitution(array, PPFrame_DOM)
#15 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(3036): PPFrame_DOM->expand(DOMElement, integer)
#16 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(1354): Parser->replaceVariables(string)
#17 /srv/mediawiki/php-1.33.0-wmf.18/includes/parser/Parser.php(482): Parser->internalParse(string)
#18 /srv/mediawiki/php-1.33.0-wmf.18/includes/content/WikitextContent.php(369): Parser->parse(string, Title, ParserOptions, boolean, boolean, NULL)
#19 /srv/mediawiki/php-1.33.0-wmf.18/includes/content/AbstractContent.php(555): WikitextContent->fillParserOutput(Title, NULL, ParserOptions, boolean, ParserOutput)
#20 /srv/mediawiki/php-1.33.0-wmf.18/includes/EditPage.php(4089): AbstractContent->getParserOutput(Title, NULL, ParserOptions)
#21 /srv/mediawiki/php-1.33.0-wmf.18/includes/EditPage.php(3996): EditPage->doPreviewParse(WikitextContent)
#22 /srv/mediawiki/php-1.33.0-wmf.18/includes/EditPage.php(2775): EditPage->getPreviewText()
#23 /srv/mediawiki/php-1.33.0-wmf.18/includes/EditPage.php(719): EditPage->showEditForm()
#24 /srv/mediawiki/php-1.33.0-wmf.18/includes/actions/EditAction.php(60): EditPage->edit()
#25 /srv/mediawiki/php-1.33.0-wmf.18/includes/MediaWiki.php(501): EditAction->show()
#26 /srv/mediawiki/php-1.33.0-wmf.18/includes/MediaWiki.php(294): MediaWiki->performAction(Article, Title)
#27 /srv/mediawiki/php-1.33.0-wmf.18/includes/MediaWiki.php(867): MediaWiki->performRequest()
#28 /srv/mediawiki/php-1.33.0-wmf.18/includes/MediaWiki.php(517): MediaWiki->main()
#29 /srv/mediawiki/php-1.33.0-wmf.18/index.php(42): MediaWiki->run()

Impact

ImageMap does not currently prescribe what should happen if one of the coordinates in <imagemap> (in wikitext) is set as something invalid (not numerical).

What happens by accident now is that PHP encounters the invalid request to convert such string to an integer, logs this warning implicitly, and then pretends 0 was given and continues as normal.

Right now, this is logged as a problem with the software, whereas it should be logged (if at all) as a problem with the user input. That way it will not affect the perceived stability levels of our production cluster.

Notes

If the current accidental behaviour is intentional, we should validate the input with something like if ( ctype_digit( .. ) and explicitly default to 0.

This problem was first observed last week when the PHP 7 trials became public. PHP 7.2 is stricter than PHP 7.0 and HHVM in this regard.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 25 2019, 8:06 PM

(Still reproducible on 1.33-wmf.21. Tagging per mw:Maintainers.)

aezell claimed this task.Mar 22 2019, 6:32 PM
aezell added a subscriber: aezell.

I'm taking a look at this bug. Seemed like an easy one to tackle on a Friday afternoon.

Change 498445 had a related patch set uploaded (by Aezell; owner: Aezell):
[mediawiki/extensions/ImageMap@master] Default image map coords to zero

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

That patch fails a CI security check with this error: Calling method \Xml::element() in \ImageMap::render that outputs using tainted argument $attribs. (Caused by: Builtin-\Xml::element) (Caused by: ./includes/ImageMap.php +249; ./includes/ImageMap.php +189; ./includes/ImageMap.php +252; ./includes/ImageMap.php +263)

I'll look into it. At first glance, it appears to have nothing to do with my change. Maybe I can clean it up.

Upon further examination, it's possible this bug only presents for poly maps. There is code to protect against non-numeric coords for rect and circle and default maps.

The fix may be similar but should probably happen elsewhere in the code. This might also help clean up the error above.

I pushed a new patch that works much better and passes Jenkins.

Now, instead of defaulting to zero, we warn the user that there is an invalid coordinate. I like that solution much better.

Change 498445 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@master] Warn user about invalid coordinates

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

Krinkle closed this task as Resolved.Mar 22 2019, 10:42 PM
Krinkle removed a project: Patch-For-Review.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMar 22 2019, 10:42 PM
Cirdan added a subscriber: Cirdan.Mar 29 2019, 7:04 AM

Thanks for cleaning this up!

However, this change caused all articles about major cities on the German-language Wikipedia to display a large, layout-breaking error message instead of the usual map right at the top of the page. A heads up to the communities would have been much appreciated.

TheDJ added a subscriber: TheDJ.EditedMar 29 2019, 9:15 AM

@aezell was the marking of floats as invalid an intentional decision ?

Because it seems that this broke poly's that were using float numbers. I think float values for imagemaps are pretty much undocumented, but they were (and for other numbers ARE) taken into account when applying scale for instance, before being cast to integers.

According to the <area> coords spec of HTML, which our image maps are targeting, they are documented as Length's, which are either percentage values or integer Pixel values. Percentages were however never properly supported by browsers. Thus in HTML 5 it was explicitly redefined to be a list of comma separated integers.

Because of that, I think it's ok to have these invalid indeed, but it is a change in behaviour.

Begoon added a subscriber: Begoon.Mar 29 2019, 9:45 AM

@Cirdan I apologize for the breakage. There was discussion here about the fact that the old code simply dumped the error into the log and that behavior might be preferable. It seems in the case you mention that it is.

As @TheDJ points out, I was basing my work on the HTML5 specs. All of the values are converted to integers so any floats, percentages, etc. get scaled naively. This was true of the previous code as well so I didn't change that behavior.

I think a possible solution here is to use is_numeric instead of ctype_digit in the check. It would permit floats even though they'll be cast to integers as the code does naive scaling.

Begoon added a comment.EditedMar 30 2019, 1:14 AM

Agree that floats should be permitted, and any type conversion only done after scaling, to avoid loss of precision. At https://en.wikipedia.org/w/index.php?title=Template:Indian_states_and_territories_image_map&diff=889959581&oldid=889956524 I was forced to convert a whole heap of poly co-ordinates to integer in order to get the template to render at all. Fortunately I could do this by using a perl command on a text file of the wikitext - "perl -i -pe 's/(\d*\.\d*)/int($1+0.5)/ge' filename" otherwise manually rounding hundreds of co-ordinates would have been quite a task. The floats had existed in the code for years without throwing this error, so it certainly was a change in behaviour.

Krinkle reopened this task as Open.Mar 30 2019, 3:10 AM
Krinkle triaged this task as Unbreak Now! priority.

This is breaking real content that has worked for years.

Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptMar 30 2019, 3:10 AM

Change 500159 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/ImageMap@master] Revert "Warn user about invalid coordinates"

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

Change 500160 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/ImageMap@wmf/1.33.0-wmf.23] Revert "Warn user about invalid coordinates"

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

Reverted in-branch to unbreak prod. Still broken in master, to be fixed (or also reverted) before the next train.

Change 500160 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@wmf/1.33.0-wmf.23] Revert "Warn user about invalid coordinates"

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

This is breaking real content that has worked for years.

https://en.wikipedia.org/w/index.php?title=Template:Indian_states_and_territories_image_map&oldid=889956524

Now renders an image again, and with clickable countries:

Mentioned in SAL (#wikimedia-operations) [2019-03-30T03:39:06Z] <krinkle@deploy1001> Synchronized php-1.33.0-wmf.23/extensions/ImageMap/includes/ImageMap.php: I1387825f25e / T217087 (duration: 00m 52s)

Begoon added a comment.EditedMar 30 2019, 5:20 AM

This is breaking real content that has worked for years.

https://en.wikipedia.org/w/index.php?title=Template:Indian_states_and_territories_image_map&oldid=889956524
Now renders an image again, and with clickable countries:

Thank you for your quick action. Much appreciated.

Cirdan lowered the priority of this task from Unbreak Now! to Normal.Mar 30 2019, 2:03 PM
Cirdan added a project: User-notice.

The change has been reverted.

Please consider giving the communities a few weeks of time to clean up invalid imagemaps (or, preferably, run a script) before deploying again.

Change 500184 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/ImageMap@master] Use is_numeric instead of ctype_digit

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

Change 500184 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@master] Use is_numeric instead of ctype_digit for 'poly' validation

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

aezell added a comment.EditedApr 1 2019, 4:08 PM

My apologies to all for this breakage. Thank you to @Krinkle for cleaning up my mess.

And thanks to @Esanders for the new patch.

aezell added a subscriber: Esanders.Apr 1 2019, 4:10 PM
dduvall raised the priority of this task from Normal to Unbreak Now!.EditedApr 2 2019, 5:44 PM
dduvall added a subscriber: dduvall.

Thanks for escalating this prior to deployment. Marking UBN as is policy with all deployment blockers.

Jdforrester-WMF closed this task as Resolved.Apr 2 2019, 5:46 PM
Jdforrester-WMF lowered the priority of this task from Unbreak Now! to Normal.
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Thanks for escalating this prior to deployment. Marking UBN as is policy with all deployment blockers.

It was a blocker, it was UBN, it was reverted in production and fixed in master, it's no longer UBN.

Change 500159 abandoned by Krinkle:
Revert "Warn user about invalid coordinates"

Reason:
Thx

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

Johan added a subscriber: Johan.Apr 2 2019, 10:24 PM

Is this still relevant for a Tech News update? In that case, how do we best phrase it?

(I.e. do we still need to give warning about upcoming changes that might affect content?)

@Johan I think the new patch from @Esanders fixes the bug without changing the expected use cases. I don't think this new fix will affect the users in any way.

TheDJ added a comment.Apr 3 2019, 11:09 AM

We should probably update parser tests for this though, considering we are deviating from HTML5 expectations, this issue could easily be reintroduced...

Kipod added a subscriber: Kipod.Apr 10 2019, 3:49 PM

legit maps on many wikis are still broken. i believe this story should be reopened - i could not understand the status of the patch, but in production sites (wikipedias of all languages), this is still borked.

also, i may have missed something, but i saw only "float" values discussed, while in the real world, some of the issues are not with float values, but rather with _negative_ values.

as far as i know, -1 (even though it lies outside the image borders) is a legit coordinate, which previously worked with no complaints, and currently display error message, instead of the map.

(shameless plug): this case demonstrates again that adding tracking categories to imagemap errors is not a "luxury". please resolve T106075.

i'll reopen this story - not 100% sure this is the right thing to do, but my grandma told me - "when in doubt, reopen".

peace.

Kipod reopened this task as Open.Apr 10 2019, 3:50 PM
Cirdan raised the priority of this task from Normal to Unbreak Now!.Apr 10 2019, 4:52 PM

This appears to have been deployed again. Users in dewiki report broken Imagemaps again.

aezell added a comment.EditedApr 10 2019, 4:56 PM

@Cirdan Do you have any examples? I'm looking at https://de.wikipedia.org/wiki/Baden-W%C3%BCrttemberg and https://de.wikipedia.org/wiki/Bayern and https://de.wikipedia.org/wiki/Berlin which seem fine.

Also, just for clarity's sake, this wasn't "redeployed." There was a second patch made which is deployed as this while loop in the master branch of the extension indicates: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/ImageMap/+/refs/heads/master/includes/ImageMap.php#209

In it, you can see that it excludes negative coordinates as @Kipod's post comments on.

Here's a link to a broken page:
https://en.wikipedia.org/w/index.php?title=Template:Imagemap_Germany_district_AW&oldid=566491718

A couple of minor enhancement requests:

  1. Add a tracking category, e.g. Category:Imagemap with invalid coordinates, so that gnomes can find and fix these errors instead of having to encounter them randomly.
  2. Tweak the error text, which complains about the input not being a "number", even when the input is a number that happens to be negative.
  3. Best option: If negative numbers are truly invalid, deprecate the use of negative numbers but continue the old behavior of replacing the negative number with a zero, but add a tracking category so that gnomes can fix these templates. Give the job queue some time to populate the error category (see also T157670, which means that it may take many months or years for the error category to populate fully), then remove support for negative numbers once all pages are null-edited.
Cirdan lowered the priority of this task from Unbreak Now! to High.Apr 10 2019, 6:38 PM

These were fixed (i.e. negative numbers removed) during the first time this issue arose, now for example this template caused an error (it has been fixed by a user in the meantime).

Also, just for clarity's sake, this wasn't "redeployed." There was a second patch made which is deployed as this while loop in the master branch of the extension indicates: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/ImageMap/+/refs/heads/master/includes/ImageMap.php#209

Thanks for clearing this up. What I have observed is that after Krinkle rolled back the changes, everything was fine, but now the same error message started to appear again and users were complaining. Hence it seemed to me as if the original code was back in production. (I'm glad to hear that this is not the case!)

I'd like to second @Jonesey95's call for a maintenance category so that all imagemaps can be fixed.

TheDJ added a comment.Apr 10 2019, 8:02 PM

Negative numbers were explicitly excluded it seems. The HTML 5 spec however notes that negative numbers are allowed in the syntax for imagemap poly's. Seems like something that should be fixed yes.. Also parsertests ;)

Based on the patch and its commit message, I believe the author was following the same logic as is used for the coordinates in a circle or rectangle map. Whatever the spec, I think we can agree that negative coordinates aren't logical in the coordinate system used for polygonal image maps.

However, we have a lot of polygon image maps in our content that are breaking on this. So, this seems to be a case where being technically correct isn't the best approach.

This should be an easy change. I'll see if I can prepare a patch.

Change 502888 had a related patch set uploaded (by Aezell; owner: Aezell):
[mediawiki/extensions/ImageMap@master] Allow negative coordinates in poly image maps

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

Kipod added a comment.EditedApr 10 2019, 10:34 PM

....., I think we can agree that negative coordinates aren't logical in the coordinate system used for polygonal image maps.

no, we can't agree.
it is legit to have a poly area, where one of the vertices is outside the edge of the image.
it actually makes sense: basically, the part of the polygon intersecting the image itself is "click sensitive".

also, the test itself is inconsistent: it allows poly points outside the image to the right and bottom, but not to the left or top.

of course, the winning argument here is that there _are_ such imagemaps all over the place, which until recently worked just fine, thank you, and you do not introduce a breaking change based on "we can agree".

looked at https://gerrit.wikimedia.org/r/502888 , and i don't like it.
it has "if (coord < 0) coord = 0". this perverts the shape of the polygon as set by the user.
html5 does not preclude negative coords, and you should not pervert the shape based on a superstition. if you get a negative coord, please be a good citizen and pass it along.

peace.

Kipod added a comment.EditedApr 10 2019, 10:55 PM

Here's a link to a broken page:
https://en.wikipedia.org/w/index.php?title=Template:Imagemap_Germany_district_AW&oldid=566491718
A couple of minor enhancement requests:

  1. Add a tracking category, e.g. Category:Imagemap with invalid coordinates, so that gnomes can find and fix these errors instead of having to encounter them randomly.
  2. Tweak the error text, which complains about the input not being a "number", even when the input is a number that happens to be negative.
  3. Best option: If negative numbers are truly invalid, deprecate the use of negative numbers but continue the old behavior of replacing the negative number with a zero, but add a tracking category so that gnomes can fix these templates. Give the job queue some time to populate the error category (see also T157670, which means that it may take many months or years for the error category to populate fully), then remove support for negative numbers once all pages are null-edited.

+1

  • regarding "tracking" - let me rephrase: please resolve T106075
  • as to the error message: let me expand: the recent incident taught me something: with imagemaps, where you have very very very long lines, which sometime wraps 4 or even more lines in editor, it would be very helpful to include the offending string that could not be parsed (sanitize it, if you have to) as part of the error message. otherwise it can be a real challenge to locate the problem. imagine O instead of 0 in one of the numbers, or l/I instead of 1.
  • as to "if negative numers are truly invalid" - i do not believe they are valid.

peace

it is legit to have a poly area, where one of the vertices is outside the edge of the image.

That's true. To the end user, it will look like two vertices that intersect with the edge. I was confused about how the browser would render that.

of course, the winning argument here is that there _are_ such imagemaps all over the place, which until recently worked just fine, thank you, and you do not introduce a breaking change based on "we can agree".

Exactly as I said in my response from which you are quoting. As you're agreeing with me here, I don't understand why your tone is so condescending. I didn't introduce the breaking change regarding the negative coordinates. The break is already in production. I was suggesting a patch that might help resolve it. You are implying that I have an agenda that is something other than fixing the breakage that is already in production and that implication is incorrect.

looked at https://gerrit.wikimedia.org/r/502888 , and i don't like it.
it has "if (coord < 0) coord = 0". this perverts the shape of the polygon as set by the user.
html5 does not preclude negative coords, and you should not pervert the shape based on a superstition. if you get a negative coord, please be a good citizen and pass it along.

I agree with your technical points and am amending my patch to allow for negative coordinates.

Your tone in this response is highly critical of me personally. It's uncalled for and diminishes your otherwise helpful collaboration.

Kipod added a comment.EditedApr 11 2019, 3:21 PM

Your tone in this response is highly critical of me personally. It's uncalled for and diminishes your otherwise helpful collaboration.

this was not intentional. i did not mean anything personal in my comment (when i say "you" in phabricator, i usually take it to mean "mediawiki software", as i have no personal acquaintance with any developer, and i usually do not check to see who wrote what ("blame" in git-ish) )

please accept my sincere apology - again, no personal slight was meant. my style tends to be somewhat informal, and sometimes i do use "graphic" terms.
in this context, "superstition" meant to imply that since such limitation ("all poly area vertices must reside within the map") is not documented, it must have come from someone's belief...

again, please accept my apology. i will do my best to avoid phrases that can be perceived as offensive in the future, but my track record in this area is not stellar.....

peace.

again, please accept my apology.

Thank you for this apology. It is well received.

And thanks for helping me understand the negative coordinates better. I appreciate your help.

@aezell To confirm, this patch makes it behave the same as it did previously? Or is turning negative into positive different from how it was tolerated by the parser in the past?

For what it's worth, if previous behaviour was to skip and render only the remaining coords, that seems perfectly fine as well. The main issue is to have the rest still render. (eg. the bad coord could be skipped or re-read as a copy of a preceding/next coord instead to make a mostly usable output still.)

@Krinkle This patch should ensure that the coordinates are numeric, either positive or negative. The only change here, from production, is to allow negative coordinates. This modifies Ed's patch which came after your revert.

So, it should preserve the previous parsing behavior while also avoiding the log errors that were occurring which got this whole thing started.

@aezell Yeah, it changes what we have in prod now, but I mean the behaviour of "negative becomes positive" is what we had up until recently, right? Or were they previously tolerated in a different way?

Change 503473 had a related patch set uploaded (by Krinkle; owner: Aezell):
[mediawiki/extensions/ImageMap@wmf/1.33.0-wmf.25] Allow negative coordinates in poly image maps

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

Change 503473 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@wmf/1.33.0-wmf.25] Allow negative coordinates in poly image maps

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

Change 502888 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@master] Allow negative coordinates in poly image maps

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

Mentioned in SAL (#wikimedia-operations) [2019-04-12T21:10:51Z] <krinkle@deploy1001> Synchronized php-1.33.0-wmf.25/extensions/ImageMap/includes/ImageMap.php: I0ee84f059da / T217087 (duration: 05m 12s)

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:07 PM