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

[coleen.phillimore at oracle.com]

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,
Coleen
>
> Thanks,
> David
> ----
>
>> Thanks,
>> Coleen