You are viewing a plain text version of this content. The canonical link for it is here.
Posted to j-dev@xerces.apache.org by "Randall Theobald (JIRA)" <xe...@xml.apache.org> on 2007/07/18 16:28:04 UTC
[jira] Commented: (XERCESJ-1248) Static textNode on AttrImpl breaks
thread-safety, mutations of independant documents from within a mutation
listener, and introduces potential memory leaks
[ https://issues.apache.org/jira/browse/XERCESJ-1248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12513585 ]
Randall Theobald commented on XERCESJ-1248:
-------------------------------------------
We are seeing negative memory side-effects that we've tracked to this issue in (at least) one of our products.
> Static textNode on AttrImpl breaks thread-safety, mutations of independant documents from within a mutation listener, and introduces potential memory leaks
> -----------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: XERCESJ-1248
> URL: https://issues.apache.org/jira/browse/XERCESJ-1248
> Project: Xerces2-J
> Issue Type: Bug
> Components: DOM (Level 2 Events)
> Affects Versions: 2.9.0
> Environment: Applies to all environments
> Reporter: Stephen White
>
> AttrImpl has two modes of operation, dependant on the hasStringValue() method. This difference allows faster, lighter, operation in many cases .. as attribute values can be stored as Strings. In some situations, notably DOM Events and the inclusion of entity references in Attribute values, the value must be stored 'correctly' as one or more DOM Nodes rather than as a String.
> In some Situations an AttrImpl may be operating in the 'String based' mode, but then switch to the 'Node based' mode. This happens when a Mutation listener is registered on the Document and then a mutation event affecting the attribution in question is fired. This requires the creation of a TextImpl Node to pass to the event listener. This is all fine; however the author appears to have been concerned about the number of TextImpls that could potentially be constructed to pass to the event listener and then discarded immediately afterwards. Unfortunately the author introduced a static TextImpl called textNode which is used, and re-used, to avoid the repeated creation of TextImpls. This static field introduces several problems.
> Threading:
> This static field could potential be used by multiple independant nodes in independant documents. If these documents are manipulated in seperate threads then this same static field could potentially be used concurrently by multiple threads and so introduce a problem.
> Mutations of independant documents from within a mutation listener:
> This is a subtle issue, but is 100% re-producable. A mutation listener for one document can modify an independant document. As the documents are totally independant this shouldn't cause a problem. However, if both the mutation that triggered the event and the mutation that is initiated inside the event listener both utilise the static field on AttrImpl then the event listener will see its 'related node' change as an erroneous side-effect of the mutation to the independant document.
> Memory leak:
> If the static field is utilised during the mutation of a DOM document then that document will potentially be held in memory forever, though the 'owerNode' reference on the TextImpl object referenced from the static field. As this static field is only used in certain specific circumstances it is possible for an application to continue processing new documents without this field being reset, and therefore the application will unwittingly continue to reference the old document.
> I have produced an example application which demonstrates that manipulating an independant document from within a mutation listener can have a side effect that modifies the original event object.
> ---- BEGIN SAMPLE CODE ---
> package com.decisionsoft.xercesbug;
> import java.io.ByteArrayInputStream;
> import javax.xml.parsers.DocumentBuilder;
> import org.apache.xerces.dom.AttrImpl;
> import org.apache.xerces.dom.DocumentImpl;
> import org.apache.xerces.dom.ElementImpl;
> import org.apache.xerces.dom.events.MutationEventImpl;
> import org.apache.xerces.jaxp.DocumentBuilderFactoryImpl;
> import org.w3c.dom.Document;
> import org.w3c.dom.Element;
> import org.w3c.dom.events.Event;
> import org.w3c.dom.events.EventListener;
> public class Bug {
> // Null EventListener. Just so that we can register something.
> static class NullEventListener implements EventListener {
> public void handleEvent(Event evt) {
> System.err.println("[ NullEventListener: ignoring event ]");
> }
> }
> // EventListener that mutates an unrelated document when an event is received.
> static class MyEventListener implements EventListener {
> private Document _doc;
> public MyEventListener(Document doc) {
> _doc = doc;
> }
> public void handleEvent(Event evt) {
> System.err.println("MyEventListener: start *******");
> displayEvent(evt);
> System.err.println("MyEventListener: changing unrelated document");
> Element ele = (Element) _doc.getDocumentElement().getFirstChild();
> ele.setAttribute("xml2attr", "NewValueSetInEventListener");
> System.err.println("MyEventListener: displaying original event again");
> displayEvent(evt);
> System.err.println("MyEventListener: end *******");
> }
> }
> // Utility method to display an event
> public static void displayEvent(Event evt) {
> System.err.println(" Event: " + evt + ", on " + evt.getTarget());
> if (evt instanceof MutationEventImpl) {
> MutationEventImpl mutation = (MutationEventImpl) evt;
> System.err.println(" + Related: " + mutation.getRelatedNode());
> }
> }
> /**
> * @param args
> */
> public static void main(String[] args) throws Exception {
> String xml1 = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<root><e xml1attr=\"xml1AttrValue\"/></root>\n";
> String xml2 = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<root><f xml2attr=\"xml2AttrValue\"/></root>\n";
> DocumentBuilder db = DocumentBuilderFactoryImpl.newInstance().newDocumentBuilder();
> DocumentImpl doc1 = (DocumentImpl) db.parse(new ByteArrayInputStream(xml1.getBytes()));
> DocumentImpl doc2 = (DocumentImpl) db.parse(new ByteArrayInputStream(xml2.getBytes()));
> ElementImpl ele = (ElementImpl) doc1.getDocumentElement().getFirstChild();
> AttrImpl attr = (AttrImpl) ele.getAttributeNode("xml1attr");
> attr.addEventListener(MutationEventImpl.DOM_NODE_REMOVED, new MyEventListener(doc2), false);
> doc2.addEventListener(MutationEventImpl.DOM_ATTR_MODIFIED, new NullEventListener(), true);
> Element e = (Element) doc1.getDocumentElement().getFirstChild();
> e.setAttribute("xml1attr", "xml1AttrNewValue");
> }
> }
> ---- END SAMPLE CODE ----
> Running this with Xerces 2.9.0 gives:
> MyEventListener: start *******
> Event: org.apache.xerces.dom.events.MutationEventImpl@17172ea, on [#text: xml1AttrValue]
> + Related: xml1attr="xml1AttrValue"
> MyEventListener: changing unrelated document
> [ NullEventListener: ignoring event ]
> MyEventListener: displaying original event again
> Event: org.apache.xerces.dom.events.MutationEventImpl@17172ea, on [#text: xml2AttrValue]
> + Related: xml1attr="xml2AttrValue"
> MyEventListener: end *******
> Notice that the event has changed during handleEvent method. At the end of the handleEvent method the 'relatedNode' on the event is wrong, it proclaims to relate to the attirubte 'xml1attr' with the value 'xml2AttrValue' (a value which that attribute never contains).
> The corrent output would be:
> MyEventListener: start *******
> Event: org.apache.xerces.dom.events.MutationEventImpl@17172ea, on [#text: xml1AttrValue]
> + Related: xml1attr="xml1AttrValue"
> MyEventListener: changing unrelated document
> [ NullEventListener: ignoring event ]
> MyEventListener: displaying original event again
> Event: org.apache.xerces.dom.events.MutationEventImpl@17172ea, on [#text: xml1AttrValue]
> + Related: xml1attr="xml1AttrValue"
> MyEventListener: end *******
> Here the mutation of the independant DOM document has not had the side effect of corrupting the event being handled by the mutation listener.
> The patch I created to correct this issue is:
> diff -Naur xerces-2_9_0.orig/src/org/apache/xerces/dom/AttrImpl.java xerces-2_9_0/src/org/apache/xerces/dom/AttrImpl.java
> --- xerces-2_9_0.orig/src/org/apache/xerces/dom/AttrImpl.java 2006-11-22 23:36:58.000000000 +0000
> +++ xerces-2_9_0/src/org/apache/xerces/dom/AttrImpl.java 2007-05-04 12:20:22.000000000 +0100
> @@ -138,8 +138,6 @@
> // REVISIT: we are losing the type information in DOM during serialization
> transient Object type;
> - protected static TextImpl textNode = null;
> -
> //
> // Constructors
> //
> @@ -361,13 +359,8 @@
> oldvalue = (String) value;
> // create an actual text node as our child so
> // that we can use it in the event
> - if (textNode == null) {
> - textNode = (TextImpl)
> + TextImpl textNode = (TextImpl)
> ownerDocument.createTextNode((String) value);
> - }
> - else {
> - textNode.data = (String) value;
> - }
> value = textNode;
> textNode.isFirstChild(true);
> textNode.previousSibling = textNode;
> I believe that this patch corrects the issue described in the bug. The corrected code will instantiate a greater number of TextImpl objects in some situations, however I believe these situations rare and the potential (small) performance penalty is outweighed by the desire for the code to be correct.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: j-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: j-dev-help@xerces.apache.org