RFR (M) 8067662: "java.lang.NullPointerException: Method name is null" from StackTraceElement.<init>

[serguei.spitsyn at oracle.com]

On 3/19/15 11:11 AM, Daniel D. Daugherty wrote:
> On 3/12/15 11:27 PM, serguei.spitsyn at oracle.com wrote:
>> Please, review the jdk 9 fix for:
>>   https://bugs.openjdk.java.net/browse/JDK-8067662
>>
>>
>> Open hotspot webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8067662-JVMTI-trace.1/ 
>>
>
> Thumbs up. None of these comments are critical.

Thanks a lot for the review, Dan!

>
>
> src/share/vm/classfile/javaClasses.hpp
>     No comments.
>
> src/share/vm/classfile/javaClasses.cpp
>
>     line 1316:   return method != NULL && 
> (method->constants()->version() == version
>     line 1317:                         && version < MAX_VERSION);
>         Don't really need the parens and the indenting implies
>         a logical grouping that isn't really there. Perhaps:
>
>           return method != NULL && method->constants()->version() == 
> version
>             && version < MAX_VERSION;
>
>         Although I also wonder why the caller would pass a 'version'
>         value that is >= MAX_VERSION. Perhaps this instead:
>
>           assert(version < MAX_VERSION, "version is too big");
>           return method != NULL && method->constants()->version() == 
> version
>
>     line 1347:   typeArrayOop    _cprefs;
>         Perhaps add a comment: // needed to insulate method name 
> against redefinition
>
>     line 1486:   holder = holder->get_klass_version(version); // Use 
> specific ik version as a holder
>         Perhaps the following comment instead:
>
>         // Use specific ik version as a holder since the mirror might
>         // refer to version that is now obsolete.
>
>         I'm not sure "obsolete" is the right term here, but say
>         something about why the mirror version is not good enough.
>
>         Update: 'obsolete' is the right term now that I've re-read the
>             code review invite! Perhaps this is better:
>
>         // Use specific ik version as a holder since the mirror might
>         // refer to version that is now obsolete and no longer accessible
>         // via the previous versions list.
>
>     line 1854:   // Get method id, cpref, bci, version and mirror from 
> chunk
>         Perhaps:
>
>         // Get method id, bci, version, mirror and cpref from chunk
>
>         since that's the order you do the work...
>
>     line 1909:     holder = holder->get_klass_version(version); // Use 
> specific ik version as a holder
>         Similar comment as on line 1486.
>
> src/share/vm/oops/instanceKlass.hpp
>     No comments.
>
> src/share/vm/oops/instanceKlass.cpp
>     No comments.
>
>
>> Open webrev for unit test update:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8067662-JVMTI-trace-test.1/ 
>>
>
> test/java/lang/instrument/RedefineMethodInBacktrace.sh
>     No comments.
>
> test/java/lang/instrument/RedefineMethodInBacktraceApp.java
>     line 175:           System.exit(1);
>         A comment to explain why this test needs to exit on
>         failure would be good. When the work to get rid of
>         shell script driven tests gets here, it would be
>         useful for understanding why this test must be
>         standalone.

All of the above suggestions are good.
Will update before the push.
Thanks!

>
> test/java/lang/instrument/RedefineMethodInBacktraceTarget.java
>     No comments.
>
> test/java/lang/instrument/RedefineMethodInBacktraceTargetB.java
>     No comments.
>
> test/java/lang/instrument/RedefineMethodInBacktraceTargetB_2.java
>     No comments.
>
> test/java/lang/instrument/RedefineMethodInBacktraceTarget_2.java
>     No commetns.
>


Thanks!
Serguei

>
> Dan
>
>
>>
>>
>> Summary:
>>   An NPE is trown in a thread dumping via JMX when doing CPU 
>> profiling in NetBeans Profiler and VisualVM.
>>   The issue is that the methods and related klass versions are not 
>> kept alive if a class redefinition
>>   takes place between catching a Throwable and taking a thread dump.
>>   It can result in loosing an obsolete method, and so, the 
>> reconstruction of method name becomes impossible.
>>   In such a case, the null reference is returned instead of a valid 
>> method name.
>>
>>   The solution is to use current klass version and method orig_idnum 
>> instead of ordinary method idnum
>>   to find required method pointer. In the worst case when the related 
>> klass version is gone
>>   (was not included to or was removed from the previous_versions 
>> linked list), a saved method name
>>   CP index of the latest klass version can be used to restore the 
>> method name.
>>   The footprint extra overhead for this approach is u2 per stack frame.
>>
>>   The plan is also to backport the fix to 8u60.
>>
>> Testing:
>>   In progress: nsk redefine classes tests, JTREG java/lang/instrument
>>
>>
>> Thanks,
>> Serguei
>>
>>
>