JDK-6887249 : Get rid of double-check for isValid() idiom in validate() methods
  • Type: Bug
  • Component: client-libs
  • Sub-Component: java.awt
  • Affected Version: 7
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2009-10-01
  • Updated: 2011-01-19
  • Resolved: 2009-11-25
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 7
7 b77Fixed
Description
The Container.validate() method uses the following idiom to boost the performance:

if (!isValid()) {
   synchronized (getTreeLock()) {
      if (!isValid()) {
         // here go all the real actions
      }
   }
}

This was done to avoid grabbing the lock when it doesn't seem to be needed. However, the isValid() method actually retrieves the value of the 'valid' private memeber which must only be updated while holding the lock. That means that reading the value w/o the lock may theoretically produce an incorrect result.

Therefore, it is suggested to remove the outer if () statement and only check for validity under the lock.
Comments
SUGGESTED FIX --- old/src/share/classes/java/awt/Container.java 2009-10-01 17:07:33.000000000 +0400 +++ new/src/share/classes/java/awt/Container.java 2009-10-01 17:07:33.000000000 +0400 @@ -1542,28 +1542,25 @@ * @see #validateTree */ public void validate() { - /* Avoid grabbing lock unless really necessary. */ - if (!isValid()) { - boolean updateCur = false; - synchronized (getTreeLock()) { - if (!isValid() && peer != null) { - ContainerPeer p = null; - if (peer instanceof ContainerPeer) { - p = (ContainerPeer) peer; - } - if (p != null) { - p.beginValidate(); - } - validateTree(); - if (p != null) { - p.endValidate(); - updateCur = isVisible(); - } + boolean updateCur = false; + synchronized (getTreeLock()) { + if (!isValid() && peer != null) { + ContainerPeer p = null; + if (peer instanceof ContainerPeer) { + p = (ContainerPeer) peer; + } + if (p != null) { + p.beginValidate(); + } + validateTree(); + if (p != null) { + p.endValidate(); + updateCur = isVisible(); } } - if (updateCur) { - updateCursorImmediately(); - } + } + if (updateCur) { + updateCursorImmediately(); } }
01-10-2009
EVALUATION Need to check for isValid() under the TreeLock only.
01-10-2009