Fix compatibility issues in XmlPullParserFactory.

- We should set features on a parser only if the feature
  value is true. I.e, we guarantee that we'll never call
  XmlPullParser.setFeature(..., false).

- In change 0363556bad8930a, we didn't consider the fact
  that apps could extend XmlPullParserFactory and modify
  protected fields to change the parser / serializer that's
  instantiated. I've reinstated this feature, despite the
  fact that it's a particularly pointless feature on a
  more-or-less pointless public API. Apps can and should
  instantiate their own parser instances directly instead
  of going through this factory.

bug: 12956724

Change-Id: I793eba335b5385eb641e023b3613bba4515a85bf

2 files changed, 427 insertions, 28 deletions

diff --git a/luni/src/test/java/libcore/xml/XmlPullParserFactoryTest.java b/luni/src/test/java/libcore/xml/XmlPullParserFactoryTest.java
index ea795f8..7194414 100644
--- a/luni/src/test/java/libcore/xml/XmlPullParserFactoryTest.java
+++ b/luni/src/test/java/libcore/xml/XmlPullParserFactoryTest.java
@@ -16,19 +16,359 @@
 
 package libcore.xml;
 
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.io.Reader;
+import java.io.Writer;
 import junit.framework.TestCase;
+import org.kxml2.io.KXmlParser;
+import org.kxml2.io.KXmlSerializer;
 import org.xmlpull.v1.XmlPullParser;
