RFR 8009728: nsk/jvmti/AttachOnDemand/attach030 crashes on Win32

Coleen Phillimore coleen.phillimore at oracle.com
Fri Aug 9 13:44:51 PDT 2013


Hi Misha,

Thank you for reviewing the test.

On 8/9/2013 12:49 PM, Mikhailo Seledtsov wrote:
> Hi Coleen,
>
>  I have reviewed only the test portion of your change-set.
> It was a good learning exercise for me on class transformation.
> I have several comments:
>
> 1. Agent.java: redefine()
>     With your change, the block of code
>
> 	Agent transformer = new Agent();
> 	instrumentation.addTransformer(transformer, true);
>
> will be called 3 times, thus adding the transformer 3 times.
> Was it intended? If not, you could move this code to premain()

It wasn't intended.   I meant to move that to premain and have moved it 
and the test still passes.
>
> 2. WalkThroughInvoke.java: stackWalk()
>     If the AccessControlException() is always expected, and making sure it occurs is one of the test checks, I recommend throwing an exception
>     right after sm.checkMemberAccess(b, Member.DECLARED);
>
>     Such as:
>
>    try {
>    28       Class b = Object.class;
>    29       SecurityManager sm = new SecurityManager();
>    30       // walks the stack before it gets this exception.
>    31       // testing the stack walk with Method.invoke in the stack
>    32       sm.checkMemberAccess(b, Member.DECLARED);
> *   Misha>>  throw new Exception("Test failed, the expected AccessControlException did not occur");*
>    33     } catch (java.security.AccessControlException e) {
>    34       System.out.println("This exception is expected");
>    35     }
>

I don't think I want to do this change because if we change whether the 
checkMemberAccess() throws an exception, which seems unlikely, the test 
will start failing.   And so someone will have to fix the test for 
something that it's not trying to test.
> 3. Style comment:
>     The style for Java code is to use 4 spaces for tabulation/indent
> (please see the attached discussion with David Holmes).
>     

I changed the files to have 4 space indentation.

Thanks!
Coleen
>
> Misha
>
>
> On 8/8/2013 7:21 PM, Coleen Phillimore wrote:
>>
>> Thanks, Serguei!
>> Coleen
>>
>> On 08/08/2013 07:03 PM, serguei.spitsyn at oracle.com wrote:
>>> Coleen,
>>>
>>> The fix looks good, I do not see any issues.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 8/6/13 2:33 PM, Coleen Phillimore wrote:
>>>> Summary: ActiveMethodOopsCache was used to keep track of old 
>>>> versions of some methods that are cached in Universe but is buggy 
>>>> with permgen removal and not needed anymore
>>>>
>>>> There was a crash in this function that I couldn't reproduce. It 
>>>> was likely that the crash was for something else, but this is a 
>>>> lurking bug.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8009728/
>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8009728
>>>> local bug link https://jbs.oracle.com/bugs/browse/JDK-8009728
>>>>
>>>> Tested with vm.quick.testlist which includes redefine classes tests 
>>>> and jck lang and vm tests.
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130809/559e3641/attachment.html 


More information about the hotspot-runtime-dev mailing list