[rfc][icedtea-web] Archive URL wrongly resolved - PR1204

[Andrew Azores]

On 06/14/2013 10:37 AM, Jiri Vanek wrote:
>
>> fix.patch
>>
>>
>> diff --git a/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java 
>> b/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
>> --- a/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
>> +++ b/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
>> @@ -38,6 +38,8 @@ exception statement from your version. *
>>   package net.sourceforge.jnlp.cache;
>>
>>   import java.net.MalformedURLException;
>> +import java.net.URI;
>> +import java.net.URISyntaxException;
>>   import java.net.URL;
>>   import java.util.LinkedList;
>>   import java.util.List;
>> @@ -152,23 +154,24 @@ public class ResourceUrlCreator {
>>        * @param resource the resource to get the url for
>>        */
>>       protected URL getVersionedUrlUsingQuery(Resource resource) {
>> -        String actualLocation = resource.getLocation().getProtocol() 
>> + "://"
>> -                + resource.getLocation().getHost();
>> -        if (resource.getLocation().getPort() != -1) {
>> -            actualLocation += ":" + resource.getLocation().getPort();
>> +        URL resourceUrl = resource.getLocation();
>> +        try {
>> +            String query = resourceUrl.getQuery();
>> +            if (query == null) query = "";
>> +            if (resource.requestVersion != null
>> +                    && resource.requestVersion.isVersionId()) {
>> +                if (!query.isEmpty()) {
>> +                    query += "&";
>> +                }
>> +                query += "version-id=" + resource.requestVersion;
>> +            }
>> +            URI uri = new URI(resourceUrl.getProtocol(), null, 
>> resourceUrl.getHost(), resourceUrl.getPort(), resourceUrl.getPath(), 
>> query, null);
>> +            return uri.toURL();
>> +        } catch (MalformedURLException e) {
>> +            return resourceUrl;
>> +        } catch (URISyntaxException e) {
>> +            return resourceUrl;
>>           }
>
>
> This is really deep change. It would need huge unittesting. (dont 
> forget to negative cases and versioning)
>
> The new approach looks correct, but I'm afraid it can affect caching 
> and versioned ajrs. You will have to prove that thay are still working 
> correctly.

Will do. Adam actually wrote this hunk (minus one line I added) so I 
will bother him if I need any help with it.

>> +
>> +            requestURI = UrlUtil.encode(baseURI, "UTF-8");
>> +            System.out.println("##### PluginAppletViewer 
>> requestPluginProxyInfo uri: " + uri.toString());
>> +            System.out.println("##### PluginAppletViewer 
>> requestPluginProxyInfo requestURI: " + requestURI);
>
> This debug should be removed, or moved to if debug, with some more 
> informative output (eg was, is, should be...) - I'm however for 
> removal your choice :)

Oops :) I meant to remove those.

> ANyway I think this is very wrong palce - especially just plugin 
> (proxy?!?!?) is affected -  This chanage should affect both javaws and 
> appelt. (and so the query change, but its place looks more correct)

Based on my debugging and tracing, and the log messages I found output 
by the plugin (which matched those in the original bug report), the "//" 
part of the bug was occurring only in the plugin and specifically had to 
do with retrieving proxy info. I can move this fix somewhere else, but I 
put it here because this to me seems like the first place where the 
malformed URL occurs.

> The reproducer is nice. To current one just two nits - there should be 
> @bug annotation on top of testcases with bug id (with PR) and links to 
> this thread in mail archive.
> Second nit is reusing of already existing jars. Last time you have 
> already wrote applet aproximatly same as your new 
> AbsolutePathAndQueryStringApplet. So the best for this patch will be 
> to get rid of this new, and reuse the same :)
> In jnlp and html files you can call the existing one.
>
> Also this test needs to be enhanced -
>  - you need to test it for both javaws and for appelts.
>  - you need to check how this behaves with versioned jars
>  - you need to test if the cache is still ok (query)

Working on this. I have the first point done and I'll start looking into 
the other two.

>
>> diff --git 
>> a/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/resources/AbsolutePathAndQueryStringApplet.html 
>> b/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/resources/AbsolutePathAndQueryStringApplet.html 
>>
>> new file mode 100644
>> --- /dev/null
>> +++ 
>> b/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/resources/AbsolutePathAndQueryStringApplet.html
>> @@ -0,0 +1,48 @@
>> < snip >
>> +    @Override
>> +    public void init() {
>> +        System.out.println(appletCloseString);
>> +    }
>
> init or start? :)

Ah, thank you. I thought it was init, and that's what I used last time 
as well. I've changed that now.



Thanks,

Andrew A