[RFC][icedtea-web] Plugin doesn't halt when required elements from JNLP files are missing.

[Danesh Dadachanji]

On 12/03/12 05:26 AM, Jiri Vanek wrote:
> On 03/09/2012 09:32 PM, Danesh Dadachanji wrote:
>> On 11/01/12 02:18 PM, Danesh Dadachanji wrote:
>>> On 11/01/12 11:31 AM, Jiri Vanek wrote:
>>>> On 01/06/2012 08:08 PM, Danesh Dadachanji wrote:
>>>>> On 06/01/12 12:37 PM, Jiri Vanek wrote:
>>>>>> On 01/06/2012 05:44 PM, Danesh Dadachanji wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Looking at the plugin docs[1], <title> and <vendor> are required
>>>>>>> elements of <information>, without them the plugin should stop
>>>>>>> with a
>>>>>>> fatal error. Currently, icedtea-web just returns null from their
>>>>>>> respective getters and handles null accordingly. However it
>>>>>>> should not
>>>>>>> even reach that far. Here's a simple patch that throws a
>>>>>>> ParseException when not found.
>>>>>>>
>>>>>>> ChangeLog
>>>>>>> +2012-01-06 Danesh Dadachanji <ddadacha at redhat.com>
>>>>>>> +
>>>>>>> + Applications using JNLP files without a title or vendor section
>>>>>>> + still run, despite them being required elements.
>>>>>>> + * netx/net/sourceforge/jnlp/Parser.java:
>>>>>>> + (getInformationDesc): If title or vendor are not found in info,
>>>>>>> + a new ParseException is thrown.
>>>>>>> + * netx/net/sourceforge/jnlp/resources/Messages.properties: Add
>>>>>>> + PNoTitleElement and PNoVendorElement
>>>>>>> +
>>>>>>>
>>>>>>> Any comments? Is this okay for HEAD?
> In title of this email you write "Plugin" but it seems to me this is fix
> IN java web start with impact to plugin.

Woops, yes it is meant to be in both webstart and plugin. I noticed this 
while running plugin applets but developed it with general icedtea-web 
in mind.

> If it is also for applets, can you add javaws someAppelt.jnlp tests? (as
> AppeltTest is dooing now) and test the same exceptions as in case of
> javaws some-application.jnlp? You can use already translated jar-ed
> applet and just call jnlp fiels upon him (same as I'm suggesting for
> your current javaws tests-set)

Hmm am I misunderstanding this? I think you're asking for what 
TitleVendorParser does, would you like something on top of this? Just to 
clarify, TitleVendorParser is for javaws right now, not for the plugin 
(as the misleading subject says!)

> Soon there will be intorudcued in-browser applet testing, do you think
> to remind me to add html tests for this case later? (Or you can do this,
> if you will want to check the in-browser "engine" ;)
>

I had it on my to-do along with the CheckServices test. ;)

> Few more comment in-line:

[snip]

>>
>> + if (info.getTitle() == null || info.getTitle().equals(""))
>> + throw new ParseException(R("PNoTitleElement"));
>> + if (info.getVendor() == null || info.getVendor().equals(""))
>> + throw new ParseException(R("PNoVendorElement"));
>> +
>> return info;
>> }
>>
> I would suggest "".equals(info.get*().trim()) instead of
> info.get*().equals("")
> Usage of trim i straight forward and I think it is necessary.
> The "".equals instead of string.equals("") really just minor nitpick :)
> "".equals(null) returns false instead of NUllPointerException. I do not
> believe trim() will ever return null, but it is good habit (IMHO O:) )
> If no need for trim will be here, then just "".equals(info.get*()) will
> be enough instead of both conditions.
>

Commenting on this in the other responses from yourself and Pavel.

[snip]

>> a/tests/jnlp_tests/simple/SetContextClassLoader/resources/SetContextClassLoader.jnlp
>>
>> +++
>> b/tests/jnlp_tests/simple/SetContextClassLoader/resources/SetContextClassLoader.jnlp
>>
>> @@ -4,6 +4,7 @@
>> href="SetContextClassLoader.jnlp">
>> <information>
>> <title>set context classloader</title>
>> +<vendor>Red Hat</vendor>
>> </information>
>> <resources>
>> <jar href="SetContextClassLoader.jar" main="true" download="eager"/>
>
> Yap, tahnx for this. I think you can push them asap - independently on
> rest of the fix.
>

Okay, I'll do that now.

[snip..again]

>
> Now you will hate me :
> I think it is worthy to add also one testy with both missing, and few
> tests which will try to paly with count and order of mandatory and
> obligatory elements.
> Also I woud asuggest to introduce
> String s1 = "Sould not be reached but JNLP was parsed fine.";
> Assert.assertFalse("testForVendor stdout should not contain " + s1 + "
> but did.", pr.stdout.contains(s1));
> String s2 = "net.sourceforge.jnlp.ParseException: The vendor section has
> not been defined in the JNLP file.";
> Assert.assertTrue("testForVendor stderr should contain " + s2 + " but
> did not.", pr.stderr.contains(s2));
> Assert.assertFalse(pr.wasTerminated);
> Assert.assertEquals((Integer)0, pr.returnValue);
>
> as separate method (especialy if more tests will be added)
>

Ah smart thinking!

>
> Do I understand correctly that when information element is missing, then
> jnlp is processed correctly?

No, information is also required from the spec[1].

> For both yes and no - can you add test for it?

Sure.

> If yes - what does getInformationDesc() returns?? null?? I believe jnlp
> should not be processed :-/

Yes, it should not be processed. Currently, javaws throws an exception 
when <information> is missing. I believe that's where I got the idea for 
this from. =)

> Also I think you do not need to add new jar with new srcs class
> (TitleVendorParser) (==just remove thsi file and no jar will be
> created), but you can yuse eg simpletest1) and just use your new jnlps
> to call this old jar. But this is really up to you, if you want to have
> by-your-test-controlled jar, then as you wish. (Me, personally, should
> probably stay with my own jar ;)

Good idea, I can probably do without making new ones. If it gives me too 
much trouble, I might be lazy and just use my own. =)

> I got used to check for ClassNotFound exception, which is here for
> reason that netx.jar is broken, javaws is broken or the whole test is
> compeltly broken. But when you are checking for stdout of your jar, then
> it is not necessary, it just helps to see imidietly that soemting more
> bad then just test failure have occured. Do you think it is worthy to
> add? By other words - Should I get rid of this "habit" ?

Hmm so it's basically a sanity check? I'm not sure TBH, on the one hand 
it is easier to identify issues outside of the test's scope. However, on 
the other hand, it's a redundant test that should be passing all the 
time once you have the test working properly. Does it have any use after 
the test runs properly? If it doesn't, I don't see the point in keeping it.

>> diff --git
>> a/tests/netx/unit/net/sourceforge/jnlp/templates/template8.jnlp
>> b/tests/netx/unit/net/sourceforge/jnlp/templates/template8.jnlp
>> --- a/tests/netx/unit/net/sourceforge/jnlp/templates/template8.jnlp
>> +++ b/tests/netx/unit/net/sourceforge/jnlp/templates/template8.jnlp
>> @@ -1,5 +1,9 @@
>> <?xml version="1.0"?>
>> <jnlp spec="1.5+" codebase="file" href="www.redhat.com">
>> +<information>
>> +<title>Sample Test</title>
>> +<vendor>RedHat</vendor>
>> +</information>
>> <resources>
>> <j2se version='1.6+' />
>> <jar href='myResource.jar' main='true' />
> Thsi one should be pushed with rest of in-tests vendor/title fixes
>
>
> TYVM!
>

My pleasure! I'll push these minor fixes now. Thanks for the feedback!

Cheers,
Danesh

[1] 
http://docs.oracle.com/javase/6/docs/technotes/guides/javaws/developersguide/syntax.html