[
https://issues.apache.org/jira/browse/XERCESJ-1248?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Michael Glavassevich resolved XERCESJ-1248.
-------------------------------------------
Resolution: Fixed
Fix Version/s: 2.10.0
Ludger, thanks for the patch. I reviewed it yesterday evening. It looked good
and I just committed it to SVN (see rev 924245) with one minor change: setting
the data field directly on the text node instead of calling
setNodeValueInternal().
> 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
> Assignee: Michael Glavassevich
> Fix For: 2.10.0
>
> Attachments: staticTextNodePatch.txt
>
>
> 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.mutationeventi...@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.mutationeventi...@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.mutationeventi...@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.mutationeventi...@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: [email protected]
For additional commands, e-mail: [email protected]
