Author: lryan
Date: Sat May 16 00:26:56 2009
New Revision: 775404
URL: http://svn.apache.org/viewvc?rev=775404&view=rev
Log:
Fix for dangerous DOM cloning behavior in template processing. Cloning nodes
from a singleton template DOM across many gadget renders can cause those cloned
nodes to accumulate in the singleton''s template Document userData. Use
importNode across different documents and cloning within the same document to
avoid issue.
Modified:
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessorTest.java
Modified:
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
URL:
http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java?rev=775404&r1=775403&r2=775404&view=diff
==============================================================================
---
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
(original)
+++
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
Sat May 16 00:26:56 2009
@@ -98,7 +98,7 @@
private static enum Bypass { ALL, ONLY_SELF, NONE };
private static UserDataHandler copyOnClone = new UserDataHandler() {
public void handle(short operation, String key, Object data, Node src,
Node dst) {
- if (operation == NODE_CLONED) {
+ if (operation == NODE_IMPORTED || operation == NODE_CLONED) {
dst.setUserData(key, data, copyOnClone);
}
}
Modified:
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java
URL:
http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java?rev=775404&r1=775403&r2=775404&view=diff
==============================================================================
---
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java
(original)
+++
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java
Sat May 16 00:26:56 2009
@@ -333,12 +333,15 @@
if (handler != null) {
handler.process(result, element, this);
} else {
- Element resultNode = (Element) element.cloneNode(false);
- // Make sure that the resultNode is in the correct owner document.
- // It would be cleaner to require that the incoming element is
- // already in the correct document, but would require extra clones.
- if (resultNode.getOwnerDocument() != result.getOwnerDocument()) {
- result.getOwnerDocument().adoptNode(resultNode);
+ // Be careful cloning nodes! If a target node belongs to a different
document than the
+ // template node then use importNode rather than cloneNode as that
avoids side-effects
+ // in UserDataHandlers where the cloned template node would belong to
its original
+ // document before being adopted by the target document.
+ Element resultNode;
+ if (element.getOwnerDocument() != result.getOwnerDocument()) {
+ resultNode = (Element)result.getOwnerDocument().importNode(element,
false);
+ } else {
+ resultNode = (Element)element.cloneNode(false);
}
clearSpecialAttributes(resultNode);
Modified:
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessorTest.java
URL:
http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessorTest.java?rev=775404&r1=775403&r2=775404&view=diff
==============================================================================
---
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessorTest.java
(original)
+++
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessorTest.java
Sat May 16 00:26:56 2009
@@ -22,6 +22,7 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertFalse;
+import org.apache.shindig.common.xml.XmlUtil;
import org.apache.shindig.expressions.Expressions;
import org.apache.shindig.expressions.RootELResolver;
import org.apache.shindig.gadgets.GadgetException;
@@ -29,6 +30,9 @@
import org.apache.shindig.gadgets.parse.DefaultHtmlSerializer;
import org.apache.shindig.gadgets.parse.ParseModule;
import org.apache.shindig.gadgets.parse.nekohtml.SocialMarkupHtmlParser;
+import org.apache.shindig.gadgets.render.SanitizingGadgetRewriter;
+
+
import org.json.JSONObject;
import org.json.JSONArray;
import org.junit.Before;
@@ -43,6 +47,8 @@
import com.google.common.collect.Maps;
import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Hashtable;
import java.util.Map;
import java.util.Set;
@@ -67,12 +73,16 @@
private SocialMarkupHtmlParser parser;
private static final String TEST_NS = "http://example.com";;
-
+ protected SingletonElementHandler singletonElementHandler;
+
@Before
public void setUp() throws Exception {
expressions = new Expressions();
variables = Maps.newHashMap();
- Set<TagHandler> handlers = ImmutableSet.of((TagHandler) new
TestTagHandler());
+ singletonElementHandler = new SingletonElementHandler();
+ Set<TagHandler> handlers = ImmutableSet.<TagHandler>of(
+ new TestTagHandler(),
+ singletonElementHandler);
registry = new DefaultTagRegistry(handlers);
processor = new DefaultTemplateProcessor(expressions);
@@ -202,6 +212,44 @@
assertEquals("<input class=\"false\" disabled=\"disabled\">", output);
}
+ /**
+ * Ensure that the element cloning handling of processChildren correctly
+ * copies and element to the target element, including making sure that
+ * document references are properly cleaned up and user_data in the original
+ * content does not refer to the target document
+ * @throws Exception
+ */
+ @Test
+ public void testSafeCrossDocumentCloning() throws Exception {
+ String template = "<test:Bar text='${foo.title}' data='${user}'/>";
+ executeTemplate(template, "xmlns:test='" + TEST_NS + "'");
+ executeTemplate(template, "xmlns:test='" + TEST_NS + "'");
+
+ // This is a little hacky but is fine for testing purposes. Assumes that
DOM implementation
+ // is based on Xerces which will always has a userData hashtable
+ Document doc = singletonElementHandler.elem.getOwnerDocument();
+ Class docClass = doc.getClass();
+ Field userDataField = null;
+ while (userDataField == null) {
+ try {
+ userDataField = docClass.getDeclaredField("userData");
+ } catch (NoSuchFieldException nsfe) {
+ // Ignore. Try the parent
+ }
+ docClass = docClass.getSuperclass();
+ }
+ // Access is typically protected so just bypass
+ userDataField.setAccessible(true);
+ Hashtable userDataMap = (Hashtable)userDataField.get(doc);
+
+ // There should be only one element in the user data map, if there are
more then the
+ // cloning process has put them there which can be a nasty source of
memory leaks. Consider
+ // the case of this test where the singleton template is a shared and
re-used template where
+ // the template documents userData starts to accumulate cloned nodes for
every time that
+ // template is rendered
+ assertEquals(1, userDataMap.size());
+ }
+
private String executeTemplate(String markup) throws Exception {
return executeTemplate(markup, "");
}
@@ -253,4 +301,21 @@
result.appendChild(b);
}
}
+
+ /**
+ * A tag to test the correct behavior of user data and element cloning
+ */
+ private static class SingletonElementHandler extends AbstractTagHandler {
+
+ Element elem =
XmlUtil.parseSilent("<wrapper><div><a>out</a></div></wrapper>");
+
+ public SingletonElementHandler() {
+ super(TEST_NS, "Bar");
+
SanitizingGadgetRewriter.bypassSanitization((Element)elem.getFirstChild(),
true);
+ }
+
+ public void process(Node result, Element tag, TemplateProcessor processor)
{
+ processor.processChildNodes(result, elem);
+ }
+ }
}
