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

[Jiri Vanek]

On 06/14/2013 07:28 PM, Andrew Azores wrote:
> 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
ping?