6751643: ThreadReference.ownedMonitors() can return null
Summary: Make a local copy of the cache so it doesn't get modified by a racy resume
Reviewed-by: dcubed, swamyv
--- a/jdk/src/share/classes/com/sun/tools/jdi/ThreadReferenceImpl.java Mon Sep 29 22:10:00 2008 -0700
+++ b/jdk/src/share/classes/com/sun/tools/jdi/ThreadReferenceImpl.java Thu Oct 02 18:23:23 2008 -0700
@@ -61,7 +61,8 @@
private ThreadGroupReference threadGroup;
// This is cached only while this one thread is suspended. Each time
- // the thread is resumed, we clear this and start with a fresh one.
+ // the thread is resumed, we abandon the current cache object and
+ // create a new intialized one.
private static class LocalCache {
JDWP.ThreadReference.Status status = null;
List<StackFrame> frames = null;
@@ -74,6 +75,28 @@
boolean triedCurrentContended = false;
}
+ /*
+ * The localCache instance var is set by resetLocalCache to an initialized
+ * object as shown above. This occurs when the ThreadReference
+ * object is created, and when the mirrored thread is resumed.
+ * The fields are then filled in by the relevant methods as they
+ * are called. A problem can occur if resetLocalCache is called
+ * (ie, a resume() is executed) at certain points in the execution
+ * of some of these methods - see 6751643. To avoid this, each
+ * method that wants to use this cache must make a local copy of
+ * this variable and use that. This means that each invocation of
+ * these methods will use a copy of the cache object that was in
+ * effect at the point that the copy was made; if a racy resume
+ * occurs, it won't affect the method's local copy. This means that
+ * the values returned by these calls may not match the state of
+ * the debuggee at the time the caller gets the values. EG,
+ * frameCount() is called and comes up with 5 frames. But before
+ * it returns this, a resume of the debuggee thread is executed in a
+ * different debugger thread. The thread is resumed and running at
+ * the time that the value 5 is returned. Or even worse, the thread
+ * could be suspended again and have a different number of frames, eg, 24,
+ * but this call will still return 5.
+ */
private LocalCache localCache;
private void resetLocalCache() {
@@ -129,8 +152,9 @@
}
/**
- * Note that we only cache the name string while suspended because
- * it can change via Thread.setName arbitrarily
+ * Note that we only cache the name string while the entire VM is suspended
+ * because the name can change via Thread.setName arbitrarily while this
+ * thread is running.
*/
public String name() {
String name = null;
@@ -240,19 +264,20 @@
}
private JDWP.ThreadReference.Status jdwpStatus() {
- JDWP.ThreadReference.Status myStatus = localCache.status;
+ LocalCache snapshot = localCache;
+ JDWP.ThreadReference.Status myStatus = snapshot.status;
try {
if (myStatus == null) {
myStatus = JDWP.ThreadReference.Status.process(vm, this);
if ((myStatus.suspendStatus & SUSPEND_STATUS_SUSPENDED) != 0) {
// thread is suspended, we can cache the status.
- localCache.status = myStatus;
+ snapshot.status = myStatus;
}
}
} catch (JDWPException exc) {
throw exc.toJDIException();
}
- return myStatus;
+ return myStatus;
}
public int status() {
@@ -304,9 +329,10 @@
}
public int frameCount() throws IncompatibleThreadStateException {
+ LocalCache snapshot = localCache;
try {
- if (localCache.frameCount == -1) {
- localCache.frameCount = JDWP.ThreadReference.FrameCount
+ if (snapshot.frameCount == -1) {
+ snapshot.frameCount = JDWP.ThreadReference.FrameCount
.process(vm, this).frameCount;
}
} catch (JDWPException exc) {
@@ -318,7 +344,7 @@
throw exc.toJDIException();
}
}
- return localCache.frameCount;
+ return snapshot.frameCount;
}
public List<StackFrame> frames() throws IncompatibleThreadStateException {
@@ -335,22 +361,22 @@
* local is known to be non-null. Should only be called from
* a sync method.
*/
- private boolean isSubrange(LocalCache localCache,
+ private boolean isSubrange(LocalCache snapshot,
int start, int length) {
- if (start < localCache.framesStart) {
+ if (start < snapshot.framesStart) {
return false;
}
if (length == -1) {
- return (localCache.framesLength == -1);
+ return (snapshot.framesLength == -1);
}
- if (localCache.framesLength == -1) {
- if ((start + length) > (localCache.framesStart +
- localCache.frames.size())) {
+ if (snapshot.framesLength == -1) {
+ if ((start + length) > (snapshot.framesStart +
+ snapshot.frames.size())) {
throw new IndexOutOfBoundsException();
}
return true;
}
- return ((start + length) <= (localCache.framesStart + localCache.framesLength));
+ return ((start + length) <= (snapshot.framesStart + snapshot.framesLength));
}
public List<StackFrame> frames(int start, int length)
@@ -371,14 +397,14 @@
// Lock must be held while creating stack frames so if that two threads
// do this at the same time, one won't clobber the subset created by the other.
-
+ LocalCache snapshot = localCache;
try {
- if (localCache.frames == null || !isSubrange(localCache, start, length)) {
+ if (snapshot.frames == null || !isSubrange(snapshot, start, length)) {
JDWP.ThreadReference.Frames.Frame[] jdwpFrames
= JDWP.ThreadReference.Frames.
process(vm, this, start, length).frames;
int count = jdwpFrames.length;
- localCache.frames = new ArrayList<StackFrame>(count);
+ snapshot.frames = new ArrayList<StackFrame>(count);
for (int i = 0; i<count; i++) {
if (jdwpFrames[i].location == null) {
@@ -388,20 +414,20 @@
jdwpFrames[i].frameID,
jdwpFrames[i].location);
// Add to the frame list
- localCache.frames.add(frame);
+ snapshot.frames.add(frame);
}
- localCache.framesStart = start;
- localCache.framesLength = length;
- return Collections.unmodifiableList(localCache.frames);
+ snapshot.framesStart = start;
+ snapshot.framesLength = length;
+ return Collections.unmodifiableList(snapshot.frames);
} else {
- int fromIndex = start - localCache.framesStart;
+ int fromIndex = start - snapshot.framesStart;
int toIndex;
if (length == -1) {
- toIndex = localCache.frames.size() - fromIndex;
+ toIndex = snapshot.frames.size() - fromIndex;
} else {
toIndex = fromIndex + length;
}
- return Collections.unmodifiableList(localCache.frames.subList(fromIndex, toIndex));
+ return Collections.unmodifiableList(snapshot.frames.subList(fromIndex, toIndex));
}
} catch (JDWPException exc) {
switch (exc.errorCode()) {
@@ -415,15 +441,16 @@
}
public List<ObjectReference> ownedMonitors() throws IncompatibleThreadStateException {
+ LocalCache snapshot = localCache;
try {
- if (localCache.ownedMonitors == null) {
- localCache.ownedMonitors = Arrays.asList(
+ if (snapshot.ownedMonitors == null) {
+ snapshot.ownedMonitors = Arrays.asList(
(ObjectReference[])JDWP.ThreadReference.OwnedMonitors.
process(vm, this).owned);
if ((vm.traceFlags & vm.TRACE_OBJREFS) != 0) {
vm.printTrace(description() +
" temporarily caching owned monitors"+
- " (count = " + localCache.ownedMonitors.size() + ")");
+ " (count = " + snapshot.ownedMonitors.size() + ")");
}
}
} catch (JDWPException exc) {
@@ -435,54 +462,57 @@
throw exc.toJDIException();
}
}
- return localCache.ownedMonitors;
+ return snapshot.ownedMonitors;
}
public ObjectReference currentContendedMonitor()
throws IncompatibleThreadStateException {
+ LocalCache snapshot = localCache;
try {
- if (localCache.contendedMonitor == null &&
- !localCache.triedCurrentContended) {
- localCache.contendedMonitor = JDWP.ThreadReference.CurrentContendedMonitor.
+ if (snapshot.contendedMonitor == null &&
+ !snapshot.triedCurrentContended) {
+ snapshot.contendedMonitor = JDWP.ThreadReference.CurrentContendedMonitor.
process(vm, this).monitor;
- localCache.triedCurrentContended = true;
- if ((localCache.contendedMonitor != null) &&
+ snapshot.triedCurrentContended = true;
+ if ((snapshot.contendedMonitor != null) &&
((vm.traceFlags & vm.TRACE_OBJREFS) != 0)) {
vm.printTrace(description() +
" temporarily caching contended monitor"+
- " (id = " + localCache.contendedMonitor.uniqueID() + ")");
+ " (id = " + snapshot.contendedMonitor.uniqueID() + ")");
}
}
} catch (JDWPException exc) {
switch (exc.errorCode()) {
+ case JDWP.Error.THREAD_NOT_SUSPENDED:
case JDWP.Error.INVALID_THREAD: /* zombie */
throw new IncompatibleThreadStateException();
default:
throw exc.toJDIException();
}
}
- return localCache.contendedMonitor;
+ return snapshot.contendedMonitor;
}
public List<MonitorInfo> ownedMonitorsAndFrames() throws IncompatibleThreadStateException {
+ LocalCache snapshot = localCache;
try {
- if (localCache.ownedMonitorsInfo == null) {
+ if (snapshot.ownedMonitorsInfo == null) {
JDWP.ThreadReference.OwnedMonitorsStackDepthInfo.monitor[] minfo;
minfo = JDWP.ThreadReference.OwnedMonitorsStackDepthInfo.process(vm, this).owned;
- localCache.ownedMonitorsInfo = new ArrayList<MonitorInfo>(minfo.length);
+ snapshot.ownedMonitorsInfo = new ArrayList<MonitorInfo>(minfo.length);
for (int i=0; i < minfo.length; i++) {
JDWP.ThreadReference.OwnedMonitorsStackDepthInfo.monitor mi =
minfo[i];
MonitorInfo mon = new MonitorInfoImpl(vm, minfo[i].monitor, this, minfo[i].stack_depth);
- localCache.ownedMonitorsInfo.add(mon);
+ snapshot.ownedMonitorsInfo.add(mon);
}
if ((vm.traceFlags & vm.TRACE_OBJREFS) != 0) {
vm.printTrace(description() +
" temporarily caching owned monitors"+
- " (count = " + localCache.ownedMonitorsInfo.size() + ")");
+ " (count = " + snapshot.ownedMonitorsInfo.size() + ")");
}
}
@@ -495,7 +525,7 @@
throw exc.toJDIException();
}
}
- return localCache.ownedMonitorsInfo;
+ return snapshot.ownedMonitorsInfo;
}
public void popFrames(StackFrame frame) throws IncompatibleThreadStateException {
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/com/sun/jdi/SimulResumerTest.java Thu Oct 02 18:23:23 2008 -0700
@@ -0,0 +1,278 @@
+/*
+ * Copyright 2008 Sun Microsystems, Inc. All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
+ * CA 95054 USA or visit www.sun.com if you need additional information or
+ * have any questions.
+ */
+
+/**
+ * @test
+ * @bug 6751643
+ * @summary ThreadReference.ownedMonitors() can return null
+ *
+ * @author jjh
+ *
+ * @run build TestScaffold VMConnection TargetListener TargetAdapter
+ * @run compile -g SimulResumerTest.java
+ * @run main SimulResumerTest
+ */
+import com.sun.jdi.*;
+import com.sun.jdi.event.*;
+import com.sun.jdi.request.*;
+
+import java.util.*;
+
+/*
+ * This debuggee basically runs two threads each of
+ * which loop, hitting a bkpt in each iteration.
+ *
+ */
+class SimulResumerTarg extends Thread {
+ static boolean one = false;
+ static String name1 = "Thread 1";
+ static String name2 = "Thread 2";
+ static int count = 10000;
+ public static void main(String[] args) {
+ System.out.println("Howdy!");
+ SimulResumerTarg t1 = new SimulResumerTarg(name1);
+ SimulResumerTarg t2 = new SimulResumerTarg(name2);
+
+ t1.start();
+ t2.start();
+ }
+
+ public SimulResumerTarg(String name) {
+ super(name);
+ }
+
+ public void run() {
+ if (getName().equals(name1)) {
+ run1();
+ } else {
+ run2();
+ }
+ }
+
+ public void bkpt1(int i) {
+ synchronized(name1) {
+ yield();
+ }
+ }
+
+ public void run1() {
+ int i = 0;
+ while (i < count) {
+ i++;
+ bkpt1(i);
+ }
+ }
+
+ public void bkpt2(int i) {
+ synchronized(name2) {
+ yield();
+ }
+ }
+
+ public void run2() {
+ int i = 0;
+ while (i < count) {
+ i++;
+ bkpt2(i);
+ }
+ }
+}
+
+/********** test program **********/
+
+public class SimulResumerTest extends TestScaffold {
+ ReferenceType targetClass;
+ ThreadReference mainThread;
+ BreakpointRequest request1;
+ BreakpointRequest request2;
+ static volatile int bkpts = 0;
+ static int iters = 0;
+ Thread resumerThread;
+ static int waitTime = 100;
+ ThreadReference debuggeeThread1 = null;
+ ThreadReference debuggeeThread2 = null;
+
+ SimulResumerTest (String args[]) {
+ super(args);
+ }
+
+ public static void main(String[] args) throws Exception {
+ new SimulResumerTest(args).startTests();
+ }
+
+ /* BreakpointEvent handler */
+
+ public void breakpointReached(BreakpointEvent event) {
+ // save ThreadRefs for the two debuggee threads
+ ThreadReference thr = event.thread();
+ if (bkpts == 0) {
+ resumerThread.start();
+ debuggeeThread1 = thr;
+ System.out.println("thr1 = " + debuggeeThread1);
+ }
+
+ if (debuggeeThread2 == null && thr != debuggeeThread1) {
+ debuggeeThread2 = thr;
+ System.out.println("thr2 = " + debuggeeThread2);
+ }
+
+ synchronized("abc") {
+ bkpts++;
+ }
+ /**
+ if (bkpts >= SimulResumerTarg.count * 2) {
+ resumerThread.interrupt();
+ }
+ *****/
+
+ }
+
+ /********** test core **********/
+
+ void check(ThreadReference thr) {
+ // This calls each ThreadReference method that could fail due to the bug
+ // that occurs if a resume is done while a call to the method is in process.
+ String kind = "";
+ if (thr != null) {
+ try {
+ kind = "ownedMonitors()";
+ System.out.println("kind = " + kind);
+ if (thr.ownedMonitors() == null) {
+ failure("failure: ownedMonitors = null");
+ }
+
+ kind = "ownedMonitorsAndFrames()";
+ System.out.println("kind = " + kind);
+ if (thr.ownedMonitorsAndFrames() == null) {
+ failure("failure: ownedMonitorsAndFrames = null");
+ }
+
+ kind = "currentContendedMonitor()";
+ System.out.println("kind = " + kind);
+ thr.currentContendedMonitor();
+ // no failure return value here; could cause an NPE
+
+ kind = "frames()";
+ System.out.println("kind = " + kind);
+ List<StackFrame> frames = thr.frames();
+ // no failure return value here; could cause an NPE
+
+ int nframes = frames.size();
+ if (nframes > 0) {
+ // hmm, how could it ever be 0?
+ kind = "frames(0, size - 1)";
+ System.out.println("kind = " + kind);
+ thr.frames(0, frames.size() - 1);
+ }
+
+ kind = "frameCount()";
+ System.out.println("kind = " + kind);
+ if (thr.frameCount() == -1) {
+ failure("failure: frameCount = -1");
+ }
+
+ kind = "name()";
+ System.out.println("kind = " + kind);
+ if (thr.name() == null) {
+ failure("failure: name = null");
+ }
+
+ kind = "status()";
+ System.out.println("kind = " + kind);
+ if (thr.status() < 0) {
+ failure("failure: status < 0");
+ }
+
+ } catch (IncompatibleThreadStateException ee) {
+ // ignore
+ } catch (VMDisconnectedException ee) {
+ // This is how we stop. The debuggee runs to completion
+ // and we get this exception.
+ throw ee;
+ } catch (Exception ee) {
+ failure("failure: Got exception from " + kind + ": " + ee );
+ }
+ }
+ }
+
+ protected void runTests() throws Exception {
+ /*
+ * Get to the top of main()
+ * to determine targetClass and mainThread
+ */
+ BreakpointEvent bpe = startToMain("SimulResumerTarg");
+ targetClass = bpe.location().declaringType();
+ mainThread = bpe.thread();
+ EventRequestManager erm = vm().eventRequestManager();
+ final Thread mainThread = Thread.currentThread();
+
+ /*
+ * Set event requests
+ */
+ Location loc1 = findMethod(targetClass, "bkpt1", "(I)V").location();
+ Location loc2 = findMethod(targetClass, "bkpt2", "(I)V").location();
+ request1 = erm.createBreakpointRequest(loc1);
+ request2 = erm.createBreakpointRequest(loc2);
+ request1.enable();
+ request2.enable();
+
+ /*
+ * This thread will be started when we get the first bkpt.
+ */
+ resumerThread = new Thread("test resumer") {
+ public void run() {
+ while (true) {
+ iters++;
+ System.out.println("bkpts = " + bkpts + ", iters = " + iters);
+ try {
+ Thread.sleep(waitTime);
+ check(debuggeeThread1);
+ check(debuggeeThread2);
+ } catch (InterruptedException ee) {
+ // If the test completes, this occurs.
+ println("resumer Interrupted");
+ break;
+ } catch (VMDisconnectedException ee) {
+ println("VMDisconnectedException");
+ break;
+ }
+ }
+ }
+ };
+
+ /*
+ * resume the target, listening for events
+ */
+ listenUntilVMDisconnect();
+ resumerThread.interrupt();
+ /*
+ * deal with results of test
+ * if anything has called failure("foo") testFailed will be true
+ */
+ if (!testFailed) {
+ println("SimulResumerTest: passed; bkpts = " + bkpts + ", iters = " + iters);
+ } else {
+ throw new Exception("SimulResumerTest: failed; bkpts = " + bkpts + ", iters = " + iters);
+ }
+ }
+}