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

[Mikhailo Seledtsov]

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()

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     }


3. Style comment:
    The style for Java code is to use 4 spaces for tabulation/indent
(please see the attached discussion with David Holmes).
    

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/serviceability-dev/attachments/20130809/0f5a251a/attachment.html 
-------------- next part --------------
An embedded message was scrubbed...
From: David Holmes <david.holmes at oracle.com>
Subject: Re: RFR (S): 6726963: multi_allocate() call does not CHECK_NULL and
	causes crash in fastdebug bits
Date: Mon, 03 Jun 2013 22:28:05 +1000
Size: 3628
Url: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130809/0f5a251a/Code_Tabulation_Style__Email01.eml