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

[Chris Plummer]

Yes, you are correct. As I mentioned earlier, it turns out this 
functionality isn't used, other wise it also would have been exposed by 
the other bug I fixed (the unnecessary outter loop). I'll fix this to 
use !isLoaded().

thanks,

Chris

On 5/21/20 10:44 AM, Daniil Titov wrote:
> Hi Chris,
>
> I have a question about a change in
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/Dictionary.java.
>
> 61   /** All classes, and their initiating class loader, passed in. */
>    62   public void allEntriesDo(ClassLoaderDataGraph.ClassAndLoaderVisitor v, Oop loader) {
>    63     int tblSize = tableSize();
>    64     for (int index = 0; index < tblSize; index++) {
>    65       for (DictionaryEntry probe = (DictionaryEntry) bucket(index); probe != null;
>    66                                               probe = (DictionaryEntry) probe.next()) {
>    67         Klass k = probe.klass();
>    68         // Only visit InstanceKlasses that are at least in the "loaded" init_state. Otherwise
>    69         // the InstanceKlass won't have some required fields initialized, which can cause problems.
>    70         if (k instanceof InstanceKlass && ((InstanceKlass)k).isLoaded()) {
>    71             continue;
>    72         }
>    73         v.visit(k, loader);
>    74       }
>    75     }
>    76   }
>
>
> Based on the comment at lines 68-69 should not condition on line 70 be
>
> 70         if (k instanceof InstanceKlass && !((InstanceKlass)k).isLoaded()) {
>
> in order to we skip  InstanceKlasses  that are not in the "loaded" state?
>
> Thank you,
> Daniil
>
> On 5/19/20, 12:47 PM, "serviceability-dev on behalf of Chris Plummer" <serviceability-dev-bounces at openjdk.java.net on behalf of chris.plummer at oracle.com> wrote:
>
>      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
>
>
>
>