Review Request: 8006506: Add test for redefining methods in backtraces to java/lang/instrument tests

[David Holmes]

Hi Stefan,

This part of the test:

   86     private static void doClassUnloading() {
   87         // This will clean out old, unused redefined methods.
   88         System.gc();
   89     }

seems to make assumptions about System.gc() and class unloading. Are we 
relying on knowledge of hotspot internals here?

I don't have a good suggestion to avoid this. This topic came up on 
another thread today regarding weak references. For that case we can 
typically add a loop and sleep until we see the reference clear, but 
here ... how can you check that class unloading occurred? Actually is 
that really what is happening - I don't see how any class unloading will 
occur here. I can imagine that GC might cleanup unreferenced methods.

David

On 1/02/2013 9:27 PM, Stefan Karlsson wrote:
> On 2013-02-01 12:17, Stefan Karlsson wrote:
>> On 2013-02-01 12:11, serguei.spitsyn at oracle.com wrote:
>>> On 2/1/13 1:57 AM, Stefan Karlsson wrote:
>>>> On 2013-02-01 10:22, serguei.spitsyn at oracle.com wrote:
>>>>> Nice test!
>>>>> It looks good.
>>>> Thanks for reviewing!
>>>>>
>>>>> As the original bug and the test are non-trivial, it'd make sense
>>>>> to add a comment to
>>>>> the class RedefineMethodInBacktraceApp and explain a little bit
>>>>> what the test is doing,
>>>>> and what behavior is expected.
>>>> http://cr.openjdk.java.net/~stefank/8006506/webrev.04/
>>>>
>>>> Tell me if you think this is good enough.
>>>
>>> It is good.
>>>
>>> Nit: it'd be enough if it is more specific. :)
>>> This method is a key point:
>>>    90     private static void touchRedefinedMethodInBacktrace(Throwable throwable) {
>>>    91         throwable.getStackTrace();
>>>    92     }
>>> Is it true that the test expects the getStackTrace() does not crash nor
>>> throw an exception which would happen if the old/obsolete method is
>>> gc'ed?
>>
>> I see. I'll add a comment that we shouldn't crash.
>
> http://cr.openjdk.java.net/~stefank/8006506/webrev.05/
>
> StefanK
>
>>
>> Thanks,
>> StefanK
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>>
>>>> thanks,
>>>> StefanK
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Sergueri
>>>>>
>>>>>
>>>>> On 2/1/13 12:13 AM, Stefan Karlsson wrote:
>>>>>> http://cr.openjdk.java.net/~stefank/8006506/webrev.03/
>>>>>>
>>>>>> 1) Reverted the ProblemList change, since the fix has already
>>>>>> propagaged to jdk8/tl
>>>>>> 2) Renamed do_redefine -> doRedefine
>>>>>> 3) Updated the .sh file with the bug number of the original CR
>>>>>> instead of the test CR.
>>>>>>
>>>>>> thanks,
>>>>>> StefanK
>>>>>>
>>>>>> On 2013-01-22 14:11, Stefan Karlsson wrote:
>>>>>>> http://cr.openjdk.java.net/~stefank/8006506/webrev.00/
>>>>>>>
>>>>>>> This test provokes the JVM crash described in bug: JDK-7174978.
>>>>>>>
>>>>>>> I intend to push this to:
>>>>>>> http://hg.openjdk.java.net/jdk8/tl/jdk
>>>>>>>
>>>>>>> thanks,
>>>>>>> StefanK
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>