[RFC][icedtea-web] Plugin doesn't halt when required elements from JNLP files are missing.
Danesh Dadachanji
ddadacha at redhat.com
Fri Mar 9 12:32:03 PST 2012
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?
>>>>>
>>>>> Regards,
>>>>> Danesh
>>>>>
>>>>> [1]
>>>>> http://docs.oracle.com/javase/7/docs/technotes/guides/javaws/developersguide/syntax.html
>>>>>
>>>>>
>>>>>
>>>>
>>>> I do not like the idea of forcing somebody to have elements, when they
>>>> can be still empty. But if documentation say so.... How proprietary
>>>> plugin is dealing with it?
>>>
>>> It throws an exception, stopping further execution - the same as what
>>> this patch does for icedtea-web. =)
>>>
>>> If you'd like to test it, remove a title/vendor tag from a JNLP file
>>> and run it. You may need to with the java console being set to "Show
>>> console" - especially if it's an applet.
>>>
>>> Regards,
>>> Danesh
>>
>> Jsut from couriosity - how does proprietary plugin is dealing when
>> vendor or title is empty?
>
> Nice catch, I did not think about this. The proprietary plugin behaves
> the same way if either are empty - it throws a fatal exception. I've
> attached an updated patch to take this into consideration. Note that if
> the tags are only white spaces, this is also considered empty but the
> parser handles that case already. (e.g. <title> </title>)
>
>> I still don't like the idea of exiting when element is not present, but
>> continue when element is empty :(
>>
>
> IMO, it helps end-users of applets/webstarts. It forces devs to add more
> information to their jnlp files and therefore gives end-users more
> detailed warning dialogs when the app is asking for more access. It also
> acts as a reminder for any dev that's forgotten or does not know about
> this attribute. Obviously this can be used to spoof end-users but that's
> what my other patch is for =)
>
>> But specification is clear - they are required and existence of content
>> is not mentioned.
>>
>> In that case I think it is ok for head. But please wait until branching
>> is done.
>>
>
> Once the updated patch is good with you, I'll push to HEAD after
> branching is finished.
>
>> Thanx for reading the specification :)
>>
>
> Thanks for the comments! They are much appreciated. =)
Realized a bunch of our tests do not include <vendor> so the attachment
is a new patch adding these.
I've also added a regression test that has a JNLP without <title> and
another without <vendor>. The testcase checks for the exception message
thrown. Right now, the tests will not pass without the launch error
patch that's in review[1] but I figured I'd post to get some comments.
Any thoughts? Can I push to HEAD once [1] is in there?
ChangeLog:
+2012-03-09 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:
+ Added PNoTitleElement and PNoVendorElement
+ * tests/jnlp_tests/signed/CacheReproducer/resources/CacheReproducer1.jnlp,
+ *
tests/jnlp_tests/signed/CacheReproducer/resources/CacheReproducer1_1.jnlp,
+ * tests/jnlp_tests/signed/CacheReproducer/resources/CacheReproducer2.jnlp,
+ *
tests/jnlp_tests/signed/CacheReproducer/resources/CacheReproducer2_1.jnlp,
+ * tests/jnlp_tests/signed/MissingJar/resources/MissingJar.jnlp,
+ * tests/jnlp_tests/signed/MissingJar/resources/MissingJar2.jnlp,
+ * tests/jnlp_tests/signed/MissingJar/resources/MissingJar3.jnlp,
+ * tests/jnlp_tests/signed/MissingJar/resources/MissingJar4.jnlp,
+ *
tests/jnlp_tests/signed/ReadPropertiesBySignedHack/resources/ReadPropertiesBySignedHack.jnlp,
+ *
tests/jnlp_tests/signed/ReadPropertiesSigned/resources/ReadPropertiesSigned1.jnlp,
+ *
tests/jnlp_tests/signed/ReadPropertiesSigned/resources/ReadPropertiesSigned2.jnlp,
+ *
tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackage.jnlp,
+ * tests/jnlp_tests/simple/AddShutdownHook/resources/AddShutdownHook.jnlp,
+ * tests/jnlp_tests/simple/AllStackTraces/resources/AllStackTraces.jnlp
+ *
tests/jnlp_tests/simple/CreateClassLoader/resources/CreateClassLoader.jnlp,
+ * tests/jnlp_tests/simple/ReadEnvironment/resources/ReadEnvironment.jnlp,
+ * tests/jnlp_tests/simple/ReadProperties/resources/ReadProperties1.jnlp,
+ * tests/jnlp_tests/simple/ReadProperties/resources/ReadProperties2.jnlp,
+ * tests/jnlp_tests/simple/RedirectStreams/resources/RedirectStreams.jnlp,
+ *
tests/jnlp_tests/simple/ReplaceSecurityManager/resources/ReplaceSecurityManager.jnlp,
+ *
tests/jnlp_tests/simple/SetContextClassLoader/resources/SetContextClassLoader.jnlp,
+ * tests/netx/unit/net/sourceforge/jnlp/templates/template8.jnlp:
+ Added missing title/vendor tags that make them fail with this changeset.
+ * tests/jnlp_tests/simple/TitleVendorParser/resources/TitleParser.jnlp,
+ * tests/jnlp_tests/simple/TitleVendorParser/resources/VendorParser.jnlp,
+ * tests/jnlp_tests/simple/TitleVendorParser/srcs/TitleVendorParser.java,
+ *
tests/jnlp_tests/simple/TitleVendorParser/testcases/TitleVendorParserTest.java:
+ New test that runs two JNLPs, one without the title tag and the other
+ without the vendor tag, checking that the correct eception is thrown.
+
Cheers,
Danesh
[1]
http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2012-March/017620.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: title-vendor-required-03.patch
Type: text/x-patch
Size: 25689 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120309/97d4b10b/title-vendor-required-03.patch
More information about the distro-pkg-dev
mailing list