Skip to content

Commit 4b24c86

Browse files
Lukasz Kostyrahjohn
Lukasz Kostyra
authored and
hjohn
committed
8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet
Reviewed-by: jhendrikx, kcr
1 parent 2f5dcfd commit 4b24c86

File tree

3 files changed

+110
-0
lines changed

3 files changed

+110
-0
lines changed

‎modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.util.Collection;
3030
import java.util.Comparator;
3131
import java.util.List;
32+
import java.util.Objects;
3233
import java.util.RandomAccess;
3334

3435
import com.sun.javafx.collections.NonIterableChange.SimplePermutationChange;
@@ -94,6 +95,7 @@ public int size() {
9495

9596
@Override
9697
protected void doAdd(int index, E element) {
98+
Objects.checkIndex(index, size() + 1);
9799
if (elementObserver != null)
98100
elementObserver.attachListener(element);
99101
backingList.add(index, element);
@@ -158,6 +160,7 @@ public void clear() {
158160

159161
@Override
160162
public void remove(int fromIndex, int toIndex) {
163+
Objects.checkFromToIndex(fromIndex, toIndex, size());
161164
beginChange();
162165
for (int i = fromIndex; i < toIndex; ++i) {
163166
remove(fromIndex);

‎modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333
import java.util.Iterator;
3434
import java.util.List;
3535
import java.util.ListIterator;
36+
import java.util.Objects;
37+
3638
import javafx.beans.InvalidationListener;
3739
import javafx.collections.ListChangeListener;
3840
import javafx.collections.ObservableList;
@@ -110,6 +112,7 @@ public boolean setAll(E... elements) {
110112

111113
@Override
112114
public boolean setAll(Collection<? extends E> col) {
115+
Objects.requireNonNull(col);
113116
onProposedChange(Collections.unmodifiableList(new ArrayList<>(col)), 0, size());
114117
try {
115118
modCount++;
@@ -161,6 +164,7 @@ public boolean retainAll(E... elements) {
161164

162165
@Override
163166
public void remove(int from, int to) {
167+
Objects.checkFromToIndex(from, to, size());
164168
onProposedChange(Collections.<E>emptyList(), from, to);
165169
try {
166170
modCount++;
@@ -230,6 +234,7 @@ public boolean containsAll(Collection<?> c) {
230234

231235
@Override
232236
public boolean addAll(Collection<? extends E> c) {
237+
Objects.requireNonNull(c);
233238
onProposedChange(Collections.unmodifiableList(new ArrayList<>(c)), size(), size());
234239
try {
235240
modCount++;
@@ -245,6 +250,8 @@ public boolean addAll(Collection<? extends E> c) {
245250

246251
@Override
247252
public boolean addAll(int index, Collection<? extends E> c) {
253+
Objects.requireNonNull(c);
254+
Objects.checkIndex(index, size() + 1);
248255
onProposedChange(Collections.unmodifiableList(new ArrayList<>(c)), index, index);
249256
try {
250257
modCount++;
@@ -260,6 +267,7 @@ public boolean addAll(int index, Collection<? extends E> c) {
260267

261268
@Override
262269
public boolean removeAll(Collection<?> c) {
270+
Objects.requireNonNull(c);
263271
removeFromList(this, 0, c, false);
264272
try {
265273
modCount++;
@@ -275,6 +283,7 @@ public boolean removeAll(Collection<?> c) {
275283

276284
@Override
277285
public boolean retainAll(Collection<?> c) {
286+
Objects.requireNonNull(c);
278287
removeFromList(this, 0, c, true);
279288
try {
280289
modCount++;
@@ -313,6 +322,7 @@ public E set(int index, E element) {
313322

314323
@Override
315324
public void add(int index, E element) {
325+
Objects.checkIndex(index, size() + 1);
316326
onProposedChange(Collections.singletonList(element), index, index);
317327
try {
318328
modCount++;
@@ -325,6 +335,7 @@ public void add(int index, E element) {
325335

326336
@Override
327337
public E remove(int index) {
338+
Objects.checkIndex(index, size());
328339
onProposedChange(Collections.<E>emptyList(), index, index + 1);
329340
try {
330341
modCount++;
@@ -460,6 +471,7 @@ public boolean containsAll(Collection<?> c) {
460471

461472
@Override
462473
public boolean addAll(Collection<? extends E> c) {
474+
Objects.requireNonNull(c);
463475
checkForComodification();
464476
onProposedChange(Collections.unmodifiableList(new ArrayList<>(c)), offset + size(), offset + size());
465477
try {
@@ -476,6 +488,8 @@ public boolean addAll(Collection<? extends E> c) {
476488

477489
@Override
478490
public boolean addAll(int index, Collection<? extends E> c) {
491+
Objects.requireNonNull(c);
492+
Objects.checkIndex(index, size() + 1);
479493
checkForComodification();
480494
onProposedChange(Collections.unmodifiableList(new ArrayList<>(c)), offset + index, offset + index);
481495
try {
@@ -492,6 +506,7 @@ public boolean addAll(int index, Collection<? extends E> c) {
492506

493507
@Override
494508
public boolean removeAll(Collection<?> c) {
509+
Objects.requireNonNull(c);
495510
checkForComodification();
496511
removeFromList(this, offset, c, false);
497512
try {
@@ -508,6 +523,7 @@ public boolean removeAll(Collection<?> c) {
508523

509524
@Override
510525
public boolean retainAll(Collection<?> c) {
526+
Objects.requireNonNull(c);
511527
checkForComodification();
512528
removeFromList(this, offset, c, true);
513529
try {
@@ -550,6 +566,7 @@ public E set(int index, E element) {
550566

551567
@Override
552568
public void add(int index, E element) {
569+
Objects.checkIndex(index, size() + 1);
553570
checkForComodification();
554571
onProposedChange(Collections.singletonList(element), offset + index, offset + index);
555572
try {
@@ -563,6 +580,7 @@ public void add(int index, E element) {
563580

564581
@Override
565582
public E remove(int index) {
583+
Objects.checkIndex(index, size());
566584
checkForComodification();
567585
onProposedChange(Collections.<E>emptyList(), offset + index, offset + index + 1);
568586
try {

‎modules/javafx.graphics/src/test/java/test/javafx/scene/ParentTest.java‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131
import com.sun.javafx.tk.Toolkit;
3232
import com.sun.javafx.geom.PickRay;
3333
import com.sun.javafx.scene.input.PickResultChooser;
34+
35+
import java.util.Collection;
3436
import java.util.concurrent.atomic.AtomicBoolean;
3537
import javafx.scene.Group;
3638
import javafx.scene.GroupShim;
@@ -838,6 +840,93 @@ public void testPickingChildNodeWithDirtyViewOrder() {
838840
assertNull(res.getIntersectedNode());
839841
}
840842

843+
@Test
844+
public void testNegativeIndex_Add() {
845+
Group g = new Group();
846+
g.getChildren().addAll(new Rectangle(), new Rectangle());
847+
848+
int preTestSize = g.getChildren().size();
849+
850+
// Adding an object at a negative or too big index should throw IndexOutOfBoundsException and not modify internal state
851+
assertThrows(IndexOutOfBoundsException.class, () -> g.getChildren().add(-1, new Rectangle()));
852+
assertEquals(preTestSize, g.getChildren().size());
853+
854+
assertThrows(IndexOutOfBoundsException.class, () -> g.getChildren().add(g.getChildren().size() + 1, new Rectangle()));
855+
assertEquals(preTestSize, g.getChildren().size());
856+
857+
// below call should throw no exception - if it does, internal state is corrupted
858+
g.getChildren().remove(0);
859+
}
860+
861+
@Test
862+
public void testNegativeIndex_Set() {
863+
Group g = new Group();
864+
g.getChildren().addAll(new Rectangle(), new Rectangle());
865+
866+
int preTestSize = g.getChildren().size();
867+
868+
// Setting an object at a negative or too big index should throw IndexOutOfBoundsException and not modify internal state
869+
assertThrows(IndexOutOfBoundsException.class, () -> g.getChildren().set(-1, new Rectangle()));
870+
assertEquals(preTestSize, g.getChildren().size());
871+
872+
assertThrows(IndexOutOfBoundsException.class, () -> g.getChildren().set(g.getChildren().size(), new Rectangle()));
873+
assertEquals(preTestSize, g.getChildren().size());
874+
875+
// below call should throw no exception - if it does, internal state is corrupted
876+
g.getChildren().remove(0);
877+
}
878+
879+
@Test
880+
public void testNegativeIndex_Remove() {
881+
Group g = new Group();
882+
g.getChildren().addAll(new Rectangle(), new Rectangle());
883+
884+
int preTestSize = g.getChildren().size();
885+
886+
// Removing an object at negative or too big index should throw IndexOutOfBoundsException and not modify internal state
887+
assertThrows(IndexOutOfBoundsException.class, () -> g.getChildren().remove(-1));
888+
assertEquals(preTestSize, g.getChildren().size());
889+
890+
assertThrows(IndexOutOfBoundsException.class, () -> g.getChildren().remove(g.getChildren().size()));
891+
assertEquals(preTestSize, g.getChildren().size());
892+
893+
// below call should throw no exception - if it does, internal state is corrupted
894+
g.getChildren().remove(0);
895+
}
896+
897+
@Test
898+
public void testNullObject_AddAll() {
899+
Group g = new Group();
900+
g.getChildren().addAll(new Rectangle(), new Rectangle());
901+
902+
int preTestSize = g.getChildren().size();
903+
904+
// Adding a null object should throw NPE and not modify internal state
905+
assertThrows(NullPointerException.class, () -> g.getChildren().addAll((Collection<Node>)null));
906+
assertEquals(preTestSize, g.getChildren().size());
907+
908+
assertThrows(NullPointerException.class, () -> g.getChildren().addAll(0, (Collection<Node>)null));
909+
assertEquals(preTestSize, g.getChildren().size());
910+
911+
// below call should throw no exception - if it does, internal state is corrupted
912+
g.getChildren().remove(0);
913+
}
914+
915+
@Test
916+
public void testNullObject_SetAll() {
917+
Group g = new Group();
918+
g.getChildren().addAll(new Rectangle(), new Rectangle());
919+
920+
int preTestSize = g.getChildren().size();
921+
922+
// Setting a null object should throw NPE and not modify internal state
923+
assertThrows(NullPointerException.class, () -> g.getChildren().setAll((Collection<Node>)null));
924+
assertEquals(preTestSize, g.getChildren().size());
925+
926+
// below call should throw no exception - if it does, internal state is corrupted
927+
g.getChildren().remove(0);
928+
}
929+
841930
public static class MockParent extends Parent {
842931
public MockParent(Node... children) {
843932
ParentShim.getChildren(this).addAll(children);

0 commit comments

Comments
 (0)