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

[Omair Majid]

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.

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[.

Cheers,
Omair