+import org.xmlpull.v1.XmlPullParserException;
 import org.xmlpull.v1.XmlPullParserFactory;
 import org.xmlpull.v1.XmlSerializer;
 
 public class XmlPullParserFactoryTest extends TestCase {
 
-    public void testNewInstance() throws Exception {
+    public void testDefaultNewInstance() throws Exception {
         XmlPullParserFactory factory = XmlPullParserFactory.newInstance(null, null);
         XmlPullParser parser = factory.newPullParser();
         XmlSerializer serializer = factory.newSerializer();
 
         assertNotNull(parser);
         assertNotNull(serializer);
+        assertTrue(parser instanceof KXmlParser);
+        assertTrue(serializer instanceof KXmlSerializer);
+    }
+
+    /**
+     * Tests that trying to instantiate a parser with an empty list of
+     * parsers and serializers fails.
+     */
+    public void testOverriding_emptyClassList() {
+        TestXmlPullParserFactory tf = new TestXmlPullParserFactory(null, null);
+
+        try {
+            tf.newPullParser();
+            fail();
+        } catch (XmlPullParserException expected) {
+        }
+
+        try {
+            tf.newPullParser();
+            fail();
+        } catch (XmlPullParserException expected) {
+        }
+    }
+
+    public void testOverriding_customClassList() throws Exception {
+        TestXmlPullParserFactory tf = new TestXmlPullParserFactory(
+                new String[] { "libcore.xml.XmlPullParserFactoryTest$XmlPullParserStub" },
+                new String[] { "libcore.xml.XmlPullParserFactoryTest$XmlSerializerStub" });
+
+        assertTrue(tf.newPullParser() instanceof XmlPullParserStub);
+        assertTrue(tf.newSerializer() instanceof XmlSerializerStub);
+
+        // Also check that we ignore instantiation errors as long as
+        // at least one parser / serializer is instantiable.
+        tf = new TestXmlPullParserFactory(
+                new String[] {
+                        "libcore.xml.XmlPullParserFactoryTest$InaccessibleXmlParser",
+                        "libcore.xml.XmlPullParserFactoryTest$XmlPullParserStub" },
+                new String[] {
+                        "libcore.xml.XmlPullParserFactoryTest$InaccessibleXmlSerializer",
+                        "libcore.xml.XmlPullParserFactoryTest$XmlSerializerStub" });
+
+        assertTrue(tf.newPullParser() instanceof XmlPullParserStub);
+        assertTrue(tf.newSerializer() instanceof XmlSerializerStub);
+    }
+
+    // https://b/12956724
+    public void testSetFeature_setsFeatureOnlyIfTrue() throws Exception {
+        TestXmlPullParserFactory tf = new TestXmlPullParserFactory(
+                new String[] { "libcore.xml.XmlPullParserFactoryTest$XmlParserThatHatesAllFeatures" }, null);
+
+        tf.setFeature("foo", false);
+        tf.newPullParser();
+    }
+
+
+    /**
+     * A class that makes use of inherited XmlPullParserFactory fields to check they are
+     * supported.
+     */
+    static final class TestXmlPullParserFactory extends XmlPullParserFactory {
+        TestXmlPullParserFactory(String[] parserClassList, String[] serializerClassList) {
+            super();
+            parserClasses.remove(0);
+            serializerClasses.remove(0);
+
+            try {
+                if (parserClassList != null) {
+                    for (String parserClass : parserClassList) {
+                        parserClasses.add(Class.forName(parserClass));
+                    }
+                }
+
+                if (serializerClassList != null) {
+                    for (String serializerClass : serializerClassList) {
+                        serializerClasses.add(Class.forName(serializerClass));
+                    }
+                }
+            } catch (ClassNotFoundException ignored) {
+                throw new AssertionError(ignored);
+            }
+        }
+    }
+
+    public static final class XmlParserThatHatesAllFeatures extends XmlPullParserStub {
+        @Override
+        public void setFeature(String name, boolean state) {
+            fail();
+        }
+    }
+
+    static final class InaccessibleXmlSerializer extends XmlSerializerStub {
+    }
+
+    static final class InaccessibleXmlParser extends XmlPullParserStub {
+    }
+
+    public static class XmlSerializerStub implements XmlSerializer {
+
+        public void setFeature(String name, boolean state) throws IllegalArgumentException,
+                IllegalStateException {
+        }
+
+        public boolean getFeature(String name) {
+            return false;
+        }
+
+        public void setProperty(String name, Object value) {
+        }
+
+        public Object getProperty(String name) {
+            return null;
+        }
+
+        public void setOutput(OutputStream os, String encoding) throws IOException {
+        }
+
+        public void setOutput(Writer writer) throws IOException {
+        }
+
+        public void startDocument(String encoding, Boolean standalone) throws IOException {
+        }
+
+        public void endDocument() throws IOException {
+        }
+
+        public void setPrefix(String prefix, String namespace) throws IOException {
+        }
+
+        public String getPrefix(String namespace, boolean generatePrefix) throws IllegalArgumentException {
+            return null;
+        }
+
+        public int getDepth() {
+            return 0;
+        }
+
+        public String getNamespace() {
+            return null;
+        }
+
+        public String getName() {
+            return null;
+        }
+
+        public XmlSerializer startTag(String namespace, String name) throws IOException {
+            return null;
+        }
+
+        public XmlSerializer attribute(String namespace, String name, String value) throws IOException {
+            return null;
+        }
+
+        public XmlSerializer endTag(String namespace, String name) throws IOException {
+            return null;
+        }
+
+        public XmlSerializer text(String text) throws IOException {
+            return null;
+        }
+
+        public XmlSerializer text(char[] buf, int start, int len) throws IOException {
+            return null;
+        }
+
+        public void cdsect(String text) throws IOException {
+        }
+
+        public void entityRef(String text) throws IOException {
+        }
+
+        public void processingInstruction(String text) throws IOException {
+        }
+
+        public void comment(String text) throws IOException {
+        }
+
+        public void docdecl(String text) throws IOException {
+        }
+
+        public void ignorableWhitespace(String text) throws IOException {
+        }
+
+        public void flush() throws IOException {
+        }
+    }
+
+    public static class XmlPullParserStub implements XmlPullParser {
+        public void setFeature(String name, boolean state) throws XmlPullParserException {
+        }
+
+        public boolean getFeature(String name) {
+            return false;
+        }
+
+        public void setProperty(String name, Object value) throws XmlPullParserException {
+        }
+
+        public Object getProperty(String name) {
+            return null;
+        }
+
+        public void setInput(Reader in) throws XmlPullParserException {
+        }
+
+        public void setInput(InputStream inputStream, String inputEncoding)
+                throws XmlPullParserException {
+        }
+
+        public String getInputEncoding() {
+            return null;
+        }
+
+        public void defineEntityReplacementText(String entityName, String replacementText)
+                throws XmlPullParserException {
+        }
+
+        public int getNamespaceCount(int depth) throws XmlPullParserException {
+            return 0;
+        }
+
+        public String getNamespacePrefix(int pos) throws XmlPullParserException {
+            return null;
+        }
+
+        public String getNamespaceUri(int pos) throws XmlPullParserException {
+            return null;
+        }
+
+        public String getNamespace(String prefix) {
+            return null;
+        }
+
+        public int getDepth() {
+            return 0;
+        }
+
+        public String getPositionDescription() {
+            return null;
+        }
+
+        public int getLineNumber() {
+            return 0;
+        }
+
+        public int getColumnNumber() {
+            return 0;
+        }
+
+        public boolean isWhitespace() throws XmlPullParserException {
+            return false;
+        }
+
+        public String getText() {
+            return null;
+        }
+
+        public char[] getTextCharacters(int[] holderForStartAndLength) {
+            return null;
+        }
+
+        public String getNamespace() {
+            return null;
+        }
+
+        public String getName() {
+            return null;
+        }
+
+        public String getPrefix() {
+            return null;
+        }
+
+        public boolean isEmptyElementTag() throws XmlPullParserException {
+            return false;
+        }
+
+        public int getAttributeCount() {
+            return 0;
+        }
+
+        public String getAttributeNamespace(int index) {
+            return null;
+        }
+
+        public String getAttributeName(int index) {
+            return null;
+        }
+
+        public String getAttributePrefix(int index) {
+            return null;
+        }
+
+        public String getAttributeType(int index) {
+            return null;
+        }
+
+        public boolean isAttributeDefault(int index) {
+            return false;
+        }
+
+        public String getAttributeValue(int index) {
+            return null;
+        }
+
+        public String getAttributeValue(String namespace, String name) {
+            return null;
+        }
+
+        public int getEventType() throws XmlPullParserException {
+            return 0;
+        }
+
+        public int next() throws XmlPullParserException, IOException {
+            return 0;
+        }
+
+        public int nextToken() throws XmlPullParserException, IOException {
+            return 0;
+        }
+
+        public void require(int type, String namespace, String name)
+                throws XmlPullParserException, IOException {
+        }
+
+        public String nextText() throws XmlPullParserException, IOException {
+            return null;
+        }
+
+        public int nextTag() throws XmlPullParserException, IOException {
+            return 0;
+        }
     }
 }
diff --git a/xml/src/main/java/org/xmlpull/v1/XmlPullParserFactory.java b/xml/src/main/java/org/xmlpull/v1/XmlPullParserFactory.java
index 32e4f20..41db89c 100644
--- a/xml/src/main/java/org/xmlpull/v1/XmlPullParserFactory.java
+++ b/xml/src/main/java/org/xmlpull/v1/XmlPullParserFactory.java
@@ -20,18 +20,13 @@ import java.util.Map;
  */
 
 public class XmlPullParserFactory {
-    // TODO: Deprecate or remove these fields. They're currently unused
-    // but are public APIs.
 
-    /** Currently unused. */
-    public static final String PROPERTY_NAME =
-        "org.xmlpull.v1.XmlPullParserFactory";
-    /** Currently unused */
+    public static final String PROPERTY_NAME = "org.xmlpull.v1.XmlPullParserFactory";
+    protected ArrayList parserClasses;
+    protected ArrayList serializerClasses;
+
+    /** Unused, but we have to keep it because it's public API. */
     protected String classNamesLocation = null;
-    /** Currently unused */
-    protected ArrayList parserClasses = null;
-    /** Currently unused */
-    protected ArrayList serializerClasses = null;
 
     // features are kept there
     // TODO: This can't be made final because it's a public API.
@@ -40,12 +35,18 @@ public class XmlPullParserFactory {
     /**
      * Protected constructor to be called by factory implementations.
      */
-
     protected XmlPullParserFactory() {
+        parserClasses = new ArrayList<String>();
+        serializerClasses = new ArrayList<String>();
+
+        try {
+            parserClasses.add(Class.forName("org.kxml2.io.KXmlParser"));
+            serializerClasses.add(Class.forName("org.kxml2.io.KXmlSerializer"));
+        } catch (ClassNotFoundException e) {
+            throw new AssertionError();
+        }
     }
 
-
-
     /**
      * Set the features to be set when XML Pull Parser is created by this factory.
      * <p><b>NOTE:</b> factory features are not used for XML Serializer.
@@ -53,7 +54,6 @@ public class XmlPullParserFactory {
      * @param name string with URI identifying feature
      * @param state if true feature will be set; if false will be ignored
      */
-
     public void setFeature(String name, boolean state) throws XmlPullParserException {
         features.put(name, state);
     }
@@ -67,8 +67,7 @@ public class XmlPullParserFactory {
      * @return The value of named feature.
      *     Unknown features are <string>always</strong> returned as false
      */
-
-    public boolean getFeature (String name) {
+    public boolean getFeature(String name) {
         Boolean value = features.get(name);
         return value != null ? value.booleanValue() : false;
     }
@@ -81,7 +80,6 @@ public class XmlPullParserFactory {
      * @param awareness true if the parser produced by this code
      *    will provide support for XML namespaces;  false otherwise.
      */
-
     public void setNamespaceAware(boolean awareness) {
         features.put (XmlPullParser.FEATURE_PROCESS_NAMESPACES, awareness);
     }
@@ -94,12 +92,10 @@ public class XmlPullParserFactory {
      * @return  true if the factory is configured to produce parsers
      *    which are namespace aware; false otherwise.
      */
-
     public boolean isNamespaceAware() {
-        return getFeature (XmlPullParser.FEATURE_PROCESS_NAMESPACES);
+        return getFeature(XmlPullParser.FEATURE_PROCESS_NAMESPACES);
     }
 
-
     /**
      * Specifies that the parser produced by this factory will be validating
      * (it simply set feature XmlPullParser.FEATURE_VALIDATION to true or false).
@@ -108,9 +104,8 @@ public class XmlPullParserFactory {
      *
      * @param validating - if true the parsers created by this factory  must be validating.
      */
-
     public void setValidating(boolean validating) {
-        features.put (XmlPullParser.FEATURE_VALIDATION, validating);
+        features.put(XmlPullParser.FEATURE_VALIDATION, validating);
     }
 
     /**
@@ -122,7 +117,7 @@ public class XmlPullParserFactory {
      */
 
     public boolean isValidating() {
-        return getFeature (XmlPullParser.FEATURE_VALIDATION);
+        return getFeature(XmlPullParser.FEATURE_VALIDATION);
     }
 
     /**
@@ -131,16 +126,80 @@ public class XmlPullParserFactory {
      *
      * @return A new instance of a XML Pull Parser.
      */
-
     public XmlPullParser newPullParser() throws XmlPullParserException {
-        final XmlPullParser pp = new KXmlParser();
+        final XmlPullParser pp = getParserInstance();
         for (Map.Entry<String, Boolean> entry : features.entrySet()) {
-            pp.setFeature(entry.getKey(), entry.getValue());
+            // NOTE: This test is needed for compatibility reasons. We guarantee
+            // that we only set a feature on a parser if its value is true.
+            if (entry.getValue()) {
+                pp.setFeature(entry.getKey(), entry.getValue());
+            }
         }
 
         return pp;
     }
 
+    private XmlPullParser getParserInstance() throws XmlPullParserException {
+        ArrayList<Exception> exceptions = null;
+
+        if (parserClasses != null && !parserClasses.isEmpty()) {
+            exceptions = new ArrayList<Exception>();
+            for (Object o : parserClasses) {
+                try {
+                    if (o != null) {
+                        Class<?> parserClass = (Class<?>) o;
+                        return (XmlPullParser) parserClass.newInstance();
+                    }
+                } catch (InstantiationException e) {
+                    exceptions.add(e);
+                } catch (IllegalAccessException e) {
+                    exceptions.add(e);
+                } catch (ClassCastException e) {
+                    exceptions.add(e);
+                }
+            }
+        }
+
+        throw newInstantiationException("Invalid parser class list", exceptions);
+    }
+
+    private XmlSerializer getSerializerInstance() throws XmlPullParserException {
+        ArrayList<Exception> exceptions = null;
+
+        if (serializerClasses != null && !serializerClasses.isEmpty()) {
+            exceptions = new ArrayList<Exception>();
+            for (Object o : serializerClasses) {
+                try {
+                    if (o != null) {
+                        Class<?> serializerClass = (Class<?>) o;
+                        return (XmlSerializer) serializerClass.newInstance();
+                    }
+                } catch (InstantiationException e) {
+                    exceptions.add(e);
+                } catch (IllegalAccessException e) {
+                    exceptions.add(e);
+                } catch (ClassCastException e) {
+                    exceptions.add(e);
+                }
+            }
+        }
+
+        throw newInstantiationException("Invalid serializer class list", exceptions);
+    }
+
+    private static XmlPullParserException newInstantiationException(String message,
+            ArrayList<Exception> exceptions) {
+        if (exceptions == null || exceptions.isEmpty()) {
+            return new XmlPullParserException(message);
+        } else {
+            XmlPullParserException exception = new XmlPullParserException(message);
+            for (Exception ex : exceptions) {
+                exception.addSuppressed(ex);
+            }
+
+            return exception;
+        }
+    }
 
     /**
      * Creates a new instance of a XML Serializer.
@@ -153,7 +212,7 @@ public class XmlPullParserFactory {
      */
 
     public XmlSerializer newSerializer() throws XmlPullParserException {
-        return new KXmlSerializer();
+        return getSerializerInstance();
     }
 
     /**