[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Fwd: RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2020-05-21 14:28:19
Message-ID: d1d88b15-7aaa-38c5-25ab-cb00f954cf90 () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
</head>
<body>
Ping!<br>
<div class="moz-forward-container"><br>
-------- Forwarded Message --------
<table class="moz-email-headers-table" cellspacing="0"
cellpadding="0" border="0">
<tbody>
<tr>
<th valign="BASELINE" nowrap="nowrap" align="RIGHT">Subject:
</th>
<td>RFR(S): 8244203:
sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails
with NullPointerException</td>
</tr>
<tr>
<th valign="BASELINE" nowrap="nowrap" align="RIGHT">Date: </th>
<td>Tue, 19 May 2020 12:47:17 -0700</td>
</tr>
<tr>
<th valign="BASELINE" nowrap="nowrap" align="RIGHT">From: </th>
<td>Chris Plummer <a class="moz-txt-link-rfc2396E" \
href="mailto:chris.plummer@oracle.com"><chris.plummer@oracle.com></a></td> \
</tr> <tr>
<th valign="BASELINE" nowrap="nowrap" align="RIGHT">To: </th>
<td>serviceability-dev
<a class="moz-txt-link-rfc2396E" \
href="mailto:serviceability-dev@openjdk.java.net"><serviceability-dev@openjdk.java.net></a></td>
</tr>
</tbody>
</table>
<br>
<br>
Hello,<br>
<br>
Please review the following:<br>
<br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~cjplummer/8244203/webrev.00/index.html">http://cr.openjdk.java.net/~cjplummer/8244203/webrev.00/index.html</a><br>
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8244203">https://bugs.openjdk.java.net/browse/JDK-8244203</a><br>
<br>
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.<br>
<br>
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..<br>
<br>
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:<br>
<br>
public void classesDo(ClassLoaderDataGraph.ClassVisitor v) {<br>
for (Klass l = getKlasses(); l != null; l =
l.getNextLinkKlass()) {<br>
v.visit(l);<br>
}<br>
}<br>
<br>
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.<br>
<br>
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:<br>
<br>
_loader_data->add_class(ik, publicize);<br>
+ os::naked_short_sleep(2);<br>
<br>
By doing this the bug reproduced almost every time, and the fixes
resolved it.<br>
<br>
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.<br>
<br>
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.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic