[rfc][icedtea-web] Remove use of clear cache in PackGZip Tests

[Jie Kang]

----- Original Message -----
> On 11/21/2014 09:41 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> On 11/21/2014 09:31 PM, Jie Kang wrote:
> >>> Hello,
> >>>
> >>>
> >>> I just noticed that my push for the PackGZip tests included a mistake
> >>> where
> >>> the test would clear cache in @Setup.
> >>>
> >>> This patch removes the method call.
> >>>
> >>> Okay to push?
> >>>
> >>>
> >>> Regards,
> >>>
> >> Well, why to not clean in @before? It gave sense to me to clear....
> >
> > Main reason for me is that I don't remember any other reproducer that
> > deletes the whole cache before running. I don't think it's dangerous.
> 
> Yes, but no other reproducer is verifying cache content. See eg clear cache
> reproducer :) It is clearing and filing it all around manytimes...
> >
> > Note that 'CacheUtil.clearCache' deletes the entire cache. So when you run
> > all the reproducers and it hits the PackGZip reproducer, the cache will be
> > reset to empty...
> 
> And it do not hurt.  Better to have cache clean, then have it unpredictable.
> 
> I'm +1 for keeping those lines in.

Okay then. I thought a un-clean cache would be more useful so we can possibly find problems in cache hahah... I don't really have an issue with keeping it in.

> 
> 
> Btw, are you able to reproduce the issue aph hitted?  I'm not....

It is a very specific situation that occurred to cause the StringOutOfBoundsException, with 14to15 transfer messing up somewhere, invalid content in 'recently_used' file, and you could say 'bad luck' :\

I believe could create a 'reproducer' but it would involve creating dummy files on the file system.


    private static String pathToURLPath(String path) {
        int len = cacheDir.length();
        int index = path.indexOf(File.separatorChar, len + 1);
        return path.substring(index);
    }


   The SOOB-Exception was because
     int index = path.indexOf(File.separatorChar, len + 1)
               = -1
      
     .indexOf(..) only returns -1 if File.separatorChar is not found in 'path' from index 'len+1' to the end of the string.
          
            and len+1 is from cacheDir.length()


>From looking at how 'cacheDir' is constructed, I would say most probable that String: 'path' was not valid.

This 'path' comes from the LRU 'recently_used' file.

So to reproduce, we need to create a situation where LRU 'recently_used' is corrupted. The simplest way is to write a bad entry into the file.

Something like:

1416601158426,1=/home/jkang/.cache/icedtea-web/cache/thiswillprobablyfail(i-hope)



I think for a long-term fix, we should look at improving the cache system to not blindly assume LRU file is correct. And of course, figure out what to do when it is corrupted. ie. redownload into non-corrupt location, remove corrupt entries, etc. etc.



Also::

CacheLRUWrapper::checkData()
  This function is supposed to check LRU for corrupt entries, but the 'path' checking portion only has code:
 
  // 2. check path format - does the path look correct?
            if (path != null) {
                if (path.indexOf(cacheDir) < 0) {
                    it.remove();
                    modified = true;
                }
            } else {
                it.remove();
                modified = true;
            }
 

  This only checks that there is a path and that it contains the cacheDir at the front. This should be improved. (file should probably exist, be a valid directory format... etc etc)



I hope this helps!

If you'd like, I can try to write a reproducer on Monday.

Regards,


> 
> Thanx, J.
> >
> >
> > Regards,
> >
> >
> >>
> >> J.
> >>
> >
> 
> 

-- 

Jie Kang