From 22aec774e65a6d14de4acec3a0ceb1fbc49bbaca Mon Sep 17 00:00:00 2001
From: Stephen Colebourne <scolebourne@apache.org>
Date: Sat, 21 Jan 2006 01:49:21 +0000
Subject: [PATCH] TreeList iterator previous() broken as remove(int) could
 break invariants bug 35258, reported by and test case from Tomas D.

git-svn-id: https://svn.apache.org/repos/asf/jakarta/commons/proper/collections/trunk@370952 13f79535-47bb-0310-9956-ffa450edef68
---
 RELEASE-NOTES.html                            |  1 +
 .../commons/collections/list/TreeList.java    |  7 ++++
 .../collections/list/TestTreeList.java        | 35 +++++++++++++++++--
 3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/RELEASE-NOTES.html b/RELEASE-NOTES.html
index 6f87cd7f2..7ee1850cc 100644
--- a/RELEASE-NOTES.html
+++ b/RELEASE-NOTES.html
@@ -87,6 +87,7 @@ If this causes major headaches to anyone please contact commons-dev at jakarta.a
 <li>FastArrayList - Fix iterator remove where ConcurrentModificationException not as expected [34690]</li>
 <li>CursorableLinkedList (list subpackage) - Fix iterator remove/set not throwing IllegalStateException after next-previous-removeByIndex [35766]</li>
 <li>TreeList/CursorableLinkedList/NodeCachingLinkedList/AbstractLinkedList - Fix iterator remove not working properly when called after previous [35258]</li>
+<li>TreeList - remove(int) could break class invariants, breaking iterator previous [35258]</li>
 <li>SetUniqueList.set(int,Object) - Destroyed set status in certain circumstances [33294]</li>
 <li>AbstractLinkedMap.init() - Now calls createEntry() to create the map entry object [33706]</li>
 <li>AbstractHashedMap deserialization - Fix to prevent doubling of internal data array [34265]</li>
diff --git a/src/java/org/apache/commons/collections/list/TreeList.java b/src/java/org/apache/commons/collections/list/TreeList.java
index 69305502e..4fcd65abc 100644
--- a/src/java/org/apache/commons/collections/list/TreeList.java
+++ b/src/java/org/apache/commons/collections/list/TreeList.java
@@ -564,7 +564,14 @@ public class TreeList extends AbstractList {
                 if (rightIsNext) {
                     right = leftMax.right;
                 }
+                AVLNode leftPrevious = left.left;
                 left = left.removeMax();
+                if (left == null) {
+                    // special case where left that was deleted was a double link
+                    // only occurs when height difference is equal
+                    left = leftPrevious;
+                    leftIsPrevious = true;
+                }
                 if (relativePosition > 0) {
                     relativePosition--;
                 }
diff --git a/src/test/org/apache/commons/collections/list/TestTreeList.java b/src/test/org/apache/commons/collections/list/TestTreeList.java
index 23a633503..391489828 100644
--- a/src/test/org/apache/commons/collections/list/TestTreeList.java
+++ b/src/test/org/apache/commons/collections/list/TestTreeList.java
@@ -1,5 +1,5 @@
 /*
- *  Copyright 2004 The Apache Software Foundation
+ *  Copyright 2004,2006 The Apache Software Foundation
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -16,6 +16,7 @@
 package org.apache.commons.collections.list;
 
 import java.util.List;
+import java.util.ListIterator;
 
 import junit.framework.Test;
 
@@ -52,7 +53,6 @@ public class TestTreeList extends AbstractTestList {
     }
 
     public static void benchmark(List l) {
-        StringBuffer sb = new StringBuffer();
         long start = System.currentTimeMillis();
         for (int i = 0; i < 100000; i++) {
             l.add(new Integer(i));
@@ -210,4 +210,35 @@ public class TestTreeList extends AbstractTestList {
 //        l.add("A5");
 //        l.add("A6");
 //    }
+
+    public void testBug35258() {
+        Object objectToRemove = new Integer(3);
+        
+        List treelist = new TreeList();
+        treelist.add(new Integer(0));
+        treelist.add(new Integer(1));
+        treelist.add(new Integer(2));
+        treelist.add(new Integer(3));
+        treelist.add(new Integer(4));
+        
+        // this cause inconsistence of ListIterator()
+        treelist.remove(objectToRemove);
+        
+        ListIterator li = treelist.listIterator();
+        assertEquals(new Integer(0), li.next());
+        assertEquals(new Integer(0), li.previous());
+        assertEquals(new Integer(0), li.next());
+        assertEquals(new Integer(1), li.next());
+        // this caused error in bug 35258
+        assertEquals(new Integer(1), li.previous());
+        assertEquals(new Integer(1), li.next());
+        assertEquals(new Integer(2), li.next());
+        assertEquals(new Integer(2), li.previous());
+        assertEquals(new Integer(2), li.next());
+        assertEquals(new Integer(4), li.next());
+        assertEquals(new Integer(4), li.previous());
+        assertEquals(new Integer(4), li.next());
+        assertEquals(false, li.hasNext());
+    }
+
 }