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

[David Holmes]

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

Thanks,
David
-----

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