RFR: 8085903: New fix for memory leak in ProtectionDomain cache

[Sean Mullan]

New (simpler) fix using Map.put():

http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.02/

This should be ok, as the JDK 8 code and the JDK 9 code prior to the fix 
for 8055753 [1] was using Map.put instead of putIfAbsent.

Thanks,
Sean

[1] https://bugs.openjdk.java.net/browse/JDK-8055753

On 01/12/2016 11:02 AM, Sean Mullan wrote:
> On 01/12/2016 10:26 AM, Xuelei Fan wrote:
>> I think hashMap.put() may be similar or more effective than
>> hashMap.putIfAbsent().
>
> Ok, I think I see what you are trying to say.
>
> The common case is that the putIfAbsent method is going to succeed (i.e.
> the entry will be absent). It may not succeed in cases where there are
> multiple threads simultaneously computing the initial permissions for
> the same PD or the value is garbage collected due to memory demand.
>
> So, I think the question boils down to whether put is faster than
> putIfAbsent in the common case (when there is not an entry). Probably
> yes, since it doesn't have to check if there is an entry already.
>
>> Is there a case that the key/weakPd is the same,
>> but the value/pc is different?
>
> I believe the pc should always be the same. If you change the policy
> file(s), then you have to explicitly refresh it (using Policy.refresh())
> which should cause a new cache to be created.
>
> Let me do some more testing around this, and see if I can check
> performance and see what is better.
>
> Thanks,
> Sean
>
>>
>> Xuelei
>>
>> On 1/12/2016 11:01 PM, Sean Mullan wrote:
>>> Hi Ivan,
>>>
>>> On 01/12/2016 09:38 AM, Ivan Gerasimov wrote:
>>>> Hi Sean!
>>>>
>>>> On 12.01.2016 16:26, Sean Mullan wrote:
>>>>> I received a private comment that there may be cases where the map's
>>>>> value is reclaimed by the garbage collector, but the key still exists.
>>>>> If that ProtectionDomain is subsequently used for permission checks, a
>>>>> Map.putIfAbsent method will not replace the value.
>>>>>
>>>>> So, I have added an additional check for this case in the PDCache.put
>>>>> method, and it instead uses the Map.replace method. Updated webrev:
>>>>>
>>>>
>>>> Wouldn't it be a little bit more efficient to use merge() here?
>>>>
>>>> Something like:
>>>> pdMap.merge(weakPd, v, (pv, nv) -> pv.get() != null ? pv : nv);
>>>
>>> Yes - good catch!
>>>
>>> Will update and send out new webrev after a round of testing ...
>>>
>>> Thanks,
>>> Sean
>>>
>>>>
>>>> Sincerely yours,
>>>> Ivan
>>>>
>>>>> http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.01/
>>>>>
>>>>> --Sean
>>>>>
>>>>> On 01/08/2016 03:06 PM, Sean Mullan wrote:
>>>>>> Please review this fix for a memory leak in the ProtectionDomain
>>>>>> cache
>>>>>> (which maps each ProtectionDomain to their granted
>>>>>> PermissionCollection). The memory leak only occurs if custom
>>>>>> permissions
>>>>>> are used in a policy file.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.00/
>>>>>>
>>>>>> Custom permissions cause a chain of strong references that link
>>>>>> back to
>>>>>> the ProtectionDomain key used in the map, preventing it from being
>>>>>> removed (and also preventing the custom permission's URLClassLoader
>>>>>> from
>>>>>> being garbage collected). This fix wraps the PermissionCollection
>>>>>> in a
>>>>>> SoftReference which allows the objects to be garbage collected in
>>>>>> response to memory demand.
>>>>>>
>>>>>> Thanks,
>>>>>> Sean
>>>>>
>>>>
>>