RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with NullPointerException

[Chris Plummer]

Hello,

Please review the following:

http://cr.openjdk.java.net/~cjplummer/8244203/webrev.00/index.html
https://bugs.openjdk.java.net/browse/JDK-8244203

The root of the problem is that SA is trying iterate over all loaded 
classes by using ClassLoaderDataGraph, but it possible that a class that 
ClassLoaderDataGraph knows about can be in a state that makes it 
findable by SA, but not yet initialized far enough to make is usable by SA.

The first issue I tracked down in this area was a case where an 
InstanceKlass did not yet have its java_mirror. This resulted in the NPE 
you see reported in the bug, because there is some SA code that assumes 
all InstanceKlass's have a java_mirror. I fixed this by not having 
ClassLoaderData.classesDo() return any InstanceKlass that was not yet 
initialized to the point of being considered "loaded". That fixed this 
particular problem, but there was another still lurking that was similar..

The second issue was that event attempting to iterate over the set of 
loaded classes could cause an NPE, so you couldn't even get to the point 
of being able to reject an InstanceKlass if it was not yet int he 
"loaded" state.  ClassLoaderData.classesDo() needs to get the first 
Klass in its list:

     public void classesDo(ClassLoaderDataGraph.ClassVisitor v) {
         for (Klass l = getKlasses(); l != null; l = l.getNextLinkKlass()) {
             v.visit(l);
         }
     }

Since the first Klass is the one most recently added, its also subject 
to sometimes not yet being fully loaded. Calling getKlasses() will 
instantiate (wrap) the first Klass in the list, which in this case is a 
partially loaded InstanceKlass which was so early in its initialization 
that InstanceKlass::_fields had not yet been setup. Creating an 
InstanceKlass (on the SA side) requires _fields to be set, otherwise it 
gets an NPE. I fixed this by allowing the InstanceKlass to still be 
created if not yet "loaded", but skipped any further initialization of 
the InstanceKlass that would rely on _fields. This allows the 
InstanceKlass to still be used by ClassLoaderData.classesDo() to iterate 
over the classes. However, I also wanted to make sure this uninitialized 
InstanceKlass wasn't leaked out to SA beyond its use by classesDo(). It 
looks like the only other way this is possible is with 
ClassLoaderData.find() and Dictionary.allEntriesDo(), so I put an 
InstanceKlass.isLoaded() checks there also.

One way I tested these fixes was to by introducing short sleep in 
ClassFileParser::fill_instance_klass() right after the Klass gets added 
to the ClassLoaderData:

    _loader_data->add_class(ik, publicize);
+  os::naked_short_sleep(2);

By doing this the bug reproduced almost every time, and the fixes 
resolved it.

You'll also notice a bug fix in ClassLoaderData.allEntriesDo(). The 
outside loop is not only unnecessary, but results in the same Klass 
being visited multiple times. It turns out no one uses this 
functionality, but I fixed it anyway rather than rip it out because it 
seemed it might be useful in the future.

The changes in ClhsdbClasses.java test are to better check for expected 
classes when dumping the set of all classes. I added these after 
realizing I had introduced a bug that caused only InstanceKlasses to be 
dumped, not interfaces or arrays, so I added a few more types to the 
list that we check.

thanks,

Chris