RFR (S) 8218755: [REDO] Symbol leak in prepend_host_package_name

[coleen.phillimore at oracle.com]

On 2/12/19 9:06 PM, David Holmes wrote:
> On 13/02/2019 11:54 am, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 2/12/19 7:39 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 13/02/2019 1:13 am, coleen.phillimore at oracle.com wrote:
>>>> Summary: fix Symbol refcounting again, add comment and a test.
>>>>
>>>> open webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/2019/8218755.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8218755
>>>>
>>>> I hope this is better.  Ran hs-tier1-6 and a new test!
>>>
>>> You got rid of the NULL check in update_class_name() but the one 
>>> remaining call-site has:
>>>
>>>   // Update _class_name which could be null previously
>>>   // to reflect the name in the constant pool
>>>   update_class_name(class_name_in_cp);
>>>
>>> implying we still need it - even though we assert _class_name is not 
>>> NULL earlier (perhaps shouldn't be NULL but we need to be 
>>> conservative?).
>>>
>>
>> The comment is wrong.  We initialize _class_name with :
>>
>>    _class_name = name != NULL ? name : vmSymbols::unknown_class_name();
>>
>> So it's never null.
>>
>>> I'm inclined to just inline update_class_name along with the 
>>> comments - they can be shortened and are no worse than the block you 
>>> added elsewhere:
>>
>> I really dislike the exceptionally long functions that we have in 
>> Hotspot, especially parse_stream, and would rather have this call out 
>> to do this special refcount magic.   I tried but I couldn't add more 
>> lines of code to that function.
>>
>>>
>>> !     // The new class name is created with a refcount of one. When 
>>> installed into the InstanceKlass,
>>> !     // it'll be two and when the ClassFileParser destructor runs, 
>>> it'll go back to one and get deleted
>>> !     // when the class is unloaded.
>>>
>>
>> That's in an already relatively short function.
>>> ---
>>>
>>> test/hotspot/jtreg/runtime/defineAnonClass/AnonSymbolLeak.java
>>>
>>> 24 // This is copied from these DefineAnon
>>>
>>> delete "these"
>>
>> Fixed.
>>
>>>
>>> Aside: singleoneInstanceField is an insanely long (and strange) name 
>>> for a local variable that gets written out three times!
>>>
>>> Aside 2: these tests should be migrated off sun.misc.Unsafe and use 
>>> jdk.internal.misc.Unsafe
>>>
>>>   81         try {
>>>   82             p1cls.getMethod("test").invoke(null);
>>>   83         } catch (Throwable ex) {
>>>   84             ex.printStackTrace();
>>>   85             fail = ex;  // throw this to make test fail, since 
>>> subtest failed
>>>   86         }
>>>
>>> This code can never fail. If the exception occurs we store it in the 
>>> local variable fail but don't do anything with it. ??
>>
>> More copy paste.
>>
>> I fixed the Unsafe code in the tests in that directory.
>>
>> http://cr.openjdk.java.net/~coleenp/2019/8218755.02/webrev/index.html
>
> Thanks for the updates! Changes mostly look good. In AnonSymbolLeak
>
>   71         } catch (Throwable ex) {
>   72             ex.printStackTrace();
>   73             throw new RuntimeException("package injections should 
> not have failed");
>   74         }
>
> given everything is delcared to throw Throwable you don't need to 
> catch anything here. If you really want to convert to a 
> RuntimeException then you don't need to printStackTrace but should 
> pass ex as the cause of the RuntimeException.
>
> Similarly in TestAnonSymbolLeak:
>
>   57                 t.printStackTrace();
>   58                 throw new RuntimeException("Exception happened");
>
> just:
>
>    throw new RuntimeException("Exception happened", t);

Okay, I removed the try blocks.

Thank you for reviewing this!
Coleen

>
> Thanks,
> David
> -----
>
>> Thanks,
>> Coleen
>>>
>>> Thanks,
>>> David
>>> ----
>>>
>>>> Thanks,
>>>> Coleen
>>