 | | | Updated: (XERCESJ-1248) Static textNode on AttrImpl breaks thread-safety, | Updated: (XERCESJ-1248) Static textNode on AttrImpl breaks thread-safety, 2007-05-20 - By Michael Glavassevich (JIRA)
[ https://issues.apache.org/jira/browse/XERCESJ-1248?page=com.atlassian
.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Glavassevich updated XERCESJ-1248:
------------------------------------------
Fix Version/s: (was: 2.9.0)
Removing the "Fix Version". This bug hasn't been fixed yet.
> 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());
> }
> }
> /**
> * @(protected) 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@(protected), 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@(protected), 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@(protected), 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@(protected), 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@(protected)
For additional commands, e-mail: j-dev-help@(protected)
|
|
|