From ce4a9015d3fc73ee6da23097c59a6795ec329c16 Mon Sep 17 00:00:00 2001 From: jjh Date: Thu, 2 Oct 2008 18:23:23 -0700 Subject: 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 --- .../com/sun/tools/jdi/ThreadReferenceImpl.java | 114 +++++---- test/com/sun/jdi/SimulResumerTest.java | 278 +++++++++++++++++++++ 2 files changed, 350 insertions(+), 42 deletions(-) create mode 100644 test/com/sun/jdi/SimulResumerTest.java diff --git a/src/share/classes/com/sun/tools/jdi/ThreadReferenceImpl.java b/src/share/classes/com/sun/tools/jdi/ThreadReferenceImpl.java index 7af01561e..0f18120f1 100644 --- a/src/share/classes/com/sun/tools/jdi/ThreadReferenceImpl.java +++ b/src/share/classes/com/sun/tools/jdi/ThreadReferenceImpl.java @@ -61,7 +61,8 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl 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 frames = null; @@ -74,6 +75,28 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl 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 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl } /** - * 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 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl } 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 class ThreadReferenceImpl extends ObjectReferenceImpl } 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 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl throw exc.toJDIException(); } } - return localCache.frameCount; + return snapshot.frameCount; } public List frames() throws IncompatibleThreadStateException { @@ -335,22 +361,22 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl * 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 frames(int start, int length) @@ -371,14 +397,14 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl // 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(count); + snapshot.frames = new ArrayList(count); for (int i = 0; i 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 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl 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 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(minfo.length); + snapshot.ownedMonitorsInfo = new ArrayList(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 @@ public class ThreadReferenceImpl extends ObjectReferenceImpl throw exc.toJDIException(); } } - return localCache.ownedMonitorsInfo; + return snapshot.ownedMonitorsInfo; } public void popFrames(StackFrame frame) throws IncompatibleThreadStateException { diff --git a/test/com/sun/jdi/SimulResumerTest.java b/test/com/sun/jdi/SimulResumerTest.java new file mode 100644 index 000000000..cf7568e8b --- /dev/null +++ b/test/com/sun/jdi/SimulResumerTest.java @@ -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 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); + } + } +} -- cgit v1.2.3