Code review request, 7115524: sun.security.provider.certpath.ssl.SSLServerCertStore no longer works

[Xuelei Fan]

On 11/27/2011 7:07 PM, Weijun Wang wrote:
> 
> 
> On 11/27/2011 05:48 PM, Xuelei Fan wrote:
>>
>>
>> Sent from my iPad
>>
>> On Nov 27, 2011, at 5:01 PM, Weijun Wang<weijun.wang at oracle.com>  wrote:
>>
>>> Anyway, I find this a little unsafe. One can add a new SSL security
>>> provider at run time, and then run this method, and the new provider
>>> can be loaded.
>>>
>> I did not follow your ideas. The new provider is loaded, and what's
>> the worries then?
> 
> Typo, I meant "cannot".
> 
> 1. Run this method, the default SSL provider loaded
> 2. Add a new SSL security provider
> 3. Run this method again, still the old provider.
> 
Got it. But this class is very special in that the security provider may
be useless.

It is a little complicated. I will call you tomorrow to explain more. I
think we still have space to improve the stability and performance based
on the latest update.

Thanks,
Xuelei

> Max
> 
>>
>> Xuelei
>>
>>> In my opinion, this is more important than performance.
>>>
>>> Thanks
>>> Max
>>>
>>>
>>> On 11/27/2011 10:15 AM, Xuelei Fan wrote:
>>>>
>>>>
>>>> Sent from my iPad
>>>>
>>>> On Nov 27, 2011, at 9:49 AM, Weijun Wang<weijun.wang at oracle.com>  
>>>> wrote:
>>>>
>>>>>>> Is it possible that the exception is thrown sometimes but not
>>>>>>> always?
>>>>>
>>>>> If it always fails, there is no problem failing once at the
>>>>> beginning. Otherwise, if there is a chance it goes fine again after
>>>>> calling some codes, maybe it's better to try call it each time?
>>>>>
>>>> I don't think it the case will happen, otherwise there must be a
>>>> potential bug. Indeed, the exception  is unlikely to happen once
>>>> JSSE boot up.
>>>>
>>>> Xuelei
>>>>
>>>>
>>>>> Max
>>>>>
>>>>> On 11/27/2011 09:05 AM, Xuelei Fan wrote:
>>>>>>
>>>>>>
>>>>>> Sent from my iPad
>>>>>>
>>>>>> On Nov 27, 2011, at 12:23 AM, Weijun
>>>>>> Wang<weijun.wang at oracle.com>    wrote:
>>>>>>
>>>>>>> There is one thing I'm not sure about:
>>>>>>>
>>>>>>> In which case(s), the GeneralSecurityException will be thrown?
>>>>>> SSLContext initializatio
>>>>>>
>>>>>>> Is it possible that the exception is thrown sometimes but not
>>>>>>> always? Also, the old code has the exception as a clause of the
>>>>>>> CertStoreException. Can you also include it?
>>>>>>>
>>>>>> It is reorged in line 100 and 101, the cause is described as
>>>>>> unable to initialize the ssl context.
>>>>>>
>>>>>> Xuelei
>>>>>>
>>>>>>> Max
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Nov 26, 2011, at 11:25 PM, Xuelei
>>>>>>> Fan<xuelei.fan at oracle.com>    wrote:
>>>>>>>
>>>>>>>> new webrev: http://cr.openjdk.java.net/~xuelei/7115524/webrev.01/
>>>>>>>>
>>>>>>>> You're right on the synchronization issue. In the new webrev, I
>>>>>>>> also
>>>>>>>> remove the "synchronized" keyword on method. Instead, a
>>>>>>>> synchronization
>>>>>>>> on the static attribute is used.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>> On 11/26/2011 11:16 PM, Weijun Wang wrote:
>>>>>>>>>
>>>>>>>>> On Nov 26, 2011, at 10:58 PM, Xuelei
>>>>>>>>> Fan<xuelei.fan at oracle.com>    wrote:
>>>>>>>>>
>>>>>>>>>> I was wondering why we synchronized the getInstance() and
>>>>>>>>>> engineGetCertificates(). It seems that we only need to
>>>>>>>>>> synchronize the
>>>>>>>>>> serverChain variable.
>>>>>>>>>>
>>>>>>>>>> I will update the synchronization if no objection.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei
>>>>>>>>>>
>>>>>>>>>> On 11/26/2011 10:42 PM, Xuelei Fan wrote:
>>>>>>>>>>> On 11/26/2011 9:59 PM, Weijun Wang wrote:
>>>>>>>>>>>> Hi Xuelei
>>>>>>>>>>>>
>>>>>>>>>>>> Why move the local variables to static fields?
>>>>>>>>>>>>
>>>>>>>>>>> In order to improve the performance. SSLContext.init() is
>>>>>>>>>>> expensive.
>>>>>>>>>>>
>>>>>>>>>>>> The engineGetCertificates method is only synchronized on
>>>>>>>>>>>> this but not the whole class, so there is a chance that the
>>>>>>>>>>>> method is called by multiple threads at the same time. This
>>>>>>>>>>>> means if you only use one static GetChainTrustManager field,
>>>>>>>>>>>> its serverChain field can be modified by different threads.
>>>>>>>>>>>>
>>>>>>>>>>> We have synchronized the engineGetCertificates(), and this
>>>>>>>>>>> method is the
>>>>>>>>>>> only method to access the GetChainTrustManager object. As the
>>>>>>>>>>> attribute
>>>>>>>>>>> is declared as "final static", so it will be initialized once
>>>>>>>>>>> even in
>>>>>>>>>>> multiple threads.
>>>>>>>>>
>>>>>>>>> But this method is only synchronized on the instance, not the
>>>>>>>>> class. You can create two SSLServerCertStore objects can call
>>>>>>>>> this method at the same time.
>>>>>>>>>
>>>>>>>>> Max
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> That's, the access to serverChain variable has been
>>>>>>>>>>> synchronized by
>>>>>>>>>>> synchronizing the engineGetCertificates method.
>>>>>>>>>>>
>>>>>>>>>>> Xuelei
>>>>>>>>>>>
>>>>>>>>>>>> -Max
>>>>>>>>>>>>
>>>>>>>>>>>> On Nov 26, 2011, at 8:43 PM, Xuelei Fan wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~xuelei/7115524/webrev.00/
>>>>>>>>>>>>>
>>>>>>>>>>>>> No new regression test, the current test,
>>>>>>>>>>>>> test/sun/security/tools/keytool/printssl.sh, has already
>>>>>>>>>>>>> been able to
>>>>>>>>>>>>> test the update.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Xuelei
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>