[RFC][icedtea-web]: Fix PR909 - The Java applet at http://de.gosupermodel.com/games/wardrobegame.jsp fails

[Jiri Vanek]

On 09/13/2012 03:59 PM, Saad Mohammad wrote:
> Hello,
>
> The patch attached fixes PR909.
> <http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=909>
>
> The problem is that every URL was being validated and percent-encoded
> (normalized). This was changing the URLs to point to invalid locations
> causing resources not to be found/loaded.

I must disagree with this solution. It is removing consequence rather then fixing the issue. The url 
encoding/decoding is correct think here. And I do not believe that theirs server is not decoding url 
properly. So the error will be in handling of url in jnlp-href - eg some second encoding of url or 
something like it? Or maybe current encoding/decoding is wrong? But I would like to avoid your 
fallback unless nothing else can be done here.

Do you mind to prepare also reproducer with the fix?

Thanx for touching the issue and sorry for disagreeing!

J.


>
> Thanks.
>
> --
> Cheers,
> Saad Mohammad
>
>
> PR909Changelog0-1.patch
>
>
> diff --git a/ChangeLog b/ChangeLog
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2012-09-12  Saad Mohammad<smohammad at redhat.com>
> +
> +	Fix PR909 - URL is invalid after normalization.
> +	* netx/net/sourceforge/jnlp/cache/ResourceTracker.java (addResource):
> +	Avoids normalizing resource location URLs unless they are invalid.
> +
>   2012-09-07  Saad Mohammad<smohammad at redhat.com>
>
>   	Added signed jnlp tests for applications with multiple jar resources.
> diff --git a/NEWS b/NEWS
> --- a/NEWS
> +++ b/NEWS
> @@ -17,6 +17,7 @@
>   * Common
>     - PR1049: Extension jnlp's signed jar with the content of only META-INF/* is considered
>     - PR955: regression: SweetHome3D fails to run
> +  - PR909: The Java applet athttp://de.gosupermodel.com/games/wardrobegame.jsp  fails
>
>   New in release 1.3 (2012-XX-XX):
>   * NetX
>
>
> PR909Fix0-1.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> @@ -185,14 +185,58 @@
>       public void addResource(URL location, Version version, DownloadOptions options, UpdatePolicy updatePolicy) {
>           if (location == null)
>               throw new IllegalResourceDescriptorException("location==null");
> -        try {
> -            location = normalizeUrl(location, JNLPRuntime.isDebug());
> -        } catch (Exception ex) {
> -            System.err.println("Normalization of " + location.toString() + " have failed");
> -            ex.printStackTrace();
> +
> +        boolean downloaded = false;
> +        boolean needsNormalization = true;
> +
> +        //Attempts to continue without normalizing (validating) the URL
> +        //If the resource is already being tracked or is already downloaded,
> +        //assume normalization is not required
> +
> +        Resource resource = Resource.getResource(location, version, updatePolicy);
> +
> +        synchronized (resources) {
> +            if (resources.contains(resource))
> +                needsNormalization = false;
>           }
> -        Resource resource = Resource.getResource(location, version, updatePolicy);
> -        boolean downloaded = false;
> +
> +        // checkCache may take a while (loads properties file).  this
> +        // should really be synchronized on resources, but the worst
> +        // case should be that the resource will be updated once even
> +        // if unnecessary.
> +        downloaded = checkCache(resource, updatePolicy);
> +
> +        //Checks if the URL is invalid. If so, normalize URL
> +        if (needsNormalization && !downloaded && !"file".equals(location.getProtocol())) {
> +            try {
> +                HttpURLConnection resourceConnect = (HttpURLConnection) location.openConnection();
> +                int responseCode = resourceConnect.getResponseCode();
> +
> +                if (responseCode < 199 || responseCode > 300) {
> +                    if (JNLPRuntime.isDebug())
> +                        System.err.println("Http errored: " + responseCode + "without any url validation");
> +                } else {
> +                    needsNormalization = false;
> +                }
> +
> +            } catch (IOException e) {
> +                e.printStackTrace();
> +            } finally {
> +                if (needsNormalization) {
> +                    try {
> +                        //Normalize URL
> +                        if (JNLPRuntime.isDebug())
> +                            System.err.println("Attempting to normalize url of " + location.toString());
> +
> +                        location = normalizeUrl(location, JNLPRuntime.isDebug());
> +                        resource = Resource.getResource(location, version, updatePolicy);
> +                        downloaded = checkCache(resource, updatePolicy);
> +                    } catch (IOException e) {
> +                        e.printStackTrace();
> +                    }
> +                }
> +            }
> +        }
>
>           synchronized (resources) {
>               if (resources.contains(resource))
> @@ -206,12 +250,6 @@
>           }
>           downloadOptions.put(resource, options);
>
> -        // checkCache may take a while (loads properties file).  this
> -        // should really be synchronized on resources, but the worst
> -        // case should be that the resource will be updated once even
> -        // if unnecessary.
> -        downloaded = checkCache(resource, updatePolicy);
> -
>           synchronized (lock) {
>               if (!downloaded)
>                   if (prefetch && threads == 0) // existing threads do pre-fetch when queue empty
>