[rfc][icedtea-web] do not dye if jnlp's icons points to nothing (eg 404)

[Jiri Vanek]

On 02/02/2015 03:26 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>> On 02/02/2015 01:57 PM, Jiri Vanek wrote:
>>> This patch is fixing state, when icon is expected to be downlaoded, but its
>>> location do not exists.
>>> Then when this code expect it to be found, NPE is thrown.
>>>
>>> Hmm.. Looking to this after break,
>>>
>>> - location = CacheUtil.getCachedResource(uiconLocation, null,
>>> UpdatePolicy.SESSION)
>>> -                     .toString();
>>> + locationURL = CacheUtil.getCachedResource(uiconLocation, null,
>>> UpdatePolicy.SESSION);
>>> + if (locationURL == null) {return null;}
>>> + location = locationURL.toString();
>>>
>>> Is probably much more suitable (need to test it)
>>>
>>>
>>> J.
>> Or maybe this third attempt?
>>
>> Note - this is not refactoring, it changes behaviou, but I have not found
>> more usages of
>> getCachedResource ...
>
> Hello,
>
> Ignore my previous e-mail;
>
> This looks much better!
>
>
>
> +    public static File  getCachedResourceFile(URL location, Version version, UpdatePolicy policy) {
>
> Can we add comments to this function?

did
>
>
>       public static URL getCachedResource(URL location, Version version, UpdatePolicy policy) {
>
> Since we have getCachedResourceFile, maybe rename this to getCachedResourceURL?
>
>
> +            //this throws npe, if url (specified in jnl) points to 404
>
> s/jnl/jnlp

fixed
>
>
> +                    cantCache();
>                   }
>               } catch (IOException ex) {
>                   //favicon 404 or similar
>
> With these changes, is this IOException catch still relevant? I am wondering, we choose to throw the NonFileProtocolException up to the caller, and the function says:

I believe yes.
>
>      private void cacheIcon() throws IOException, NonFileProtocolException {
>
> How come we don't throw this IOException up too?
>
> So in:
>      public void createDesktopShortcuts(AccessWarningPaneComplexReturn.ShortcutResult menu, AccessWarningPaneComplexReturn.ShortcutResult desktop, boolean isSigned) {
>                  try {
>                      cacheIcon();
>                  } catch (NonFileProtocolException ex) {
>                      OutputController.getLogger().log(ex);
>                      //default icon will be used later
>                  }
>
> Maybe we should catch the IOException here as well? I am not too familiar with this code so any explanations would be very helpful!
>

It seems to me like two different cases. Failure in first is leading to seccond attempt. The last 
throw if ioexception is from copy. Thats very rare exception and our reaction should be different.
>

  Regards,
   And tY for check!

  J.



-------------- next part --------------
diff -r b4ac69f5297e netx/net/sourceforge/jnlp/cache/CacheUtil.java
--- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java	Fri Feb 06 15:14:32 2015 +0100
+++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java	Mon Feb 09 15:10:10 2015 +0100
@@ -70,19 +70,39 @@
      *
      * @param location location of the resource
      * @param version the version, or {@code null}
+     * @param policy how to handle update
      * @return either the location in the cache or the original location
      */
-    public static URL getCachedResource(URL location, Version version, UpdatePolicy policy) {
-        ResourceTracker rt = new ResourceTracker();
-        rt.addResource(location, version, null, policy);
+    public static URL getCachedResourceURL(URL location, Version version, UpdatePolicy policy) {
         try {
-            File f = rt.getCacheFile(location);
+            File f = getCachedResourceFile(location, version, policy);
+            //url was ponting to nowhere eg 404
+            if (f == null){
+                //originally  f.toUrl was throwing NPE
+                return null;
+                //returning null seems to be better
+                        
+            }
             // TODO: Should be toURI().toURL()
             return f.toURL();
         } catch (MalformedURLException ex) {
             return location;
         }
     }
+    
+    /**
+     * This is returning File object of cached resource originally from URL
+     * @param location original location of blob
+     * @param version
+     * @param policy
+     * @return location in ITW cache on filesystem 
+     */
+    public static File  getCachedResourceFile(URL location, Version version, UpdatePolicy policy) {
+        ResourceTracker rt = new ResourceTracker();
+        rt.addResource(location, version, null, policy);
+        File f = rt.getCacheFile(location);
+        return f;
+    }
 
     /**
      * Returns the Permission object necessary to access the
diff -r b4ac69f5297e netx/net/sourceforge/jnlp/util/XDesktopEntry.java
--- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java	Fri Feb 06 15:14:32 2015 +0100
+++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java	Mon Feb 09 15:10:10 2015 +0100
@@ -431,10 +431,14 @@
 
         String location = null;
         if (uiconLocation != null) {
-            location = CacheUtil.getCachedResource(uiconLocation, null, UpdatePolicy.SESSION)
-                    .toString();
+            //this throws npe, if url (specified in jnl) points to 404
+            URL urlLocation = CacheUtil.getCachedResourceURL(uiconLocation, null, UpdatePolicy.SESSION);
+            if (urlLocation == null) {
+                cantCache();
+            }
+            location = urlLocation.toString();
             if (!location.startsWith("file:")) {
-                throw new NonFileProtocolException("Unable to cache icon");
+                cantCache();
             }
         } else {
             //try favicon.ico
@@ -445,10 +449,14 @@
                         file.getCodeBase().getPort(),
                         "/" + FAVICON);
                 JNLPFile.openURL(favico, null, UpdatePolicy.ALWAYS);
-                location = CacheUtil.getCachedResource(favico, null, UpdatePolicy.SESSION)
-                        .toString();
+                //this MAY throw npe, if url (specified in jnlp) points to 404
+                URL urlLocation = CacheUtil.getCachedResourceURL(favico, null, UpdatePolicy.SESSION);
+                if (urlLocation == null) {
+                    cantCache();
+                }
+                location = urlLocation.toString();
                 if (!location.startsWith("file:")) {
-                    throw new NonFileProtocolException("Unable to cache icon");
+                    cantCache();
                 }
             } catch (IOException ex) {
                 //favicon 404 or similar
@@ -474,6 +482,10 @@
         }
     }
 
+    private void cantCache() throws NonFileProtocolException {
+        throw new NonFileProtocolException("Unable to cache icon");
+    }
+
     private String getDesktopIconName() {
         return sanitize(file.createJnlpTitle());
     }