Re: JDK 8 Review Request for 6854712 (JEP 124), 6637288 and 7126011

[David Schlosnagle]

On Tue, May 22, 2012 at 12:09 PM, Sean Mullan <sean.mul...@oracle.com> wrote:
> Can you please review my changes for 6854712 (JEP 124). It also includes
> fixes for a couple of other CRs: 6637288 and 7126011. Other reviewers are
> welcome too.
>
> The webrev:
> http://cr.openjdk.java.net/~mullan/webrevs/6854712_6637288_7126011/webrev.00/

Sean,

I had a few random questions and minor comments on this change set.

Have you considered any changes to the APIs to add a public boolean
isRevocationCheckingSupported() like method that indicates whether use of
getRevocationChecker() is supported so that a caller doesn't need to catch the
UnsupportedOperationException?

src/share/classes/sun/security/provider/certpath/AdjacencyList.java
src/share/classes/sun/security/provider/certpath/ForwardState.java
src/share/classes/sun/security/provider/certpath/ReverseState.java
src/share/classes/sun/security/provider/certpath/Vertex.java
src/share/classes/sun/security/provider/certpath/X509CertificatePair.java
    The toString() methods could benefit from converting string concatenation
    to StringBuilder.append to avoid unnecessary additional StringBuilders
    (-XX:+OptimizeStringConcat might take care of these as well).

src/share/classes/sun/security/provider/certpath/ConstraintsChecker.java
src/share/classes/sun/security/provider/certpath/KeyChecker.java
src/share/classes/sun/security/provider/certpath/PolicyChecker.java
    Are these classes intended to be thread safe? If so, then
    getSupportedExtensions() has a race on supportedExts instance field and
    should probably be volatile with double checked lock or thread safe holder
    idiom. It also seems like the mutable supportedExts could potentially
    escape under the race, so it might be worth building the supported
    extensions in a local set and then assigning to this.supportedExts. If they
    are not expected to be thread safe, you're probably fine although you might
    still want to avoid escaping mutable state and add JavaDoc similar to what
    you put in the PKIXRevocationChecker class docs.

src/share/classes/sun/security/provider/certpath/OCSPRequest.java
    Is it worth defining/using a constant for the OCSP OID
    "1.3.6.1.5.5.7.48.1.2" and using it on line 120 of encodeBytes()?
OCSPResponse has a
    private constant ObjectIdentifier OCSP_NONCE_EXTENSION_OID that could
    possibly be reused within the package:
 136     private static final ObjectIdentifier OCSP_NONCE_EXTENSION_OID =
 137         ObjectIdentifier.newInternal(new int[] { 1, 3, 6, 1, 5,
5, 7, 48, 1, 2});

src/share/classes/sun/security/provider/certpath/PolicyNodeImpl.java
    Stylistic nit - toString could use for-each:
 183         for (PolicyNodeImpl node : getChildren()) {
 184             buffer.append(node);
 185         }

src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java
    In depthFirstSearchForward, the finalCert logic could be optimized to avoid
    traversing the entire linked list, possibly twice:
 559                     if (cpList.isEmpty()) {
 560                         finalCert = builder.trustAnchor.getTrustedCert();
 561                     } else {
 562                         finalCert = cpList.getLast();
 563                     }

Thanks,
Dave