[PATCH] 6822627: NPE at ReferenceTypeImpl.constantPool

[Egor Ushakov]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160720/e14ca3cf/attachment.html>