Various XML fixes.

Add tests for bug 2487, exploring the situations where "]]>" is and isn't
allowed. Fix the bug by changing KXmlParser so it pays attention to
whether it's dealing with normal text or text in an attribute value and
reports errors appropriately.

In order to pass the new tests, we also need to fix DocumentBuilder to
pay attention to its DocumentBuilderFactory's "coalescing" setting: whether
or not adjacent text/CDATA nodes should be coalesced.

This in turn fixes a @KnownFailure in DocumentBuilderFactoryTest: previously
we didn't allow the caller to turn "coalescing" off (though until my
previous patch, we didn't actually coalesce anyway). Now we support both,
and I've made coalescing the default, because bug reports tell us that's
what users want. It's how the RI behaves, too.

Bug: 2487

3 files changed, 51 insertions, 27 deletions

diff --git a/xml/src/main/java/org/apache/harmony/xml/parsers/DocumentBuilderFactoryImpl.java b/xml/src/main/java/org/apache/harmony/xml/parsers/DocumentBuilderFactoryImpl.java
index 4b8eef3..5cdba97 100644
--- a/xml/src/main/java/org/apache/harmony/xml/parsers/DocumentBuilderFactoryImpl.java
+++ b/xml/src/main/java/org/apache/harmony/xml/parsers/DocumentBuilderFactoryImpl.java
@@ -70,10 +70,9 @@ public class DocumentBuilderFactoryImpl extends DocumentBuilderFactory {
          * or by throwing the full SPI monty at it.  
          */
         DocumentBuilderImpl builder = new DocumentBuilderImpl();
-        
+        builder.setCoalescing(isCoalescing());
         builder.setIgnoreComments(isIgnoringComments());
-        builder.setIgnoreElementContentWhitespace(
-                isIgnoringElementContentWhitespace());
+        builder.setIgnoreElementContentWhitespace(isIgnoringElementContentWhitespace());
         builder.setNamespaceAware(isNamespaceAware());
 
         // TODO What about expandEntityReferences?
diff --git a/xml/src/main/java/org/apache/harmony/xml/parsers/DocumentBuilderImpl.java b/xml/src/main/java/org/apache/harmony/xml/parsers/DocumentBuilderImpl.java
index 17438b7..1a8122c 100644
--- a/xml/src/main/java/org/apache/harmony/xml/parsers/DocumentBuilderImpl.java
+++ b/xml/src/main/java/org/apache/harmony/xml/parsers/DocumentBuilderImpl.java
@@ -49,6 +49,8 @@ class DocumentBuilderImpl extends DocumentBuilder {
 
     private static DOMImplementation dom = DOMImplementationImpl.getInstance();
 
+    private boolean coalescing;
+
     private EntityResolver entityResolver;
 
     private ErrorHandler errorHandler;
@@ -261,20 +263,15 @@ class DocumentBuilderImpl extends DocumentBuilder {
                  * whitespace at all.
                  */
                 if (!ignoreElementContentWhitespace) {
-                    appendText(document, node, parser.getText());
+                    appendText(document, node, true, parser.getText());
                 }
-            } else if (token == XmlPullParser.TEXT) {
-                /*
-                 * Found a piece of text. That's the easiest case. We simply
-                 * take it and create a corresponding node.
-                 */
-                appendText(document, node, parser.getText());
-            } else if (token == XmlPullParser.CDSECT) {
+            } else if (token == XmlPullParser.TEXT || token == XmlPullParser.CDSECT) {
                 /*
-                 * Found a CDATA section. That's also trivial. We simply
-                 * take it and create a corresponding node.
+                 * Found a piece of text (possibly encoded as a CDATA section).
+                 * That's the easiest case. We simply take it and create a new text node,
+                 * or merge with an adjacent text node.
                  */
-                node.appendChild(document.createCDATASection(parser.getText()));
+                appendText(document, node, token == XmlPullParser.TEXT, parser.getText());
             } else if (token == XmlPullParser.ENTITY_REF) {
                 /*
                  * Found an entity reference. If an entity resolver is
@@ -289,7 +286,7 @@ class DocumentBuilderImpl extends DocumentBuilder {
 
                 String replacement = resolveStandardEntity(entity);
                 if (replacement != null) {
-                    appendText(document, node, replacement);
+                    appendText(document, node, true, replacement);
                 } else {
                     node.appendChild(document.createEntityReference(entity));
                 }
@@ -374,20 +371,30 @@ class DocumentBuilderImpl extends DocumentBuilder {
         }
     }
 
-    private void appendText(Document document, Node node, String text) {
+    /**
+     * @param isText true for a normal TextNode, false for a CDATA section.
+     * (If we're not coalescing, it matters which kind of node we put into the DOM.)
+     */ 
+    private void appendText(Document document, Node node, boolean isText, String text) {
         // Ignore empty runs.
         if (text.length() == 0) {
             return;
         }
         // Merge with any previous text node if possible.
-        Node lastChild = node.getLastChild();
-        if (lastChild != null && lastChild.getNodeType() == Node.TEXT_NODE) {
-            Text textNode = (Text) lastChild;
-            textNode.setData(textNode.getNodeValue() + text);
-            return;
+        if (coalescing) {
+            Node lastChild = node.getLastChild();
+            if (lastChild != null && lastChild.getNodeType() == Node.TEXT_NODE) {
+                Text textNode = (Text) lastChild;
+                textNode.setData(textNode.getNodeValue() + text);
+                return;
+            }
         }
         // Okay, we really do need a new text node
-        node.appendChild(document.createTextNode(text));
+        if (isText) {
+            node.appendChild(document.createTextNode(text));
+        } else {
+            node.appendChild(document.createCDATASection(text));
+        }
     }
 
     @Override
@@ -409,6 +416,10 @@ class DocumentBuilderImpl extends DocumentBuilder {
         ignoreComments = value;
     }
 
+    public void setCoalescing(boolean value) {
+        coalescing = value;
+    }
+
     /**
      * Controls whether this DocumentBuilder ignores element content whitespace.
      * 
diff --git a/xml/src/main/java/org/kxml2/io/KXmlParser.java b/xml/src/main/java/org/kxml2/io/KXmlParser.java
index 98aae04..c4d8f3d 100644
--- a/xml/src/main/java/org/kxml2/io/KXmlParser.java
+++ b/xml/src/main/java/org/kxml2/io/KXmlParser.java
@@ -319,7 +319,9 @@ public class KXmlParser implements XmlPullParser {
                     return;
 
                 case TEXT :
-                    pushText('<', !token);
+                    // BEGIN android-changed: distinguish attribute values from normal text.
+                    pushText('<', !token, false);
+                    // END android-changed
                     if (depth == 0) {
                         if (isWhitespace)
                             type = IGNORABLE_WHITESPACE;
@@ -680,7 +682,9 @@ public class KXmlParser implements XmlPullParser {
 					read();
 				
                 int p = txtPos;
-                pushText(delimiter, true);
+                // BEGIN android-changed: distinguish attribute values from normal text.
+                pushText(delimiter, true, true);
+                // END android-changed
 
                 attributes[i] = get(p);
                 txtPos = p;
@@ -800,7 +804,7 @@ public class KXmlParser implements XmlPullParser {
     ' ': parse to whitespace or '>'
     */
 
-    private final void pushText(int delimiter, boolean resolveEntities)
+    private final void pushText(int delimiter, boolean resolveEntities, boolean inAttributeValue)
         throws IOException, XmlPullParserException {
 
         int next = peek(0);
@@ -812,21 +816,31 @@ public class KXmlParser implements XmlPullParser {
                 if (next <= ' ' || next == '>')
                     break;
 
+            // BEGIN android-changed: "<" is not allowed in attribute values.
             if (next == '&') {
                 if (!resolveEntities)
                     break;
 
                 pushEntity();
             }
+            else if (next == '<' && inAttributeValue) {
+                error("Illegal: \"<\" inside attribute value");
+            }
             else if (next == '\n' && type == START_TAG) {
                 read();
                 push(' ');
             }
             else
                 push(read());
+            // END android-changed
 
-            if (next == '>' && cbrCount >= 2 && delimiter != ']')
-                error("Illegal: ]]>");
+            // BEGIN android-changed: "]]>" *is* allowed in attribute values, but
+            // is not allowed in regular text between markup.
+            final boolean allowCloseCdata = inAttributeValue;
+            if (!allowCloseCdata && (next == '>' && cbrCount >= 2 && delimiter != ']')) {
+                error("Illegal: \"]]>\" outside CDATA section");
+            }
+            // END android-changed
 
             if (next == ']')
                 cbrCount++;