SVGParser is not thread-safe [with fix] (SVG parse error: Invalid colour keyword: white)

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Fire up N threads that will load the example SVG file
(it doesn't have to be the same file, the key is for all to have named colors 
in them)

What is the expected output? What do you see instead?
All pictures load safely instead of
com.caverock.androidsvg.SVGParseException: SVG parse error: Invalid colour 
keyword: white
    at com.caverock.androidsvg.SVGParser.parse(SVGParser.java:406)
    at com.caverock.androidsvg.SVG.getFromResource(SVG.java:187)
    at com.squareup.picasso.SVGLoader.load(SVGLoader.java:83)
    at com.squareup.picasso.SVGLoader.doInBackground(SVGLoader.java:66)
    at com.squareup.picasso.SVGLoader.doInBackground(SVGLoader.java:1)
    at ....SimpleAsyncTask.doInBackground(SimpleAsyncTask.java:8)
    at android.os.AsyncTask$2.call(AsyncTask.java:288)
    at java.util.concurrent.FutureTask.run(FutureTask.java:237)
    at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:231)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
    at java.lang.Thread.run(Thread.java:841)
Caused by: org.xml.sax.SAXException: Invalid colour keyword: white
    at com.caverock.androidsvg.SVGParser.parseColourKeyword(SVGParser.java:3066)
    at com.caverock.androidsvg.SVGParser.parseColour(SVGParser.java:3041)
    at com.caverock.androidsvg.SVGParser.parseColourSpecifer(SVGParser.java:2994)
    at com.caverock.androidsvg.SVGParser.parsePaintSpecifier(SVGParser.java:2983)
    at com.caverock.androidsvg.SVGParser.processStyleProperty(SVGParser.java:2425)
    at com.caverock.androidsvg.CSSParser.parseDeclarations(CSSParser.java:736)
    at com.caverock.androidsvg.CSSParser.parseRule(CSSParser.java:667)
    at com.caverock.androidsvg.CSSParser.parseRuleset(CSSParser.java:649)
    at com.caverock.androidsvg.CSSParser.parse(CSSParser.java:293)
    at com.caverock.androidsvg.SVGParser.parseCSSStyleSheet(SVGParser.java:3955)
    at com.caverock.androidsvg.SVGParser.endElement(SVGParser.java:605)
    at org.apache.harmony.xml.ExpatParser.endElement(ExpatParser.java:156)
    at org.apache.harmony.xml.ExpatParser.appendBytes(Native Method)
    at org.apache.harmony.xml.ExpatParser.parseFragment(ExpatParser.java:513)
    at org.apache.harmony.xml.ExpatParser.parseDocument(ExpatParser.java:474)
    at org.apache.harmony.xml.ExpatReader.parse(ExpatReader.java:316)
    at org.apache.harmony.xml.ExpatReader.parse(ExpatReader.java:279)
    at com.caverock.androidsvg.SVGParser.parse(SVGParser.java:394)
    ... 11 more

Please provide any additional information below.

Sample SVG: res/raw/questionmark.svg
<?xml version="1.0"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" 
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg xmlns="http://www.w3.org/2000/svg" viewBox="-25,-25 550 550">
    <g transform="translate(250, 250)">
        <circle r="250" stroke="black" stroke-width="50" fill="white" />
        <text text-anchor="middle" font-size="400" font-weight="bold" dy="125">?</text>
    </g>
</svg>

Looking at the code it's clear that the initialise*KeywordsMap usages are not 
thread safe despite the synchronized keyword:
The first thread sees a null map and goes to initialize it, first assigning the 
new instance of a Map and then filling it.
In the middle of the filling a context switch happens and
the second thread comes it, which sees that the map is not null (no need to 
initialize) and goes on using it.
But hey! the first thread didn't finish putting all mappings in, and "white" is 
towards the end probably that's why I discovered it.
In general all accesses (read/write) should be synchronized to a shared 
resource among Threads.

See
https://code.google.com/p/androidsvg/source/browse/src/com/caverock/androidsvg/S
VGParser.java?spec=svnc0c4748db221100ec54a93c302c59b01e3e0521b&r=ee9fa12ad768142
934e0257c87b572aea6df07d0#3059
(Huh, exactly this part was changed in the last commit)

The pattern
private static synchronized void initialise...Map() {
    map = new HashMap();
    map.put(...);
}
could be replaced by (but DON'T: see 
http://en.wikipedia.org/wiki/Double-checked_locking why not to use it)
private static synchronized void initialise...Map() {
    if (SVGParser.map == null) return; // this makes sure that any competing threads coming here will just bail quickly
    Map map = new HashMap(); // only the first thread will get here and start initializing the others will be blocked on the method's synchronized
    map.put(...);
    SVGParser.map = map; // this makes sure that all threads will wait for the initialization, since up until this point they all see an empty map and try to initialize and be blocked
    // exiting the method releases the block and all other threads will come in and see that the map is not null (first line)
}

Use this one instead: 
http://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom
Attached is the version updated and tested with:
    Runnable run = new Runnable() {
        public void run() {
            SVGParser parser = new SVGParser();
            try {
                SVG svg = parser.parse(getResources().openRawResource(R.raw.questionmark));
            } catch (NotFoundException ex) {
                ex.printStackTrace();
            } catch (SVGParseException ex) {
                ex.printStackTrace();
            }
        }
    };
    Thread[] threads = new Thread[10];
    for (int i = 0; i < threads.length; ++i) {
        threads[i] = new Thread(run);
    }
    for (int i = 0; i < threads.length; ++i) {
        threads[i].start();
    }
The original version 
(https://androidsvg.googlecode.com/hg-history/ee9fa12ad768142934e0257c87b572aea6
df07d0/src/com/caverock/androidsvg/SVGParser.java) was throwing 5+ exceptions 
every time I executed it; the patched version didn't.

Original issue reported on code.google.com by papp.robert.s on 17 Aug 2014 at 1:17

Attachments:

• SVGParser.java

[GoogleCodeExporter]

Good catch.  I should have done some multithreading testing after I made the 
lazy initialisation change.  And again after I made SVGImageView use 
asynchronous parsing.

Thanks for the patch!

Original comment by paul.leb...@gmail.com on 17 Aug 2014 at 4:47

• Changed state: Accepted

[GoogleCodeExporter]

Fixed in local revision 266.

Merged patch from papp.robert.s to fix multithreading issues in lazy 
initialised keyword maps.

Original comment by paul.leb...@gmail.com on 17 Aug 2014 at 4:51

• Changed state: Fixed