RFR: 691423 - (str) Missing synchronization in java.lang.String#contentEquals(CharSequence)

[Jim Gish]

OK.  With that in mind, here's an update where I leave contentEquals( 
StringBuffer ) in place (adding a clarifying spec), and defer the 
synchronization as I had it to the contentEquals( CharSequence ) method:

diff --git a/src/share/classes/java/lang/String.java 
b/src/share/classes/java/lang/String.java
--- a/src/share/classes/java/lang/String.java
+++ b/src/share/classes/java/lang/String.java
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1994, 2010, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1994, 2012, Oracle and/or its affiliates. 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
@@ -983,11 +983,11 @@
          }
          return false;
      }
-
-    /**
+   /**
       * Compares this string to the specified {@code StringBuffer}.  
The result
       * is {@code true} if and only if this {@code String} represents 
the same
-     * sequence of characters as the specified {@code StringBuffer}.
+     * sequence of characters as the specified {@code StringBuffer}. 
This method
+     * synchronizes on the {@code StringBuffer}.
       *
       * @param  sb
       *         The {@code StringBuffer} to compare this {@code String} 
against
@@ -999,15 +999,29 @@
       * @since  1.4
       */
      public boolean contentEquals(StringBuffer sb) {
-        synchronized (sb) {
-            return contentEquals((CharSequence) sb);
+        return contentEquals((CharSequence) sb);
+    }
+
+    private boolean nonSyncContentEquals(AbstractStringBuilder sb) {
+        char v1[] = value;
+        char v2[] = sb.getValue();
+        int i = 0;
+        int n = value.length;
+        while (n-- != 0) {
+            if (v1[i] != v2[i]) {
+                return false;
+            }
+            i++;
          }
+        return true;
      }

      /**
-     * Compares this string to the specified {@code CharSequence}.  The 
result
-     * is {@code true} if and only if this {@code String} represents 
the same
-     * sequence of char values as the specified sequence.
+     * Compares this string to the specified {@code CharSequence}.  The
+     * result is {@code true} if and only if this {@code String} 
represents the
+     * same sequence of char values as the specified sequence. Note 
that if the
+     * {@code CharSequence} is a {@code StringBuffer} then the method
+     * synchronizes on it.
       *
       * @param  cs
       *         The sequence to compare this {@code String} against
@@ -1023,16 +1037,13 @@
              return false;
          // Argument is a StringBuffer, StringBuilder
          if (cs instanceof AbstractStringBuilder) {
-            char v1[] = value;
-            char v2[] = ((AbstractStringBuilder) cs).getValue();
-            int i = 0;
-            int n = value.length;
-            while (n-- != 0) {
-                if (v1[i] != v2[i])
-                    return false;
-                i++;
+            if (cs instanceof StringBuffer) {
+                synchronized(cs) {
+                   return nonSyncContentEquals((AbstractStringBuilder)cs);
+                }
+            } else {
+                return nonSyncContentEquals((AbstractStringBuilder)cs);
              }
-            return true;
          }
          // Argument is a String
          if (cs.equals(this))



On 07/26/2012 04:28 PM, Mike Duigou wrote:
> This would appear to be source compatible but not binary compatible. I don't believe we can remove the contentsEqual(StringBuffer) overload. Code compiled against the existing interface would fail to find the CharSequence interface at runtime and fail.
>
> I believe it would be reasonable to add locking on StringBuffer into the contentsEqual(CharSequence) overload. This would make the behaviour consistent between the two invocations which is important because it will not always be possible for developers to tell which overload is being selected.
>
> Mike
>