Page MenuHomeWildfire Games

Get XML syntax errors not only the first time that file is loaded
ClosedPublic

Authored by elexis on Jun 12 2018, 10:54 AM.

Details

Summary

If an XML file happens to have syntax error (such as the one in credentials.xml in rP21847), then CXeromyces::ConvertFile will parse that file for the first time, throw a syntax error and then save a cached version of that file that doesn't have a syntax error to the cache.
Further loads will then go for the valid cache file, hiding the syntax error and possibly misleading authors into assuming their XML files are valid (such as in that commit).

Test Plan

Correctness: Notice that not writing a syntactically valid (but possibly semnatically wrong) cache file meets the only requirement of the claimed task.
Completeness: Judge whether this can break under some edge cases, such as packaged mods.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6244
Build 10368: Vulcan BuildJenkins
Build 10367: arc lint + arc unit

Event Timeline

elexis created this revision.Jun 12 2018, 10:54 AM
elexis updated this revision to Diff 6753.Jun 12 2018, 11:04 AM

Add comment.

Vulcan added a subscriber: Vulcan.Jun 12 2018, 11:04 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/644/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/645/display/redirect

elexis updated the Trac tickets for this revision.Jun 12 2018, 11:09 AM
bb accepted this revision.Jun 12 2018, 4:13 PM
bb added a subscriber: bb.

Agreeing

If it would crash for mods, the mods would receive an error in the current code, thus not caching the file wouldn't change much there.

This revision is now accepted and ready to land.Jun 12 2018, 4:13 PM
Stan added a subscriber: Stan.Jun 12 2018, 4:21 PM

What happens when archiving ?

vladislavbelov added inline comments.
source/ps/XML/Xeromyces.cpp
176

I think it's better to call it validSource, because of the function result is xmb.

ArchiveBuilder.cpp runs GenerateCachedXMB, which calls ConvertFile, which should the LOGERROR.
If the LOGERROR does not show up when running the archive build, that will be an important independent bug to solve.
mod bundlers already should see the error when they budnle, with this patch they will also see it more than once when running the affected code.
Since the cached file is not written anymore, the bundling script will exclude the XMB file for the invalid XML file.
That however is analogous to the other two LOGERRORs upon syntax errors.

The archive builder has an ENSURE that is triggered for the other LOGERRORs however, it's not triggered for this one.
So it seems like a return value would be useful.

It even says so: // For now, log the error and continue, in the future we might fail.

There is the same code in CXeromyces::LoadString.

...

Spent too much time trying to create a testcase, but it doesnt seem possible without changing the existing code,
mostly due to the static global defined in the implementation file that is not accessible.

source/ps/XML/Xeromyces.cpp
176

(Guess its not so ambiguous since valid refers to the process of validation, but if we want to narrow it down, I'd suggest XML, as that's even more narrow than source)

elexis added inline comments.Sep 16 2019, 11:54 PM
source/ps/XML/Xeromyces.cpp
176

Actually validXML is wrong, becaues the XML is valid according to XML specs, but the XML is not valid for that grammar.
And the ambiguity also exists for the validSource. validated would be wrong as well, since it also passes if there is no grammar.

elexis updated this revision to Diff 9814.Sep 16 2019, 11:56 PM

Return error code as well, i.e. doesnt load the file that is valid XML and also making the archive builder fail instead of silently accepting the file, like it does with XML files with (other) syntax errors.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/XML/Xeromyces.h
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/ps/XML/Xeromyces.h
| 115| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classXMBFile{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/726/display/redirect

This revision was landed with ongoing or failed builds.Sep 18 2019, 1:00 AM
This revision was automatically updated to reflect the committed changes.