Preparing for the 0.2 draft
Joshua Bloch
jjb at google.com
Wed Feb 3 21:56:35 PST 2010
Neal found 48 locations in OpenJDK 6 in which he claimed that evaluating
"this" in the surrounding context would be helpful. I decided to take a
closer look at them in hopes of learning something.
Roughly speaking, here's what I found. Of the 48 locations, 32 appear to be
"legitimate" (in that a qualified "this" is currently required, and could be
replaced by an unqualified this if it were evaluated in the enclosing scope.
Of these 32, 14 are calls to AccessController.doPrivileged, 12 are calls to
EventQueue.invokeLater, and 5 add or remove PropertyChangeListeners. Some of
these 32 instance creations also contain
Of the remaining 16 locations, 4 are "superfluous" (the qualified this could
be removed without harm to the code); 4 could not take advantage of the
"enclosing scope semantics for this," because the anonymous class has a
method that shadows the one in the enclosing class (see (1) below for an
example); 2 require a "this" only because a local variable or parameter
shadows a field in the enclosing class (the local variable or parameter
should be renamed); and 6 aren't "legitimate" (as defined above) but don't
fit into any of the above categories. For amusement, take a look at (22)
below. Unless I'm deeply mistaken, it's an in-your-face Swing bug. I have no
idea how it eluded detection. There is a more subtle Swing bug (race
condition) in (23).
I don't believe this constitutes a compelling case for interpreting this in
the enclosing context, but I understands that others may differ on this
point.
Here, for masochists, are brief discussions of all 48 locations identified
by Neal, including the code:
(1) src/share/classes/com/sun/java/util/jar/pack/Histogram.java:187
class Histogram {
public double getBitLength(int value) {
double prob = (double) getFrequency(value) / getTotalWeight();
return - Math.log(prob) / log2;
}
private final BitMetric bitMetric = new BitMetric() {
public double getBitLength(int value) {
return Histogram.this.getBitLength(value);
}
};
}
The qualified this is necessary only because the anonymous class gives its
sole method the same name as the one that it wants to call in its enclosing
instance. But you can't name the method in a lambda expression, so having
"this" evaluated in the surrounding context would not be necessary (or
helpful) here.
(2) src/share/classes/com/sun/media/sound/SoftJitterCorrector.java:126
The relevant code looks like this:
synchronized (JitterStream.this) {
if (!active)
break;
}
The only other code that synchronizes on JitterStream instances is this:
synchronized (this) {
active = false;
}
You could remove all of the synchronization and improve the code's beauty
and performance by declaring active to be volatile.
(3) src/share/classes/com/sun/security/auth/PolicyFile.java:849
The qualified this would be unnecessary if PolicyPermissions had been
declared as a (nonstatic) nested class of PolicyFile. And the code would
have been simpler and cleaner. Also note this class has been "entirely
deprecated."
(4) src/share/classes/com/sun/tools/example/debug/bdi/JDIEventSource.java:80
Here's the code:
boolean wantInterrupt = JDIEventSource.this.wantInterrupt;
The qualified this is necessary only because the local variable has the same
name as the field in the enclosing class, hence the local variable shadows
the field. If the local variable is given a different name, the resulting
code requires no "this," qualified or otherwise.
(5) src/share/classes/com/sun/tools/jdi/ClassTypeImpl.java:167
This appears to be a legitimate use:
CommandSender sender =
new CommandSender() {
public PacketStream send() {
return JDWP.ClassType.InvokeMethod.enqueueCommand(
vm, ClassTypeImpl.this, thread,
method.ref(), args, options);
}
};
In other words, you could eliminate the characters "ClassTypeImpl." if
"this" were evaluated in the surrounding context. (Whether if would be more
natural or not is a matter of debate.)
(6) src/share/classes/com/sun/tools/jdi/ClassTypeImpl.java:189
This is a copy-and-paste of (5), and also legitimate:
CommandSender sender =
new CommandSender() {
public PacketStream send() {
return JDWP.ClassType.NewInstance.enqueueCommand(
vm, ClassTypeImpl.this, thread,
method.ref(), args, options);
}
};
(7) src/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java:341
Another copy-and-paste of (5):
CommandSender sender =
new CommandSender() {
public PacketStream send() {
return JDWP.ObjectReference.InvokeMethod.enqueueCommand(
vm, ObjectReferenceImpl.this,
thread, refType,
method.ref(), args, options);
}
};
(8) src/share/classes/java/awt/Container.java:2744
Seems legitimate.
Runnable pumpEventsForHierarchy = new Runnable() {
public void run() {
EventDispatchThread dispatchThread =
(EventDispatchThread)Thread.currentThread();
dispatchThread.pumpEventsForHierarchy(
new Conditional() {
public boolean evaluate() {
return ((windowClosingException == null) &&
(nativeContainer.modalComp != null)) ;
}
}, Container.this);
}
};
(9) src/share/classes/java/awt/Container.java:4268
Seems legitimate.
java.security.AccessController.doPrivileged(
new java.security.PrivilegedAction() {
public Object run() {
nativeContainer.getToolkit().addAWTEventListener(
LightweightDispatcher.this,
AWTEvent.MOUSE_EVENT_MASK |
AWTEvent.MOUSE_MOTION_EVENT_MASK);
return null;
}
}
);
(10) src/share/classes/java/awt/Container.java:4283
Complementary to (9). Also legitimate.
java.security.AccessController.doPrivileged(
new java.security.PrivilegedAction() {
public Object run() {
nativeContainer.getToolkit().removeAWTEventListener(LightweightDispatcher.this);
return null;
}
}
);
(11) src/share/classes/java/awt/EventQueue.java:823
Seems legitimate:
dispatchThread = (EventDispatchThread)
AccessController.doPrivileged(new PrivilegedAction() {
public Object run() {
EventDispatchThread t =
new EventDispatchThread(threadGroup,
name,
EventQueue.this);
t.setContextClassLoader(classLoader);
t.setPriority(Thread.NORM_PRIORITY + 1);
t.setDaemon(false);
return t;
}
});
(12) src/share/classes/java/awt/SequencedEvent.java:92
Superfluous. You could remove the characters "SequencedEvent.this." to make
the code shorter and clearer:
edt.pumpEvents(SentEvent.ID, new Conditional() {
public boolean evaluate() {
return !SequencedEvent.this.isFirstOrDisposed();
}
});
(13) src/share/classes/java/awt/datatransfer/Clipboard.java:128
Appears legitimate:
EventQueue.invokeLater(new Runnable() {
public void run() {
oldOwner.lostOwnership(Clipboard.this, oldContents);
}
});
(14) src/share/classes/java/awt/datatransfer/Clipboard.java:326
Appears legitimate:
EventQueue.invokeLater(new Runnable() {
public void run() {
listener.flavorsChanged(new FlavorEvent(Clipboard.this));
}
});
(15) src/share/classes/java/beans/beancontext/BeanContextSupport.java:1296
Like (1). Evaluating "this" in the surrounding context would not be helpful.
public void propertyChange(PropertyChangeEvent pce) {
BeanContextSupport.this.propertyChange(pce);
}
(16) src/share/classes/java/beans/beancontext/BeanContextSupport.java:1310
Like (1).
public void vetoableChange(PropertyChangeEvent pce) throws
PropertyVetoException {
BeanContextSupport.this.vetoableChange(pce);
}
(17) src/share/classes/java/lang/Class.java:1306
Appears legitimate:
Class[] result = (Class[])
java.security.AccessController.doPrivileged
(new java.security.PrivilegedAction() {
public Object run() {
java.util.List<Class> list = new java.util.ArrayList();
Class currentClass = Class.this;
while (currentClass != null) {
Class[] members = currentClass.getDeclaredClasses();
for (int i = 0; i < members.length; i++) {
if
(Modifier.isPublic(members[i].getModifiers())) {
list.add(members[i]);
}
}
currentClass = currentClass.getSuperclass();
}
Class[] empty = {};
return list.toArray(empty);
}
});
(18) src/share/classes/java/nio/channels/spi/AbstractSelector.java:208
Superfluous. Could (and should) remove "AbstractSelector.this." without harm
to the code.
interruptor = new Interruptible() {
public void interrupt() {
AbstractSelector.this.wakeup();
}};
(19) src/share/classes/java/security/ProtectionDomain.java:317
Appears legitimate:
PermissionCollection perms =
java.security.AccessController.doPrivileged
(new java.security.PrivilegedAction<PermissionCollection>() {
public PermissionCollection run() {
Policy p = Policy.getPolicyNoCheck();
return p.getPermissions(ProtectionDomain.this);
}
});
(20) src/share/classes/java/util/regex/Pattern.java:3352
Like (1).
private static abstract class CharProperty extends Node {
abstract boolean isSatisfiedBy(int ch);
CharProperty complement() {
return new CharProperty() {
boolean isSatisfiedBy(int ch) {
return ! CharProperty.this.isSatisfiedBy(ch);}};
}
(21) src/share/classes/javax/management/monitor/Monitor.java:1609
Superfluous (and repeated 4 times!).
PrivilegedAction<Void> action = new PrivilegedAction<Void>() {
public Void run() {
if (Monitor.this.isActive()) {
final int an[] = alreadyNotifieds;
int index = 0;
for (ObservedObject o : Monitor.this.observedObjects) {
if (Monitor.this.isActive()) {
Monitor.this.monitor(o, index++, an);
}
}
}
return null;
}
};
(22) src/share/classes/javax/swing/BufferStrategyPaintManager.java:213
OH MY GOD! This code has four uses of this. Three are silly (necessitated
by the fact that the the run method has a local variable with the same name
as a field in the enclosing class, as in (4) above). One is horribly
broken:
SwingUtilities.invokeLater(new Runnable() {
public void run() {
java.util.List<BufferInfo> bufferInfos;
synchronized(BufferStrategyPaintManager.this) {
while (showing) {
try {
wait();
} catch (InterruptedException ie) {
}
}
bufferInfos =
BufferStrategyPaintManager.this.bufferInfos;
BufferStrategyPaintManager.this.bufferInfos = null;
}
dispose(bufferInfos);
}
});
Note that the code is waiting on the anonymous instance, but synchronizing
on the containing BufferStrategyPaintManager instance. This will throw an
IllegalMonitorStateException as soon as it waits! This is definitely a bug.
I wonder why it hasn't been reported.
So what's the right solution? You could fix this by using
BufferStrategyPaintManager.this twice, but this is ugly and error prone.
Better would be to uses something higher-level than wait/notify. If you did
that, you wouldn't need to access BufferStrategyPaintManager.this at all.
(23) src/share/classes/javax/swing/JComponent.java:4804
This is also broken. The anonymous Runnable goes to great lengths to
synchronize on its outer JComponent instance before calling setFlag, but
there are plenty of public methods in the same file that call setFlag
without synchronizing:
Runnable callRevalidate = new Runnable() {
public void run() {
synchronized(JComponent.this) {
setFlag(REVALIDATE_RUNNABLE_SCHEDULED, false);
}
revalidate();
}
};
SwingUtilities.invokeLater(callRevalidate);
(24) src/share/classes/javax/swing/JOptionPane.java:1011
Appears legitimate:
addPropertyChangeListener(new PropertyChangeListener() {
public void propertyChange(PropertyChangeEvent event) {
// Let the defaultCloseOperation handle the closing
// if the user closed the window without selecting a button
// (newValue = null in that case). Otherwise, close the
dialog.
if (dialog.isVisible() && event.getSource() ==
JOptionPane.this &&
(event.getPropertyName().equals(VALUE_PROPERTY) ||
event.getPropertyName().equals(INPUT_VALUE_PROPERTY)) &&
event.getNewValue() != null &&
event.getNewValue() != JOptionPane.UNINITIALIZED_VALUE) {
dialog.setVisible(false);
}
}
});
(25) src/share/classes/javax/swing/JOptionPane.java:1524
Copy-and-paste from 24.
if (iFrame.isVisible() &&
event.getSource() == JOptionPane.this &&
event.getPropertyName().equals(VALUE_PROPERTY)) {
(26) src/share/classes/javax/swing/ProgressMonitor.java:214
Essentially a copy-and-paste from 24.
addPropertyChangeListener(new PropertyChangeListener() {
public void propertyChange(PropertyChangeEvent event) {
if(dialog.isVisible() &&
event.getSource() == ProgressOptionPane.this &&
(event.getPropertyName().equals(VALUE_PROPERTY) ||
event.getPropertyName().equals(INPUT_VALUE_PROPERTY))){
dialog.setVisible(false);
dialog.dispose();
}
}
});
(27) src/share/classes/javax/swing/SwingWorker.java:902
Near as I can tell, the "SwingWorkerPropertyChangeSupport.this." is
superfluous:
public void firePropertyChange(final PropertyChangeEvent evt) {
if (SwingUtilities.isEventDispatchThread()) {
super.firePropertyChange(evt);
} else {
doSubmit.add(
new Runnable() {
public void run() {
SwingWorkerPropertyChangeSupport.this
.firePropertyChange(evt);
}
});
}
(28) src/share/classes/javax/swing/TimerQueue.java:95
Appears legitimate:
java.security.AccessController.doPrivileged(
new java.security.PrivilegedAction() {
public Object run() {
Thread timerThread = new Thread(threadGroup,
TimerQueue.this,
"TimerQueue");
timerThread.setDaemon(true);
timerThread.setPriority(Thread.NORM_PRIORITY);
timerThread.start();
return null;
}
});
running = true;
}
(29) src/share/classes/javax/swing/plaf/basic/BasicLookAndFeel.java:2123
Appears legitimate:
byte[] buffer = (byte[])AccessController.doPrivileged(
new PrivilegedAction() {
public Object run() {
try {
InputStream resource = BasicLookAndFeel.this.
getClass().getResourceAsStream(soundFile);
(30) src/share/classes/javax/swing/plaf/basic/BasicPopupMenuUI.java:745
Appears legitimate:
java.security.AccessController.doPrivileged(
new java.security.PrivilegedAction() {
public Object run() {
tk.addAWTEventListener(MouseGrabber.this,
AWTEvent.MOUSE_EVENT_MASK |
AWTEvent.MOUSE_MOTION_EVENT_MASK |
AWTEvent.MOUSE_WHEEL_EVENT_MASK |
AWTEvent.WINDOW_EVENT_MASK |
SunToolkit.GRAB_EVENT_MASK);
return null;
}
}
);
(31) src/share/classes/javax/swing/plaf/basic/BasicPopupMenuUI.java:778
Appears legitimate (complementary to (30))
java.security.AccessController.doPrivileged(
new java.security.PrivilegedAction() {
public Object run() {
tk.removeAWTEventListener(MouseGrabber.this);
return null;
}
}
);
(32) src/share/classes/javax/swing/text/JTextComponent.java:4932
Looks legitimate (though ripe for a NullPointerException:
isProcessInputMethodEventOverridden returns Boolean):
Boolean ret = (Boolean)AccessController.doPrivileged(new
PrivilegedAction() {
public Object run() {
return isProcessInputMethodEventOverridden(
JTextComponent.this.getClass());
}
});
(33) src/share/classes/sun/applet/AppletClassLoader.java:631
Looks legit (albeit obsolete in several ways):
AccessController.doPrivileged(new PrivilegedAction() {
public Object run() {
threadGroup = new AppletThreadGroup(base + "-threadGroup");
AppContextCreator creatorThread =
new AppContextCreator(threadGroup);
creatorThread.setContextClassLoader(AppletClassLoader.this);
...
}
});
(34) src/share/classes/sun/applet/AppletViewer.java:641
Like (33), legit but obsolete:
void appletSave() {
AccessController.doPrivileged(new PrivilegedAction() {
public Object run() {
panel.sendEvent(AppletPanel.APPLET_STOP);
FileDialog fd = new FileDialog(AppletViewer.this,
amh.getMessage("appletsave.filedialogtitle"),
FileDialog.SAVE);
...
(35) src/share/classes/sun/awt/datatransfer/SunClipboard.java:113
Appears legitimate. Many uses of sunClipboard (= SunClipboard.this) are
superfluous, but a couple are required (synchronization and callbacks).
final Runnable runnable = new Runnable() {
public void run() {
final SunClipboard sunClipboard = SunClipboard.this;
ClipboardOwner owner = null;
Transferable contents = null;
synchronized (sunClipboard) {
final AppContext context = sunClipboard.contentsContext;
if (context == null) {
return;
}
if (disposedContext == null || context ==
disposedContext) {
owner = sunClipboard.owner;
contents = sunClipboard.contents;
sunClipboard.contentsContext = null;
sunClipboard.owner = null;
sunClipboard.contents = null;
sunClipboard.clearNativeContext();
context.removePropertyChangeListener
(AppContext.DISPOSED_PROPERTY_NAME,
sunClipboard);
} else {
return;
}
}
if (owner != null) {
owner.lostOwnership(sunClipboard, contents);
}
}
};
(36) src/share/classes/sun/awt/datatransfer/SunClipboard.java:287
Copy-and-paste from 35.
(37) src/share/classes/sun/awt/im/InputContext.java:648
Highly suspicious. The only reason for the qualified this is so that it can
be cast to a subclass. This method belongs in the subclass. If it were
their, the qualified this would be superfluous:
EventQueue.invokeLater(new Runnable() {
public void run() {
((InputMethodContext)InputContext.this).releaseCompositionArea();
}
});
(38) src/share/classes/sun/security/jca/ProviderConfig.java:244
The debug println could be moved out of the runnable, eliminating the need
for the "qualified this":
return AccessController.doPrivileged(new
PrivilegedAction<Provider>() {
public Provider run() {
if (debug != null) {
debug.println("Loading provider: " +
ProviderConfig.this);
}
(39) src/share/classes/sun/security/provider/SeedGenerator.java:255
Appears legitimate:
Thread t = java.security.AccessController.doPrivileged
(new java.security.PrivilegedAction<Thread>() {
public Thread run() {
ThreadGroup parent, group =
Thread.currentThread().getThreadGroup();
while ((parent = group.getParent()) != null)
group = parent;
finalsg[0] = new ThreadGroup
(group, "SeedGenerator ThreadGroup");
Thread newT = new Thread(finalsg[0],
ThreadedSeedGenerator.this,
"SeedGenerator Thread");
newT.setPriority(Thread.MIN_PRIORITY);
newT.setDaemon(true);
return newT;
}
});
(40) src/share/classes/sun/tools/jconsole/inspector/XSheet.java:181
Three uses of qualified this. The first is only needed because a parameter
shadows a field. The second is purely extraneous. The third seems
legitimate:
private void displayMBeanNode(final DefaultMutableTreeNode node) {
final XNodeInfo uo = (XNodeInfo) node.getUserObject();
if (!uo.getType().equals(Type.MBEAN)) {
return;
}
mbeansTab.workerAdd(new Runnable() {
public void run() {
try {
XSheet.this.node = node; // 1
XSheet.this.mbean = (XMBean) uo.getData(); // 2
mbeanInfo.addMBeanInfo(mbean, mbean.getMBeanInfo());
} catch (Throwable ex) {
EventQueue.invokeLater(new ThreadDialog(
XSheet.this, // 3
ex.getMessage(),
Resources.getText("Problem displaying MBean"),
JOptionPane.ERROR_MESSAGE));
return;
}
...
(41) src/share/classes/sun/tools/jconsole/inspector/XSheet.java:216
Copy-and-paste of 40.
(42) src/share/classes/sun/tools/jconsole/inspector/XSheet.java:329
Copy-and-paste of 40.
(43) src/share/classes/sun/tools/jconsole/inspector/XSheet.java:371
Copy-and-paste of 40.
(44) src/share/classes/sun/tools/jconsole/inspector/XSheet.java:411
Copy-and-paste of 40.
(45) src/share/classes/sun/tools/jconsole/inspector/XSheet.java:668
Like 40, but only the legit use.
(46) src/share/classes/sun/tools/jconsole/inspector/XSheet.java:686
Like 40, but only the legit use.
(47) src/share/classes/sun/tools/jconsole/inspector/XTree.java:159
The qualified this is used to get an object on which to synchronize. It may
be the case that the only thread that ever synchronizes on this object is
the event dispatch thread. If this is true, then the synchronization has no
effect, but I'm not in a position to say for sure:
public synchronized void addMBeanToView(final ObjectName mbean) {
final XMBean xmbean;
try {
xmbean = new XMBean(mbean, mbeansTab);
if (xmbean == null) {
return;
}
} catch (Exception e) {
// Got exception while trying to retrieve the
// given MBean from the underlying MBeanServer
//
if (JConsole.isDebug()) {
e.printStackTrace();
}
return;
}
EventQueue.invokeLater(new Runnable() {
public void run() {
synchronized (XTree.this) {
// Add the new nodes to the MBean tree from leaf to root
...
(48) src/share/classes/sun/tools/jconsole/inspector/XTree.java:261
Like 47.
More information about the lambda-dev
mailing list