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

[Daniel D. Daugherty]

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.


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.

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.


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