diff options
author | Eike Stepper | 2014-03-06 09:37:33 +0000 |
---|---|---|
committer | Eike Stepper | 2014-03-06 09:37:33 +0000 |
commit | 1f240cd4cc3ab3e1386c6bbbc38188d6e5519a88 (patch) | |
tree | 8cb01d0008b1ed2bad4f210640b4bb21d7c0c953 | |
parent | e5d1e7a5b08aede59f1e9fcbb77eea3ac81a2a52 (diff) | |
download | cdo-1f240cd4cc3ab3e1386c6bbbc38188d6e5519a88.tar.gz cdo-1f240cd4cc3ab3e1386c6bbbc38188d6e5519a88.tar.xz cdo-1f240cd4cc3ab3e1386c6bbbc38188d6e5519a88.zip |
[429709] Setting list values by index twice causes DanglingReferenceException
https://bugs.eclipse.org/bugs/show_bug.cgi?id=429709
2 files changed, 368 insertions, 128 deletions
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/delta/CDOListFeatureDeltaImpl.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/delta/CDOListFeatureDeltaImpl.java index 54a2e0e4c3..58338a1841 100644 --- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/delta/CDOListFeatureDeltaImpl.java +++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/delta/CDOListFeatureDeltaImpl.java @@ -23,6 +23,7 @@ import org.eclipse.emf.cdo.common.revision.delta.CDOMoveFeatureDelta; import org.eclipse.emf.cdo.common.revision.delta.CDORemoveFeatureDelta; import org.eclipse.emf.cdo.common.revision.delta.CDOSetFeatureDelta; import org.eclipse.emf.cdo.spi.common.revision.CDOReferenceAdjuster; +import org.eclipse.emf.cdo.spi.common.revision.InternalCDOFeatureDelta; import org.eclipse.net4j.util.ObjectUtil; import org.eclipse.net4j.util.collection.Pair; @@ -185,7 +186,7 @@ public class CDOListFeatureDeltaImpl extends CDOFeatureDeltaImpl implements CDOL cachedSources = new ListTargetAdding[initialCapacity]; } else - // i.e. unprocessedFeatureDeltas != null + // i.e. unprocessedFeatureDeltas != null { int requiredCapacity = 1 + cachedIndices[0] + unprocessedFeatureDeltas.size(); if (cachedIndices.length < requiredCapacity) @@ -226,176 +227,323 @@ public class CDOListFeatureDeltaImpl extends CDOFeatureDeltaImpl implements CDOL private boolean cleanupWithNewDelta(CDOFeatureDelta featureDelta) { EStructuralFeature feature = getFeature(); - if ((feature instanceof EReference || FeatureMapUtil.isFeatureMap(feature)) - && featureDelta instanceof CDORemoveFeatureDelta) + switch (featureDelta.getType()) { - int indexToRemove = ((CDORemoveFeatureDelta)featureDelta).getIndex(); - reconstructAddedIndicesWithNoCopy(); - - for (int i = 1; i <= cachedIndices[0]; i++) + case REMOVE: + { + if (feature instanceof EReference || FeatureMapUtil.isFeatureMap(feature)) { - int index = cachedIndices[i]; - if (indexToRemove == index) + Boolean result = cleanupWithNewRemoveDelta((CDORemoveFeatureDelta)featureDelta); + if (result != null) { - // The previous implementation set the value of the feature delta to CDOID.NULL. Databinding and probably - // others don't really like it. We now remove the ADD (or SET which seems to appear in CDOListFeatureDelta - // during opposite adjustment!? Why???) and patch the other feature deltas. - // See https://bugs.eclipse.org/bugs/show_bug.cgi?id=310574 + return result; + } + } - ListTargetAdding delta = cachedSources[i]; + break; + } + case SET: + { + Boolean result = cleanupWithNewSetDelta((CDOSetFeatureDelta)featureDelta); + if (result != null) + { + return result; + } - // We use a "floating" index which is the index (in the list) of the item to remove at the time when the - // object was still in the list. This index evolves with the feature deltas. - int floatingIndex = delta.getIndex(); + break; + } + } - // First updates cachedSources and cachedIndices using CDORemoveFeatureDelta. - ListIndexAffecting affecting = (ListIndexAffecting)featureDelta; - affecting.affectIndices(cachedSources, cachedIndices); + if (cachedIndices != null) + { + if (unprocessedFeatureDeltas == null) + { + unprocessedFeatureDeltas = new ArrayList<CDOFeatureDelta>(); + } - // Then adjusts the remaining feature deltas. - boolean skip = true; - ListIterator<CDOFeatureDelta> iterator = listChanges.listIterator(); + unprocessedFeatureDeltas.add(featureDelta); + } - while (iterator.hasNext()) - { - CDOFeatureDelta fd = iterator.next(); + return true; + } - // We only need to process feature deltas that come after the ADD (or SET) to be removed. - if (skip) - { - if (fd == delta) - { - // Found the ADD (or SET) feature delta that we need to remove. So remove it from the list and start - // processing the next feature deltas. - skip = false; - iterator.remove(); + private Boolean cleanupWithNewRemoveDelta(CDORemoveFeatureDelta removeFeatureDelta) + { + int indexToRemove = removeFeatureDelta.getIndex(); + reconstructAddedIndicesWithNoCopy(); - // SET - if (fd instanceof CDOSetFeatureDelta) - { - // If the removed delta is SET we add the REMOVE to the feature deltas. We do not need to adjust the - // other feature deltas because SET do not modify the list. - return true; - } - } + for (int i = 1; i <= cachedIndices[0]; i++) + { + int index = cachedIndices[i]; + if (indexToRemove == index) + { + // The previous implementation set the value of the feature delta to CDOID.NULL. Databinding and probably + // others don't really like it. We now remove the ADD (or SET which seems to appear in CDOListFeatureDelta + // during opposite adjustment!? Why???) and patch the other feature deltas. + // See https://bugs.eclipse.org/bugs/show_bug.cgi?id=310574 - continue; - } + ListTargetAdding delta = cachedSources[i]; + + // We use a "floating" index which is the index (in the list) of the item to remove at the time when the + // object was still in the list. This index evolves with the feature deltas. + int floatingIndex = delta.getIndex(); + + // First updates cachedSources and cachedIndices using CDORemoveFeatureDelta. + ListIndexAffecting affecting = (ListIndexAffecting)removeFeatureDelta; + affecting.affectIndices(cachedSources, cachedIndices); + + // Then adjusts the remaining feature deltas. + boolean skip = true; + ListIterator<CDOFeatureDelta> iterator = listChanges.listIterator(); - // ADD - if (fd instanceof CDOAddFeatureDelta) + while (iterator.hasNext()) + { + CDOFeatureDelta fd = iterator.next(); + + // We only need to process feature deltas that come after the ADD (or SET) to be removed. + if (skip) + { + if (fd == delta) { - // Increases the floating index if the ADD came in front of the item. - if (((CDOAddFeatureDelta)fd).getIndex() <= floatingIndex) + // Found the ADD (or SET) feature delta that we need to remove. So remove it from the list and start + // processing the next feature deltas. + skip = false; + iterator.remove(); + + // SET + if (fd instanceof CDOSetFeatureDelta) { - ++floatingIndex; + // If the removed delta is SET we add the REMOVE to the feature deltas. We do not need to adjust the + // other feature deltas because SET do not modify the list. + return true; } - - // Adjusts the feature delta too. - ((WithIndex)fd).adjustAfterRemoval(floatingIndex); } - // REMOVE - else if (fd instanceof CDORemoveFeatureDelta) + continue; + } + + // ADD + if (fd instanceof CDOAddFeatureDelta) + { + // Increases the floating index if the ADD came in front of the item. + if (((CDOAddFeatureDelta)fd).getIndex() <= floatingIndex) { - int idx = floatingIndex; - // Decreases the floating index if the REMOVE came in front of the item. - if (((CDORemoveFeatureDelta)fd).getIndex() <= floatingIndex) - { - --floatingIndex; - } + ++floatingIndex; + } - // Adjusts the feature delta too. - ((WithIndex)fd).adjustAfterRemoval(idx); + // Adjusts the feature delta too. + ((WithIndex)fd).adjustAfterRemoval(floatingIndex); + } + + // REMOVE + else if (fd instanceof CDORemoveFeatureDelta) + { + int idx = floatingIndex; + // Decreases the floating index if the REMOVE came in front of the item. + if (((CDORemoveFeatureDelta)fd).getIndex() <= floatingIndex) + { + --floatingIndex; } - // MOVE - else if (fd instanceof CDOMoveFeatureDelta) + // Adjusts the feature delta too. + ((WithIndex)fd).adjustAfterRemoval(idx); + } + + // MOVE + else if (fd instanceof CDOMoveFeatureDelta) + { + // Remembers the positions before we patch them. + int from = ((CDOMoveFeatureDelta)fd).getOldPosition(); + int to = ((CDOMoveFeatureDelta)fd).getNewPosition(); + + if (floatingIndex == from) + { + // We are moving the "to be deleted" item. So we update our floating index and remove the MOVE. It has + // no effect on the list. + floatingIndex = to; + iterator.remove(); + } + else { - // Remembers the positions before we patch them. - int from = ((CDOMoveFeatureDelta)fd).getOldPosition(); - int to = ((CDOMoveFeatureDelta)fd).getNewPosition(); + // In the other cases, we need to patch the positions. + + // If the old position is greater or equal to the current position of the item to be removed (remember, + // that's our floating index), decrease the position. + int patchedFrom = floatingIndex <= from ? from - 1 : from; + + // The new position requires more care. We need to know the direction of the move (left-to-right or + // right-to-left). + int patchedTo; + if (from > to) + { + // left-to-right. Only decreases the position if it is strictly greater than the current item + // position. + patchedTo = floatingIndex < to ? to - 1 : to; + } + else + { + // right-to-left. Decreases the position if it is greater or equal than the current item position. + patchedTo = floatingIndex <= to ? to - 1 : to; + } + + // We can now update our floating index. We use the original positions because the floating index + // represents the item "to be deleted" before it was actually removed. + if (from < floatingIndex && floatingIndex <= to) + { + --floatingIndex; + } + else if (to <= floatingIndex && floatingIndex < from) + { + ++floatingIndex; + } - if (floatingIndex == from) + // And finally adjust the feature delta. + if (patchedFrom == patchedTo) { - // We are moving the "to be deleted" item. So we update our floating index and remove the MOVE. It has - // no effect on the list. - floatingIndex = to; + // Source and destination are the same so just remove the feature delta. iterator.remove(); } else { - // In the other cases, we need to patch the positions. - - // If the old position is greater or equal to the current position of the item to be removed (remember, - // that's our floating index), decrease the position. - int patchedFrom = floatingIndex <= from ? from - 1 : from; - - // The new position requires more care. We need to know the direction of the move (left-to-right or - // right-to-left). - int patchedTo; - if (from > to) - { - // left-to-right. Only decreases the position if it is strictly greater than the current item - // position. - patchedTo = floatingIndex < to ? to - 1 : to; - } - else - { - // right-to-left. Decreases the position if it is greater or equal than the current item position. - patchedTo = floatingIndex <= to ? to - 1 : to; - } - - // We can now update our floating index. We use the original positions because the floating index - // represents the item "to be deleted" before it was actually removed. - if (from < floatingIndex && floatingIndex <= to) - { - --floatingIndex; - } - else if (to <= floatingIndex && floatingIndex < from) - { - ++floatingIndex; - } - - // And finally adjust the feature delta. - if (patchedFrom == patchedTo) - { - // Source and destination are the same so just remove the feature delta. - iterator.remove(); - } - else - { - ((CDOMoveFeatureDeltaImpl)fd).setOldPosition(patchedFrom); - ((CDOMoveFeatureDeltaImpl)fd).setNewPosition(patchedTo); - } + ((CDOMoveFeatureDeltaImpl)fd).setOldPosition(patchedFrom); + ((CDOMoveFeatureDeltaImpl)fd).setNewPosition(patchedTo); } } + } - // SET - else if (fd instanceof CDOSetFeatureDelta) - { - // Adjusts the feature delta too. - ((WithIndex)fd).adjustAfterRemoval(floatingIndex); - } + // SET + else if (fd instanceof CDOSetFeatureDelta) + { + // Adjusts the feature delta too. + ((WithIndex)fd).adjustAfterRemoval(floatingIndex); } + } - // If the removed delta was ADD so we do not add the REMOVE to the feature deltas. - return false; + // If the removed delta was ADD so we do not add the REMOVE to the feature deltas. + return false; + } + } + + return null; + } + + /** + * A new SET feature delta can interfere with former ADD or SET deltas. + */ + private Boolean cleanupWithNewSetDelta(CDOSetFeatureDelta featureDelta) + { + final class DeltaProxy implements InternalCDOFeatureDelta.WithIndex + { + private final int indexIntoListChanges; + + private int index; + + public DeltaProxy(int indexIntoListChanges, int index) + { + this.indexIntoListChanges = indexIntoListChanges; + this.index = index; + } + + public int getIndexIntoListChanges() + { + return indexIntoListChanges; + } + + public int getIndex() + { + return index; + } + + public void adjustAfterAddition(int index) + { + if (index <= this.index) + { + ++this.index; + } + } + + public void adjustAfterRemoval(int index) + { + if (index < this.index && this.index > 0) + { + --this.index; } } } - if (cachedIndices != null) + int size = listChanges.size(); + List<DeltaProxy> proxies = new ArrayList<DeltaProxy>(); + + for (int i = 0; i < size; i++) { - if (unprocessedFeatureDeltas == null) + CDOFeatureDelta fd = listChanges.get(i); + switch (fd.getType()) { - unprocessedFeatureDeltas = new ArrayList<CDOFeatureDelta>(); + case MOVE: + { + int oldPosition = ((CDOMoveFeatureDelta)fd).getOldPosition(); + int newPosition = ((CDOMoveFeatureDelta)fd).getNewPosition(); + for (DeltaProxy proxy : proxies) + { + proxy.adjustAfterRemoval(oldPosition); + proxy.adjustAfterAddition(newPosition); + } + + break; } - unprocessedFeatureDeltas.add(featureDelta); + case REMOVE: + { + int index = ((CDORemoveFeatureDelta)fd).getIndex(); + for (DeltaProxy proxy : proxies) + { + proxy.adjustAfterRemoval(index); + } + + break; + } + + case ADD: + { + int index = ((CDOAddFeatureDelta)fd).getIndex(); + for (DeltaProxy proxy : proxies) + { + proxy.adjustAfterAddition(index); + } + + proxies.add(new DeltaProxy(i, index)); + break; + } + + case SET: + { + int index = ((CDOSetFeatureDelta)fd).getIndex(); + proxies.add(new DeltaProxy(i, index)); + break; + } + } } - return true; + int index = featureDelta.getIndex(); + for (DeltaProxy proxy : proxies) + { + if (proxy.getIndex() == index) + { + int indexIntoListChanges = proxy.getIndexIntoListChanges(); + CDOFeatureDelta featureDeltaToModify = listChanges.get(indexIntoListChanges); + if (featureDeltaToModify.getType() == CDOFeatureDelta.Type.ADD) + { + ((CDOAddFeatureDeltaImpl)featureDeltaToModify).setValue(featureDelta.getValue()); + return false; + } + + // Here featureDeltaToModify is a SET delta + listChanges.remove(indexIntoListChanges); // Replace the SET delta that existed for this index + break; + } + } + + return null; } public void add(CDOFeatureDelta featureDelta) diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_429709_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_429709_Test.java new file mode 100644 index 0000000000..214051b754 --- /dev/null +++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_429709_Test.java @@ -0,0 +1,92 @@ +/* + * Copyright (c) 2014 Eike Stepper (Berlin, Germany) and others. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Eike Stepper - initial API and implementation + */ +package org.eclipse.emf.cdo.tests.bugzilla; + +import org.eclipse.emf.cdo.eresource.CDOResource; +import org.eclipse.emf.cdo.session.CDOSession; +import org.eclipse.emf.cdo.tests.AbstractCDOTest; +import org.eclipse.emf.cdo.tests.model1.Company; +import org.eclipse.emf.cdo.tests.model1.Supplier; +import org.eclipse.emf.cdo.transaction.CDOTransaction; + +import org.eclipse.emf.common.util.EList; + +/** + * Bug 429709 - Setting list values by index twice causes DanglingReferenceException. + * + * @author Eike Stepper + */ +public class Bugzilla_429709_Test extends AbstractCDOTest +{ + public void testContainmentSetSet() throws Exception + { + Supplier s0 = getModel1Factory().createSupplier(); + Supplier s1 = getModel1Factory().createSupplier(); + Supplier s2 = getModel1Factory().createSupplier(); + + Company company = getModel1Factory().createCompany(); + EList<Supplier> suppliers = company.getSuppliers(); + suppliers.add(s0); + + CDOSession session = openSession(); + CDOTransaction transaction = session.openTransaction(); + CDOResource resource = transaction.createResource(getResourcePath("/test1")); + + resource.getContents().add(company); + transaction.commit(); + + suppliers.set(0, s1); + suppliers.set(0, s2); + transaction.commit(); + } + + public void testContainmentAddSet() throws Exception + { + Supplier s0 = getModel1Factory().createSupplier(); + Supplier s1 = getModel1Factory().createSupplier(); + Supplier s2 = getModel1Factory().createSupplier(); + + Company company = getModel1Factory().createCompany(); + EList<Supplier> suppliers = company.getSuppliers(); + suppliers.add(s0); + + CDOSession session = openSession(); + CDOTransaction transaction = session.openTransaction(); + CDOResource resource = transaction.createResource(getResourcePath("/test1")); + + resource.getContents().add(company); + transaction.commit(); + + suppliers.add(s1); + suppliers.set(1, s2); + transaction.commit(); + } + + public void testContainmentAddAddSet() throws Exception + { + Company company = getModel1Factory().createCompany(); + EList<Supplier> suppliers = company.getSuppliers(); + suppliers.add(getModel1Factory().createSupplier()); + + CDOSession session = openSession(); + CDOTransaction transaction = session.openTransaction(); + CDOResource resource = transaction.createResource(getResourcePath("/test1")); + + resource.getContents().add(company); + transaction.commit(); + + suppliers.add(0, getModel1Factory().createSupplier()); // This supplier will end up at index 1 + suppliers.add(0, getModel1Factory().createSupplier()); + + suppliers.set(1, getModel1Factory().createSupplier()); // Replace supplier at index 1 + transaction.commit(); + } +} |