Bug in ArrayList iterator

[Chris Hegarty]

On 07/01/15 10:47, Daniel Fuchs wrote:
> On 07/01/15 11:31, Paul Sandoz wrote:
>>
>> On Jan 7, 2015, at 10:45 AM, Remi Forax <forax at univ-mlv.fr> wrote:
>>
>>> A simple Java question, what this code does ?
>>>
>>>   ArrayList<String> list = new ArrayList<>();
>>>   list.add("foo");
>>>   list.add("bar");
>>>   for(String s: list) {
>>>     list.remove(s);
>>>   }
>>>
>>> :(
>>>
>>
>> We could improve the best-effort basis by which
>> ConcurrentModificationException is thrown by snapshotting the
>> collection size on construction of the iterator

This was my thought too.

   diff --git a/src/java.base/share/classes/java/util/ArrayList.java 
b/src/java.base/share/classes/java/util/ArrayList.java
--- a/src/java.base/share/classes/java/util/ArrayList.java
+++ b/src/java.base/share/classes/java/util/ArrayList.java
@@ -853,9 +853,10 @@
          int cursor;       // index of next element to return
          int lastRet = -1; // index of last element returned; -1 if no such
          int expectedModCount = modCount;
+        int itrSize = size;

          public boolean hasNext() {
-            return cursor != size;
+            return cursor != itrSize;
          }

          @SuppressWarnings("unchecked")
@@ -881,6 +882,7 @@
                  cursor = lastRet;
                  lastRet = -1;
                  expectedModCount = modCount;
+                itrSize--;
              } catch (IndexOutOfBoundsException ex) {
                  throw new ConcurrentModificationException();
              }
@@ -909,7 +911,7 @@
          }

          final void checkForComodification() {
-            if (modCount != expectedModCount)
+            if (modCount != expectedModCount || size != itrSize)
                  throw new ConcurrentModificationException();
          }
      }


 >> (at the expense of an
>> extra field, but that might pack into a spare 4 bytes due to alignment
>> at least on 64 bits IIUC).
>
> Or call checkForComodification() when cursor == size in hasNext()?


I'm not sure that we'd want hasNext to possibly throw a CME. I think it 
best that next do that, if needed.

-Chris.

> -- daniel
>
>>
>> Paul.
>>
>>> Rémi
>>> tip: the bug lies in ArrayList.Itr.hasNext() (and
>>> AbstractList.Itr.hasNext()).
>>>
>>
>