[PATCH] 6822627: NPE at ReferenceTypeImpl.constantPool

[serguei.spitsyn at oracle.com]

On 7/20/16 02:53, Egor Ushakov wrote:
>
> Yes, all correct, thanks!
>
Ok, thanks.
Serguei


>
> On 20.07.2016 12:34, serguei.spitsyn at oracle.com wrote:
>> Egor,
>>
>> If I understand correctly, you do not have an openJdk user ID and an 
>> author status.
>> Please, see the link:
>> http://openjdk.java.net/census
>>
>> So that, I'll commit your fix with the comment:
>>   Contributed-by: egor.ushakov at jetbrains.com
>>
>> and push it to the jdk9/hs.
>>
>> I hope, somebody will correct me if it is not right.
>> Please, let me know if it works for you.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 7/20/16 02:10, Egor Ushakov wrote:
>>>
>>> Serguei, thanks for the review!
>>>
>>> Please sponsor the fix, I do not know how to proceed.
>>>
>>> Thanks!
>>>
>>> Egor
>>>
>>>
>>> On 20.07.2016 10:36, serguei.spitsyn at oracle.com wrote:
>>>> Hi Egor,
>>>>
>>>> Thank you for providing the test!
>>>>
>>>> A couple of nits to the test:
>>>>
>>>>    56             if (!Arrays.equals(cpbytes, cpbytes2)) {
>>>>    57                 failure("Consequent constantPool results vary, first was : " + cpbytes + ", now: " + cpbytes2);
>>>>    58             };
>>>>    Last semicolon is not need.
>>>>    74         if (!testFailed) {
>>>>    75             println("ConstantPoolInfoGC: passed");
>>>>    76         } else {
>>>>    77             throw new Exception("ConstantPoolInfoGC: failed");
>>>>    78         }
>>>>   I'd suggest to rearrange the statement above to something like this:
>>>>    74         if (testFailed) {
>>>>    75             throw new Exception("ConstantPoolInfoGC: failed");
>>>>    76         }
>>>>    77         println("ConstantPoolInfoGC: passed");
>>>>
>>>>    But I leave it up to you.    No need to see another webrev.    I 
>>>> can sponsor your fix, if needed. Thanks, Serguei On 7/19/16 00:07, 
>>>> Egor Ushakov wrote:
>>>>>
>>>>> Hi Serguei,
>>>>>
>>>>> here's a new webrev with a test included: 
>>>>> http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.01/
>>>>>
>>>>> Egor
>>>>>
>>>>> On 15.07.2016 12:22, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Egor, The fix looks good modulo the synchronization issue. Do 
>>>>>> you have a jtreg unit test reproducing this failure mode? We have 
>>>>>> a rule to provide a test coverage for the fixes. Thanks, Serguei 
>>>>>> On 7/15/16 00:03, Egor Ushakov wrote:
>>>>>>>
>>>>>>> Thanks for your comments!
>>>>>>>
>>>>>>> I totally agree that the code there requires major refactoring. 
>>>>>>> In the fix I tried not to make it worse and fix the NPE.
>>>>>>>
>>>>>>> On 14.07.2016 20:06, Martin Buchholz wrote:
>>>>>>>> The lack of volatile or synchronization, plus the use of 
>>>>>>>> multiple mutable fields, suggests that a deeper fix is 
>>>>>>>> necessary to be truly correct.  For comparison, much of the 
>>>>>>>> similar code implementing reflection has been changed to use 
>>>>>>>> volatile.  But that's a big job, for the serviceability team.
>>>>>>>> On Thu, Jul 14, 2016 at 2:33 AM, Egor Ushakov 
>>>>>>>> <egor.ushakov at jetbrains.com 
>>>>>>>> <mailto:egor.ushakov at jetbrains.com>> wrote:
>>>>>>>>
>>>>>>>>     Hi, I'm a developer from IDEA debugger (we have company
>>>>>>>>     OCA). We've increased usage of
>>>>>>>>     ReferenceTypeImpl.constantPool and now facing JDK-6822627
>>>>>>>>     more often. Please review and sponsor the fix. The problem
>>>>>>>>     was that constantPoolBytesRef SoftReference value coud be
>>>>>>>>     GCed and there was no check for that. I've added it to the
>>>>>>>>     check inside getConstantPoolInfo to request the bytes again
>>>>>>>>     in this case. BUGURL:
>>>>>>>>     https://bugs.openjdk.java.net/browse/JDK-6822627 WEBREV:
>>>>>>>>     http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.00/
>>>>>>>>     <http://cr.openjdk.java.net/%7Eavu/JDK-6822627/webrev.00/>
>>>>>>>>     Thanks!-- Egor Ushakov Software Developer JetBrains
>>>>>>>>     http://www.jetbrains.com The Drive to Develop 
>>>>>>>>
>>>>>>> -- 
>>>>>>> Egor Ushakov
>>>>>>> Software Developer
>>>>>>> JetBrains
>>>>>>> http://www.jetbrains.com
>>>>>>> The Drive to Develop
>>>>> -- 
>>>>> Egor Ushakov
>>>>> Software Developer
>>>>> JetBrains
>>>>> http://www.jetbrains.com
>>>>> The Drive to Develop
>>> -- 
>>> Egor Ushakov
>>> Software Developer
>>> JetBrains
>>> http://www.jetbrains.com
>>> The Drive to Develop
> -- 
> Egor Ushakov
> Software Developer
> JetBrains
> http://www.jetbrains.com
> The Drive to Develop
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160720/f7de0344/attachment.html>