[PATCH] JSON parsing error messages should be more helpful.

[michael]

JSON parsing error messages should be more helpful. Would help use code monkeys debug our technologies. :)

[historic_bruno]

I don't see a (documented) way to do this in the Spidermonkey API :( Maybe we need to run a separate JSON/Lint tool.

[s0600204]

Slightly cleaner error handling for failed JSON parsing

[s0600204]

Whilst no closer to resolving the actual issue of working out how to derive the line number of a fault, I've uploaded a patch that (hopefully) should make errors returned from failed parsing a little nicer:

Firstly, when JSON.parse() or Engine.ReadJSONFile() can't parse a JSON string, it doesn't return false, but instead throws an Exception. So using an if statement to check for false isn't going to work. Curiously, the function that loads civ & map data already has a try...catch (as well as a check for false that I suspect won't ever get triggered), but the function that loads technology data does not. So I've added one.

Secondly, whenever a technology's data is requested, GetTemplate() checks to see if it exists in the appropriate array within the Technology Template Manager. If it does not, the function then attempts to load it from the filesystem. However if the parsing fails, an error is thrown (good), but nothing was being done about the technology not being in the array (bad). So next time the technology data is requested, it attempts (again) to load it off the filesystem - with the same result. This causes a lot of virtually identical errors and warnings to be created, sometimes in a continuous stream. To resolve, an empty object {} is stored at the unparseable technology's place in the array.

Thirdly, my alteration to the civinfo code makes it so that if a civ file is unparseable, that civ doesn't get placed into the civData array. This is because as is, an 'undefined' entry was being created in civ-selection drop-down box in the history screen that triggered further errors when clicked on by the user, not to mention a torrent of errors on the match-making screen that resulted from 0ad repeatedly trying (and failing) to get properties from an empty object.

[Itms]

Unfortunately, I have no time to study the patch right now, but please don't forget to follow the SubmittingPatches rules, or this patch will get lost between the huge number of tickets we have in there :)

[s0600204]

Yeah, just remembered...

[leper]

JSON.parse does not have the same behaviour as Engine.ReadJSONFile(). The latter does return an unchanged value (undefined), while the actual code that parses the JSON file in the engine (using JS_ParseJSON()) just logs an error and returns. It would be nice to print a better error message in case the parsing fails in the engine function.

We should not have two different ways to handle parsing JSON, so using just the engine function (and exposing that to the GUI too (maybe with modifications according to the path) would be nice (as parseJSONData() actually duplicates the behaviour of the engine function). It seems like JSON.parse has nicer error output in some newer SpiderMonkey versions (29+) so it would be nice if we could get those error messages using JS_ParseJSON() too.

Fix the comment in civinfo to be on a line of its own.

[s0600204]

@leper: Engine.ReadJSONParse may indeed attempt to return the value unchanged when a JSON file is syntactically incorrect, but JS sees an Exception instead (hence why I said previously that it threw an Exception). My theory is that JS_ParseJSON throws a JS-only Exception of some kind that is not checked for in Engine.ParseJSON nor Engine.ReadJSONFile but gets picked up by JS on return to that side of things (and is deemed more important).

Expected error messages (extrapolated from source files):

ERROR: JS_ParseJSON failed!
ERROR: Failed to load technology "armor_cav_01"

Actual error messages:

ERROR: JS_ParseJSON failed!
ERROR: JavaScript error: simulation/components/TechnologyTemplateManager.js line 22
SyntaxError: JSON.parse: expected property name or '}'
  TechnologyTemplateManager.prototype.GetTemplate@simulation/components/TechnologyTemplateManager.js:22
  TechnologyTemplateManager.prototype.Init@simulation/components/TechnologyTemplateManager.js:15

And after adding a try...catch to JS:

ERROR: JS_ParseJSON failed!
ERROR: SyntaxError: JSON.parse: expected property name or '}' in 'armor_cav_01'

[s0600204]

Minor adjustments to previous version of patch

[leper]

In 15959:

Clean up JSON parsing code. Refs #1374.
Fix Engine.ReadJSONFile() which did throw a JS exception that was not caught. Discovered by s0600204.
Expose Engine.ReadJSONFile() to the gui scripts.

[leper]

Thanks for verifying that! I opted to fix the error reporting on the engine side and after checking all uses of the two (parseJSONData() and parseJSONFromDataFile()) I noticed that they just need the behaviour of ScriptInterface::ReadJSONFile() so I exposed that to the GUI scripts.

The rest of the ticket (better error messages) will be fixed by the SM31 upgrade (#2462).

[Yves]

In 16214:

SpiderMonkey 31 upgrade

This upgrade also introduces exact stack rooting (see to the wiki: JSRootingGuide) and fixes problems with moving GC. This allows us to enable generational garbage collection (GGC).
Measurements a few months ago have shown a performance improvement of a non-visual replay of around 13.5%. This probably varies quite a bit, but it should be somewhere between 5-20%. Memory usage has also been improved. Check the forum thread for details.

Thanks to everyone from the team who helped with this directly or indirectly (review, finding and fixing issues, the required C++11 upgrade, the new autobuilder etc.)! Also thanks to the SpiderMonkey developers who helped on the #jsapi channel or elsewhere!

Fixes #2462, #2415, #2428, #2684, #1374
Refs #2973, #2669