XalanJ2
  1. XalanJ2
  2. XALANJ-2493

BasisLibrary.nodeList2Iterator broken

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.1
    • Component/s: XSLTC
    • Security Level: No security risk; visible to anyone (Ordinary problems in Xalan projects. Anybody can view the issue.)
    • Labels:
      None
    • Xalan info:
      PatchAvailable

      Description

      The current implementation of nodeList2Iterator is broken, because it can not deal with attribute nodes. It relies on copyNodes which in turn tries to add attribute nodes as children of some top level node. Attributes don't live on the children axis, though, so this is against DOM and causes a DOM exception in the Xerces DOM implementation and probably most other implementations. The resulting HIERARCHY_REQUEST_ERR was noted e.g. in XALANJ-2424.

      Furthermore, the implementation is inefficient, because it manually copies each and every node from the source document to a new DTM to some new DTM. The time overhead for the copying as well as the memory overhead for the additional DOM can be avoided in cases where the nodes come from some input document, as opposed to a document completely loaded within some extension function.

      A comment in the related XALANJ-2425 suggests returning DTMIterator from extension functions and avoiding the re-import for those. I don't like this idea because it exposes a lot of Xalans internals to extension functions, and because the returned node list might be newly created, while at least some of thenodes might still be from the same document. So instead of special cases for the list type, I implemented special cases for every node of the list. If it is a proxy node of the same (Multi)DOM, we simply use its handle.

      If not, we add it to some w3c DOM and turn that into a DTM, pretty much like the current implementation does. However, I dropped copyNodes in favor of Document.importNode, to avoid code duplication and rely on supposedly more heavily tested code. I also added another level of elements, so that there is one such dummy node for every item of the source list, with always a single child or element. A few assertions help ensure this single child policy. This is especially important in the new implementation, because otherwise it would become difficult to get the proxied nodes and the newly DTMified nodes into correct order.

      Right now, the import of DOM nodes is only implemented for those nodes I expect to turn up in the DTM in pretty much the same form as they do in the w3c DOM. For all other nodes, an internal error is thrown. This especially concerns document fragment nodes. At least in w3c DOM, a document fragment can never be a child, so if DTM behaves the same, we would need to import document fragments seperately, or expand them to the list of their children instead. I'm not sure what correct behaviour would be here, so I'd rather throw an exception than implement wrong behaviour.

      I also noticed that org.apache.xml.dtm.ref.DTMManagerDefault.getDTMHandleFromNode would in theory provide an implementation for turning w3c nodes into DTM handles - exactly what we need here. That method seems to start importing from the topmost ancestor of a node, giving as much context as possible, in contrast to both current and my suggested XSLTC approach, which destroys ancestor references. That method also seems to employ caches in order to avoid importing a document repeatedly. Sadly, actually using that method throws a ClassCastException as it expects a DTM generate from a DOM source to be a DOM2DTM, which SAXImpl is not. A comment inside that method also indicates that future implementations might drop auto-importing and instead leave it to the caller to import a DOM if it hasn't been imported before.

      I left my own attempt at an nodeList2Iterator implementation using getDTMHandleFromNode in place, renamed to nodeList2IteratorUsingHandleFromNode and made private. So it's there, it even gets compiled, but it won't get used. If that method gets fixed in XSLTCDTMManager or its ancestor, then this method might be used instead, giving a much simpler and cleaner implementation. If some of my code would be useful for such an implementation as well, like the check for "is same DOM", feel free to copy or move my code to those classes as well.

      1. XALANJ-2493_1.patch
        15 kB
        Martin von Gagern
      2. XALANJ_2493_Test1.java
        5 kB
        Martin von Gagern
      3. XALANJ-2493_2.patch
        16 kB
        Martin von Gagern

        Issue Links

          Activity

          Martin von Gagern created issue -
          Martin von Gagern made changes -
          Field Original Value New Value
          Attachment XALANJ-2493_1.patch [ 12403433 ]
          Attachment XALANJ_2493_Test1.java [ 12403434 ]
          Martin von Gagern made changes -
          Link This issue incorporates XALANJ-2424 [ XALANJ-2424 ]
          Hide
          Henry Zongaro added a comment -

          Hi, Martin.
          I was just trying out your patch and was tripped up by the references to an AssertionError constructor. Where is that class defined?

          Show
          Henry Zongaro added a comment - Hi, Martin. I was just trying out your patch and was tripped up by the references to an AssertionError constructor. Where is that class defined?
          Hide
          Martin von Gagern added a comment -

          It's a language feature: http://java.sun.com/j2se/1.4.2/docs/api/java/lang/AssertionError.html

          I just realized that this is "fairly new", added in JDK 1.4, while Xalan according to its FAQ seems to still support 1.3. So I guess I'll have to rewrite my patch, either without assertions or with some other class for their errors.

          Show
          Martin von Gagern added a comment - It's a language feature: http://java.sun.com/j2se/1.4.2/docs/api/java/lang/AssertionError.html I just realized that this is "fairly new", added in JDK 1.4, while Xalan according to its FAQ seems to still support 1.3. So I guess I'll have to rewrite my patch, either without assertions or with some other class for their errors.
          Hide
          Michael Glavassevich added a comment -

          Probably not a good idea to start introducing dependencies on higher JDKs without discussing with the community. Xerces is also still on JDK 1.3 (because we have users who are still on that level; probably some of them use Xalan too) and relies on Xalan's serializer also being at that level.

          Show
          Michael Glavassevich added a comment - Probably not a good idea to start introducing dependencies on higher JDKs without discussing with the community. Xerces is also still on JDK 1.3 (because we have users who are still on that level; probably some of them use Xalan too) and relies on Xalan's serializer also being at that level.
          Hide
          Henry Zongaro added a comment -

          Right. I've always done my Xalan-J builds and test runs using JDK 1.3.x, to ensure I don't let any 1.4 or later features creep in.

          There's an InternalError class in the org.apache.xalan.xsltc.compiler.util package that's intended to be used for situations where the compiler has found itself in some internally inconsistent state. Traditionally we've avoided introducing dependencies on the compiler packages in the XSLTC run-time packages - just to allow for the possibility of building a smaller XSLTC run-time, though that hasn't been done recently. So I would suggest introducing an InternalRuntimeError class in org.apache.xalan.xsltc.runtime instead of using AssertionError.

          Show
          Henry Zongaro added a comment - Right. I've always done my Xalan-J builds and test runs using JDK 1.3.x, to ensure I don't let any 1.4 or later features creep in. There's an InternalError class in the org.apache.xalan.xsltc.compiler.util package that's intended to be used for situations where the compiler has found itself in some internally inconsistent state. Traditionally we've avoided introducing dependencies on the compiler packages in the XSLTC run-time packages - just to allow for the possibility of building a smaller XSLTC run-time, though that hasn't been done recently. So I would suggest introducing an InternalRuntimeError class in org.apache.xalan.xsltc.runtime instead of using AssertionError.
          Hide
          Martin von Gagern added a comment -

          OK, I rewrote the patch, now using a new class org.apache.xalan.xsltc.runtime.InternalRuntimeError as you suggested.

          I also rigged myself a build environment which is still using my local (JDK 6) javac but a version of the JDK 1.3 runtime on the bootclasspath. This is achieved by setting sun.boot.class.path and by passing source and target arguments to the javac tasks.

          Just for the record: build.xml talks about compatibility with JDK 1.1.x in some comment, but that seems to be out of date. Current code (without my patches) won't compile against the JDK 1.2 runtime for lack of a java.net.URL.getPath() method. So 1.3 is now the official minimum requirement, right?

          I guess I'll adjust the comments in the buildfile, and try to think of some clever way to handle my bootclasspath override in a generic fashion suitable for all devs without requiring the whole JDK 1.3 runtime to be present in the repository. Will post a separate bug report for that patch, though.

          Show
          Martin von Gagern added a comment - OK, I rewrote the patch, now using a new class org.apache.xalan.xsltc.runtime.InternalRuntimeError as you suggested. I also rigged myself a build environment which is still using my local (JDK 6) javac but a version of the JDK 1.3 runtime on the bootclasspath. This is achieved by setting sun.boot.class.path and by passing source and target arguments to the javac tasks. Just for the record: build.xml talks about compatibility with JDK 1.1.x in some comment, but that seems to be out of date. Current code (without my patches) won't compile against the JDK 1.2 runtime for lack of a java.net.URL.getPath() method. So 1.3 is now the official minimum requirement, right? I guess I'll adjust the comments in the buildfile, and try to think of some clever way to handle my bootclasspath override in a generic fashion suitable for all devs without requiring the whole JDK 1.3 runtime to be present in the repository. Will post a separate bug report for that patch, though.
          Martin von Gagern made changes -
          Attachment XALANJ-2493_2.patch [ 12426122 ]
          Hide
          Henry Zongaro added a comment -

          I have reviewed and applied Martin's second patch for this bug report.

          Show
          Henry Zongaro added a comment - I have reviewed and applied Martin's second patch for this bug report.
          Henry Zongaro made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s The Latest Development Code [ 12312206 ]
          Resolution Fixed [ 1 ]
          Henry Zongaro made changes -
          Mark Thomas made changes -
          Workflow jira [ 12457109 ] Default workflow, editable Closed status [ 12570466 ]
          Mark Thomas made changes -
          Workflow Default workflow, editable Closed status [ 12570466 ] jira [ 12594534 ]
          Gary Gregory made changes -
          Fix Version/s 2.7.2 [ 12323150 ]
          Affects Version/s 2.7.1 [ 10863 ]
          Affects Version/s The Latest Development Code [ 12312206 ]
          Hide
          Uwe Schindler added a comment -

          Hi,
          after upgrading to XALAN 2.7.2, I got very strange errors in one of my XSL transformations, but just for some documents. It took a while to find the problem:
          I have an extension function that returns a NodeList with some elements and text nodes (some HTML). Unfortunately this NodeList contained an empty text node as very last element. This is perfectly fine for DOM, because there is nothing that forbids empty text nodes. This empty node caused the following error when invoking <xsl:copy-of select=java:Class:myExtensionFunction()"/>:

          InternalRuntimeError: Expected element missing at 6

          (6 is the last node in the Node-Set, the empty text node)

          I fixed the issue by calling normalize() before returning the NodeList as quick hack. Later I did a better fix to not create the empty nodes at all.

          In XALAN 2.7.1 this worked without any problem.

          Should I open a new issue for that?

          Show
          Uwe Schindler added a comment - Hi, after upgrading to XALAN 2.7.2, I got very strange errors in one of my XSL transformations, but just for some documents. It took a while to find the problem: I have an extension function that returns a NodeList with some elements and text nodes (some HTML). Unfortunately this NodeList contained an empty text node as very last element. This is perfectly fine for DOM, because there is nothing that forbids empty text nodes. This empty node caused the following error when invoking <xsl:copy-of select=java:Class:myExtensionFunction()"/>: InternalRuntimeError: Expected element missing at 6 (6 is the last node in the Node-Set, the empty text node) I fixed the issue by calling normalize() before returning the NodeList as quick hack. Later I did a better fix to not create the empty nodes at all. In XALAN 2.7.1 this worked without any problem. Should I open a new issue for that?
          Hide
          Gary Gregory added a comment -

          Yes please create a new ticket with full details.

          FYI:
          The code is here: https://svn.apache.org/repos/asf/xalan/java/branches/xalan-j_2_7_1_maint
          The tests are here: https://svn.apache.org/repos/asf/xalan/test/branches/xalan-j_2_7_x

          Thank you!

          Gary

          Show
          Gary Gregory added a comment - Yes please create a new ticket with full details. FYI: The code is here: https://svn.apache.org/repos/asf/xalan/java/branches/xalan-j_2_7_1_maint The tests are here: https://svn.apache.org/repos/asf/xalan/test/branches/xalan-j_2_7_x Thank you! Gary
          Uwe Schindler made changes -
          Link This issue breaks XALANJ-2590 [ XALANJ-2590 ]
          Hide
          Uwe Schindler added a comment -

          Opened XALANJ-2590

          Show
          Uwe Schindler added a comment - Opened XALANJ-2590
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          247d 8h 11m 1 Henry Zongaro 25/Nov/09 21:56

            People

            • Assignee:
              Unassigned
              Reporter:
              Martin von Gagern
              Reviewer:
              Henry Zongaro
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development