code review request: Test case for JDK-7042126 HashMap.clone() memory leak

[David Buck]

Hi Alan!

Thank you for your help.

TL;DR version:

Strictly speaking (going only by the specifications), this could give us 
false positives, but I believe this is VERY unlikely to actually happen 
in real life.

Long version:

Yes, I gave this some thought myself. For example, on JRockit, if the 
object were in old space and System.gc() only did a young collection 
(the default behavior for JRockit), this test would result in a false 
positive. In fact, as the JVM is allowed by the specification to 
completely ignore explicit GC calls, we could never guarantee that we 
would the WeakReference would always get nulled out.

That said, in pactice this works very well for both HotSpot and JRockit. 
Every scenario I have tried it out on (with both JVMs) has provided the 
expected result every single time (i.e. failing when expected; never 
resulting in false positive otherwise). It seems that both of Oracle's 
JVMs as currently implemented are very unlikely to run into any issues 
here. Marking the test cases as "othervm" also helps to remove most edge 
case scenarios where you could still somehow imagine this failing. (For 
example, on a JRockit-like JVM, other tests running concurrently could 
trigger a gc in the middle of this test resulting in the HashMap and its 
contents being promoted to old space and the null reference not being 
cleared during the call to System.gc() as expected.)

One option would be to mark this as a manually-run test if we wanted to 
be extra cautious. What do you think?

 > Minor nit, should be WeakReference<Object> to avoid the raw type.

I will update the webrev once we have decided what (if anything) to do 
regarding the risk of false positives.

Cheers,
-Buck

On 2013/01/30 22:09, Alan Bateman wrote:
> On 29/01/2013 23:36, David Buck wrote:
>> Hi!
>>
>> This is a review request to add only the test case for the following
>> OracleJDK issue :
>>
>> [ 7042126 : (alt-rt) HashMap.clone implementation should be re-examined ]
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7042126
>>
>> * please note: I just marked the bug as "public" this morning, so
>> there will be a short delay before the above link is available.
>>
>> The issue (root cause) is not in OpenJDK (i.e. the problem was
>> OracleJDK specific), but the test case is valid for any Java SE
>> implementation so it should go into OpenJDK so we can prevent a
>> similar issue from ever happening in both releases moving forward. The
>> test case simply helps ensure that the contents of a HashMap are not
>> leaked when we clone the HashMap.
>>
>> webrev:
>> [ Code Review for jdk ]
>> http://cr.openjdk.java.net/~dbuck/7042126/webrev.00/
> How robust is this test? I'm concerned that it might fail intermittently
> if the System.gc doesn't immediately GC the now un-references entries in
> hm.
>
> Minor nit, should be WeakReference<Object> to avoid the raw type.
>
> -Alan.