[icedtea-web] RFC: PR766 javaws fails to parse an <argument> node that contains CDATA

[Deepak Bhole]

* Omair Majid <omajid at redhat.com> [2011-09-19 13:44]:
> On 09/19/2011 01:01 PM, Deepak Bhole wrote:
> >* Omair Majid<omajid at redhat.com>  [2011-09-16 18:03]:
> >>Hi,
> >>
> >>As explained in the bug report [1], icedtea-web ignores CDATA
> >>sections in JNLP files. I dug into the parser code a bit and found a
> >>few things.
> >>
> >>The code that sanitizes comments from jnlp files (sanitizeInput)
> >>actually removes CDATA sections along with comments. This is
> >>probably a bug.
> >>
> >
> >Agreed. I don't know why CDATA is being thrown away either.
> >
> >>(As an aside, I suspect that the reason this is done separately from
> >>the main parser is because the main parser is a rather strict XML
> >>parser and some of the locations where comments can appear in jnlp
> >>files are not accepted by the XML standard. See
> >>https://bugzilla.redhat.com/show_bug.cgi?id=449160 for examples of
> >>jnlp files that contain invalid comments)
> >>
> >>The NanoXML parser itself can parse and see CDATA sections. However,
> >>it is not quite perfect. It has trouble parsing when CDATA sections
> >>appear in certain places, or are surrounded by certain elements.
> >>
> >>I have attached 2 patches. The first patch adds a set of test cases,
> >>and the second makes the sanitizer skip special sections and just
> >>remove comments.
> >>
> >>Running the junit tests (including the ones in the patch) shows:
> >>Before fix: 5 Errors, 3 failures
> >>After fix: 7 Errors, 1 failure.
> >>
> >>The appearance of 2 new errors is a little surprising. But it turns
> >>out that the additional failures are being caused in JNLPMatcher
> >>tests - CDATA sections that were previously being sanitized away are
> >>now being fed to the parser which is failing to handle them. I will
> >>post a patch to address this later.
> >>
> >>Any thoughts or comments?
> >>
> >
> >So the changes to the code make it handle comments now, correct?
> 
> Yes.
> 
> >And the
> >changes to handle CDATA will be in another patch?
> 
> That's the plan. I will have to look into the code a little more
> before I can figure out exactly what needs to be corrected.
> 
> >Is there a risk that
> >passing of CDATA to the parser might start breaking existing apps (where
> >CDATA was being thrown, but the app continued to work)?
> >
> 
> I am not sure what you mean by 'breaking existing apps'.
> 
> If you are talking about jnlp applications changing their behaviour
> because now the jnlp file contains a CDATA section - that's not very
> likely. If the jnlp file contained CDATA sections, then presumably
> the application expected to see that somewhere. And if the
> application worked when the input it expected was absent, it should
> work when the input it expects is present.
> 
> If you mean the parser in icedtea-web chokes on CDATA sections (or
> handles these CDATA sections differently) - as shown in the test
> results - then yes, that might be a problem. As stated above I
> intend to write a patch with additional tests for this. As an
> additional precaution, I would like to avoid backporting this patch
> to any of the release branches.
> 

Yes, that is what I meant. However after some more thought, I retract
the question. Even if anything breaks, it will be a small % and it be in
HEAD (which is what you are planning to do anyway), and that is not a
big deal.


> Does that alleviate your concerns?
> 
> >Also, please post changelog.
> >
> 
> Whoops, still recovering from my vacation :)
> 
> do-not-remove-cdata-sections-01.patch:
> 
> 2011-09-16  Omair Majid  <omajid at redhat.com>
> 
>   * netx/net/sourceforge/nanoxml/XMLElement.java
>   (sanitizeInput): Do not remove CDATA sections along with comments.
> 
> 
> cdata-test-additions-01.patch
> 
> 2011-09-16  Omair Majid  <omajid at redhat.com>
> 
>   * tests/netx/unit/net/sourceforge/jnlp/ParserCornerCases.java
>   (testCdata): New method.
>   (testCdataNested): New method.
>   (testCDataFirstChild): New method.
>   (testCDataSecondChild): New method.
>   (testCommentInElements2): New method.
>   (testDoubleDashesInComments): New method.
>   * tests/netx/unit/net/sourceforge/jnlp/application/application0.jnlp,
>   * tests/netx/unit/net/sourceforge/jnlp/templates/template0.jnlp:
>   Change <!CDATA[ to <![CDATA[.
> 

Thanks! Okay for HEAD.

Cheers,
Deepak