XalanJ2
  1. XalanJ2
  2. XALANJ-1706

[PATCH] DocumentFragment returned by extension element causes multiple SAX endDocument() events

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.7
    • Fix Version/s: 2.7.1
    • Component/s: Other
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All

      Description

      At Krysalis Barcode (http://www.krysalis.org/barcode) I have a Xalan extension
      which creates barcodes in SVG format [1]. Until recently I've used an
      extension function for that. As I was able to create an extension element for
      SAXON I wanted to do the same for the Xalan-extension. But here's the problem:

      The extension element (like the extension function where it works correctly)
      returns a DocumentFragment containing the generated barcode in SVG format. In
      the case of the extension element the DocumentFragment is processed by a
      TreeWalker which issues start/endDocument() SAX events although it shouldn't
      IMO. This behaviour causes Apache FOP to crash with an NPE because more than
      one endDocument() SAX event is sent to FOP by Xalan.

      I will attach a test case for the bugzilla test directory. Current CVS (HEAD)
      and 2.5.1 fail in this test. I will try to find a bugfix but if someone more
      intimate with the code beats me to it, so much better.

      [1] http://cvs.sourceforge.net/viewcvs.py/krysalis/krysalis-
      barcode/src/java/org/krysalis/barcode/xalan/BarcodeExt.java

      1. XALANJ-1706-patch.diff
        2 kB
        Jeremias Maerki
      2. BugzillaExtElemSAXendDocumentCheck.zip
        4 kB
        Jeremias Maerki
      3. ASF.LICENSE.NOT.GRANTED--endDocumentPatch.diff
        2 kB
        Jeremias Maerki
      4. ASF.LICENSE.NOT.GRANTED--BugzillaExtElemSAXendDocumentCheck.zip
        4 kB
        Jeremias Maerki

        Activity

        Hide
        Jeremias Maerki added a comment -

        Created an attachment (id=8802)
        Testcase

        Show
        Jeremias Maerki added a comment - Created an attachment (id=8802) Testcase
        Hide
        Christine Li added a comment -
            • This bug has been marked as a duplicate of 25710 ***
        Show
        Christine Li added a comment - This bug has been marked as a duplicate of 25710 ***
        Hide
        Jeremias Maerki added a comment -

        I don't know what triggered Christine Li to believe that this bug is a
        duplicate of 25710. That bug is something totally different IMO.

        Anyway I'm reopening this bug as it still persists in the current CVS HEAD and
        it renders Barcode4J's (formerly Krysalis Barcode) Xalan extension virtually
        useless in conjunction with Apache FOP if output from Xalan is piped into FOP
        using SAX.

        I've also finally managed to find the time to fix the bug. Since I haven't
        managed, yet, to run all the test suites I can't tell if the patch breaks
        anything. At least it passes the test case I've attached when submitting this
        bug. I will give the test suites another try tomorrow when I'm fully awake
        again.

        Show
        Jeremias Maerki added a comment - I don't know what triggered Christine Li to believe that this bug is a duplicate of 25710. That bug is something totally different IMO. Anyway I'm reopening this bug as it still persists in the current CVS HEAD and it renders Barcode4J's (formerly Krysalis Barcode) Xalan extension virtually useless in conjunction with Apache FOP if output from Xalan is piped into FOP using SAX. I've also finally managed to find the time to fix the bug. Since I haven't managed, yet, to run all the test suites I can't tell if the patch breaks anything. At least it passes the test case I've attached when submitting this bug. I will give the test suites another try tomorrow when I'm fully awake again.
        Hide
        Jeremias Maerki added a comment -

        Created an attachment (id=12533)
        Proposed patch to fix the bug

        Show
        Jeremias Maerki added a comment - Created an attachment (id=12533) Proposed patch to fix the bug
        Hide
        Jeremias Maerki added a comment -

        I've been able to run the tests. With and without my patch Xalan-J's smoketest
        shows one failure. Unfortunately, "build alltest" simply had too many failures
        to be helpful. Maybe I did something wrong running these tests.

        Show
        Jeremias Maerki added a comment - I've been able to run the tests. With and without my patch Xalan-J's smoketest shows one failure. Unfortunately, "build alltest" simply had too many failures to be helpful. Maybe I did something wrong running these tests.
        Hide
        Jeremias Maerki added a comment -

        Is there anything I can do to finally get this patch applied? It is a simple patch including a test case. This fixes a very annoying problem. Thanks!

        Show
        Jeremias Maerki added a comment - Is there anything I can do to finally get this patch applied? It is a simple patch including a test case. This fixes a very annoying problem. Thanks!
        Hide
        Jeremias Maerki added a comment -

        Ping!

        Show
        Jeremias Maerki added a comment - Ping!
        Hide
        Brian Minchau added a comment -

        Jeremias,
        your "Ping!" caught my attention. For some reason I have an interest in SAX events, and that the events are, shall we say, well-formed.

        Thanks for the full testcase, and patch. Sorry one of the Xalan committers didn't have a look earlier. Assuming the patch is OK, and applied to CVS HEAD, is that sufficient, or are you wanting it in an official release, e.g. 2.7.1 ?

        One other point, since the patch was migrated from bugzilla, it is not marked as giving the rights to Apache. It seems silly, but please attach that patch yet again, with the appropriate checkbox set.

        Show
        Brian Minchau added a comment - Jeremias, your "Ping!" caught my attention. For some reason I have an interest in SAX events, and that the events are, shall we say, well-formed. Thanks for the full testcase, and patch. Sorry one of the Xalan committers didn't have a look earlier. Assuming the patch is OK, and applied to CVS HEAD, is that sufficient, or are you wanting it in an official release, e.g. 2.7.1 ? One other point, since the patch was migrated from bugzilla, it is not marked as giving the rights to Apache. It seems silly, but please attach that patch yet again, with the appropriate checkbox set.
        Hide
        Jeremias Maerki added a comment -

        Thanks Brian! I've attached a new patch created against latest CVS today. And I've reattached the test case. Since the test case is a set of three new files, one could argue that ALv2 §5 doesn't apply, but my CLA is on file.

        I'm happy if the patch makes it into CVS. I don't expect a bugfix release just because of the patch. But obviously, it will be helpful to point Barcode4J users to a Xalan release that has the bug fixed. So, if the fix makes it into the next normally scheduled release, I'm happy.

        Show
        Jeremias Maerki added a comment - Thanks Brian! I've attached a new patch created against latest CVS today. And I've reattached the test case. Since the test case is a set of three new files, one could argue that ALv2 §5 doesn't apply, but my CLA is on file. I'm happy if the patch makes it into CVS. I don't expect a bugfix release just because of the patch. But obviously, it will be helpful to point Barcode4J users to a Xalan release that has the bug fixed. So, if the fix makes it into the next normally scheduled release, I'm happy.
        Hide
        Brian Minchau added a comment -

        I reviewed the patch and approve it. I have applied it to CVS head branch.

        The fix only impacts DOM2DTM, and only its dispatchToEvents() method with is called by
        SerializerUtils.outputResultTreeFragment(), which is used in ElemCopyOf. So all in all
        it looks like the startDocument() endDocument() is not correct for outputting a fragment.

        It also passes the smoke test and conformance tests.

        Show
        Brian Minchau added a comment - I reviewed the patch and approve it. I have applied it to CVS head branch. The fix only impacts DOM2DTM, and only its dispatchToEvents() method with is called by SerializerUtils.outputResultTreeFragment(), which is used in ElemCopyOf. So all in all it looks like the startDocument() endDocument() is not correct for outputting a fragment. It also passes the smoke test and conformance tests.
        Hide
        Jeremias Maerki added a comment -

        Thanks, Brian. A question though: What's the reason you didn't commit the test case, too? Anything wrong with it?

        Show
        Jeremias Maerki added a comment - Thanks, Brian. A question though: What's the reason you didn't commit the test case, too? Anything wrong with it?
        Hide
        Brian Minchau added a comment -

        The fixed version is marked as 2.7.1.
        The affecteced version was marked as 2.7.1, which is clearly wrong.
        Changing the affected version to 2.7, which is better, though perhaps not fully accurate.

        Show
        Brian Minchau added a comment - The fixed version is marked as 2.7.1. The affecteced version was marked as 2.7.1, which is clearly wrong. Changing the affected version to 2.7, which is better, though perhaps not fully accurate.
        Hide
        Brian Minchau added a comment -

        Would the originator of this issue please verify that this issue is fixed in the 2.7.1 release, by adding a comment to this issue, so that we can close this issue.

        A lack of response by February 1, 2008 will be taken as consent that we can close this resolved issue.

        Regards,
        Brian Minchau

        Show
        Brian Minchau added a comment - Would the originator of this issue please verify that this issue is fixed in the 2.7.1 release, by adding a comment to this issue, so that we can close this issue. A lack of response by February 1, 2008 will be taken as consent that we can close this resolved issue. Regards, Brian Minchau
        Hide
        Jeremias Maerki added a comment -

        Works fine with 2.7.1. Issue can be closed. Thanks, Brian.

        Show
        Jeremias Maerki added a comment - Works fine with 2.7.1. Issue can be closed. Thanks, Brian.
        Hide
        Brian Minchau added a comment -

        Closing this issue.
        Jeremias: I'm impressed, 4 years later and you are still watching this issue!

        Show
        Brian Minchau added a comment - Closing this issue. Jeremias: I'm impressed, 4 years later and you are still watching this issue!

          People

          • Assignee:
            Brian Minchau
            Reporter:
            Jeremias Maerki
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development