RFR: JDK-8134373: explore potential uses of convenience factories within the JDK

[Stuart Marks]

On 9/30/16 6:57 AM, Jonathan Bluett-Duncan wrote:
> 1) It actually didn't occur to me that there was a potential TOCTOU problem in 
> ModuleFinder.compose, despite the fact that I found a potential problem in 
> FileTreeIterator - it completely missed me, so thank you for finding it! I'll 
> see if I can put a similar comment there to what I wrote in FileTreeIterator.
OK. A simple comment such as

     // make a copy since it's captured by the ModuleFinder instance belowfinal List<ModuleFinder> finderList = List.of(finders);

should be sufficient.
> 2) ... I decided just now to do a search on Grepcode for usages of 
> CookieManager.get 
> <http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@8u40-b25@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29&k=u> and 
> CookieHandler.get 
> <http://grepcode.com/search/usages?id=repository.grepcode.com$java$root@jdk$openjdk@8u40-b25@java$net@CookieHandler@get%28java.net.URI,java.util.Map%29&start=0&r=&type=method&k=u>, 
> and curiously it looks like they're only used in sun classes in the JDK. So 
> this change seems safe to me, unless I've missed something?
While we're looking at CookieManager.java, please fix this typo 
"NetscapeCookieSotre".

   84  *     by a more sophisticated CookieStore implementation, e.g. a NetscapeCookieSotre.

So, grepcode is kind of hard to work with. I've found that when doing "Find 
Usages" for an API, the results are relative to the JDK version you're looking 
at. Apparently at the time grepcode's indexes were run, most of those other 
libraries weren't building against any version of JDK 8. But if you start the 
search at (say) JDK6 b27, and then go to java.net.CookieHandler.get() and do 
"Find usages", you get a bunch of hits:

http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@6-b27@java%24net@CookieHandler@get%28java.net.URI%2Cjava.util.Map%29&k=u

Same with 6-b27 CookieManager.get():

http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@6-b27@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29&k=u

7u40-b43 CookieHandler.get():

http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24net@CookieHandler@get%28java.net.URI%2Cjava.util.Map%29&k=u

7u40-b43 CookieManager.get():

http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29&k=u

I don't know a way to search for the usage of an API across all versions of the 
JDK. It's moderately painful to have to do the same searches for several 
different JDK versions.

Anyway, I spent some time looking through usage hits from the links above. It's 
hard to know how many there are; you just keep hitting "Next" until there aren't 
any more. (Is there a way to get statistics instead of just page after page of 
matches? I don't know.) There are lots of duplicates -- different versions of 
the same library -- so you can skip past those pretty quickly.

After a while some patterns begin to emerge. For this API, the Map that's 
returned is almost always iterated over looking for "Cookie" and "Cookie2" 
entries, or being probed directly for those entries, and is then thrown away. In 
one case it was stored in a private field of a non-serializable class, but the 
only usage of that field was to iterate over it in a manner similar to other cases.

I encourage you to look through these usages as well.

In any case, it's fairly clear to me that the most common use of the returned 
Map is to read stuff out of it immediately. This isn't definitive, obviously, 
but it does look like this is the most common usage pattern. Based on this I 
think the serialization difference won't be a problem, and that it's OK to 
proceed with this change.
> 3) In my local copy of jdk9, I've removed the TOCTOU comment in 
> FileTreeIterator and changed List.of back to Arrays.asList, as your 
> explanation regarding FileTreeWalker has convinced me that TOCTOU is not a 
> real problem here....
OK
> 4) The 'resolverFields' problem/comment was regarding DateTimeFormatter (see 
> this part of latest webrev 
> <http://cr.openjdk.java.net/%7Ereinhapa/reviews/8134373/webrev.01/src/java.base/share/classes/java/time/format/DateTimeFormatter.java.cdiff.html>), 
> where I realised I couldn't use Set.of instead of 
> Collections.unmodifiableSet(new HashSet<>(Arrays.asList(*))), because I 
> noticed that one of the java.time tests was testing whether 
> DateTimeFormatter.withResolverFields(TemporalField...) could accept null 
> parameters, which made using Set.of impossible since it's null-hostile (as in 
> it throws NullPointerException upon being constructed with null element(s)).
Hm, yes, it's odd that there's a test for an array containing a null, in 
addition to an empty array and a null array. See:

jdk/test/java/time/tck/java/time/format/TCKDateTimeFormatter.java
test_resolverFields_listOfOneNull()

I'm not entirely sure what's intended here. In any case, let's wait until we 
hear from Mr. Colebourne.

I looked at the rest of the files and the only questions I have are other places 
in the java.time files.

5) For the changes in the Chronology classes, the era() method now returns an 
immutable array where it didn't before. (The List returned by Arrays.asList() 
can have individual elements modified but its size can't be changed.) The spec 
for eras() says "may be immutable" so presumably this is OK. But note, since the 
returned List is now immutable, a new instance needn't be created each time. I'm 
not sure whether it's worth keeping around a cached copy in case someone calls 
eras() again though.

It would be good if we could hear from Stephen on this one as well.

s'marks