From 7c094b3e0710bef7c69040d2d544f4e8781eaa4a Mon Sep 17 00:00:00 2001 From: Patrick Tasse Date: Wed, 26 Oct 2016 14:20:33 -0400 Subject: [PATCH] tmf: Bug 496504: Fix duplicate child entries in Control Flow view This could happen while building the state system if a child process is first discovered in an early iteration and its parent process is found in a second iteration. Removing the child from its old parent trace entry would break the link to the new parent that was just set. On a third iteration it would then add the child to the same parent again. The TimeGraphEntry is changed to be more robust in its handling of the parent-child relations. Change-Id: I99743d80484d335180459df144067fc4494b8052 Signed-off-by: Patrick Tasse Reviewed-on: https://git.eclipse.org/r/83961 Reviewed-by: Hudson CI Reviewed-by: Matthew Khouzam --- .../ui/views/controlflow/ControlFlowView.java | 2 +- .../timegraph/model/TimeGraphEntryTest.java | 152 ++++++++++++++++++ .../timegraph/model/TimeGraphEntry.java | 15 +- 3 files changed, 164 insertions(+), 5 deletions(-) diff --git a/analysis/org.eclipse.tracecompass.analysis.os.linux.ui/src/org/eclipse/tracecompass/internal/analysis/os/linux/ui/views/controlflow/ControlFlowView.java b/analysis/org.eclipse.tracecompass.analysis.os.linux.ui/src/org/eclipse/tracecompass/internal/analysis/os/linux/ui/views/controlflow/ControlFlowView.java index 2531018336..c135bef5fc 100644 --- a/analysis/org.eclipse.tracecompass.analysis.os.linux.ui/src/org/eclipse/tracecompass/internal/analysis/os/linux/ui/views/controlflow/ControlFlowView.java +++ b/analysis/org.eclipse.tracecompass.analysis.os.linux.ui/src/org/eclipse/tracecompass/internal/analysis/os/linux/ui/views/controlflow/ControlFlowView.java @@ -806,11 +806,11 @@ public class ControlFlowView extends AbstractStateSystemTimeGraphView { if (parent.getThreadId() == entry.getParentThreadId() && !(entry.getStartTime() > parent.getEndTime() || entry.getEndTime() < parent.getStartTime())) { - parent.addChild(entry); root = false; if (rootList.contains(entry)) { traceEntry.removeChild(entry); } + parent.addChild(entry); break; } } diff --git a/tmf/org.eclipse.tracecompass.tmf.ui.tests/src/org/eclipse/tracecompass/tmf/ui/tests/widgets/timegraph/model/TimeGraphEntryTest.java b/tmf/org.eclipse.tracecompass.tmf.ui.tests/src/org/eclipse/tracecompass/tmf/ui/tests/widgets/timegraph/model/TimeGraphEntryTest.java index d8d056d3e4..0b8bd3c8ba 100644 --- a/tmf/org.eclipse.tracecompass.tmf.ui.tests/src/org/eclipse/tracecompass/tmf/ui/tests/widgets/timegraph/model/TimeGraphEntryTest.java +++ b/tmf/org.eclipse.tracecompass.tmf.ui.tests/src/org/eclipse/tracecompass/tmf/ui/tests/widgets/timegraph/model/TimeGraphEntryTest.java @@ -16,6 +16,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import java.util.Arrays; +import java.util.Comparator; import java.util.Iterator; import org.eclipse.swt.SWT; @@ -227,6 +229,156 @@ public class TimeGraphEntryTest { assertIteratorsEqual(Iterators.forArray(event1s, event2a, event2b, event3s), entry.getTimeEventsIterator()); } + /** + * Test method addChild. + */ + @Test + public void testAddChild() { + TimeGraphEntry parent = new TimeGraphEntry("parent", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child = new TimeGraphEntry("child", SWT.DEFAULT, SWT.DEFAULT); + parent.addChild(child); + assertEquals(null, parent.getParent()); + assertEquals(Arrays.asList(child), parent.getChildren()); + assertEquals(parent, child.getParent()); + assertEquals(Arrays.asList(), child.getChildren()); + } + + /** + * Test method addChild at specific position. + */ + @Test + public void testAddChildAtPosition() { + TimeGraphEntry parent = new TimeGraphEntry("parent", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child1 = new TimeGraphEntry("child1", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child2 = new TimeGraphEntry("child2", SWT.DEFAULT, SWT.DEFAULT); + parent.addChild(0, child1); + parent.addChild(0, child2); + assertEquals(null, parent.getParent()); + assertEquals(Arrays.asList(child2, child1), parent.getChildren()); + assertEquals(parent, child1.getParent()); + assertEquals(Arrays.asList(), child1.getChildren()); + assertEquals(parent, child2.getParent()); + assertEquals(Arrays.asList(), child2.getChildren()); + } + + /** + * Test method addChild to same parent. + */ + @Test + public void testAddChildSameParent() { + TimeGraphEntry parent = new TimeGraphEntry("parent", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child = new TimeGraphEntry("child", SWT.DEFAULT, SWT.DEFAULT); + parent.addChild(child); + parent.addChild(child); + assertEquals(null, parent.getParent()); + assertEquals(Arrays.asList(child), parent.getChildren()); + assertEquals(parent, child.getParent()); + assertEquals(Arrays.asList(), child.getChildren()); + } + + /** + * Test method addChild to an other parent. + */ + @Test + public void testAddChildOtherParent() { + TimeGraphEntry parent1 = new TimeGraphEntry("parent1", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry parent2 = new TimeGraphEntry("parent2", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child = new TimeGraphEntry("child", SWT.DEFAULT, SWT.DEFAULT); + parent1.addChild(child); + parent2.addChild(child); + assertEquals(null, parent1.getParent()); + assertEquals(Arrays.asList(), parent1.getChildren()); + assertEquals(null, parent2.getParent()); + assertEquals(Arrays.asList(child), parent2.getChildren()); + assertEquals(parent2, child.getParent()); + assertEquals(Arrays.asList(), child.getChildren()); + } + + /** + * Test method removeChild. + */ + @Test + public void testRemoveChild() { + TimeGraphEntry parent = new TimeGraphEntry("parent", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child = new TimeGraphEntry("child", SWT.DEFAULT, SWT.DEFAULT); + parent.addChild(child); + parent.removeChild(child); + assertEquals(null, parent.getParent()); + assertEquals(Arrays.asList(), parent.getChildren()); + assertEquals(null, child.getParent()); + assertEquals(Arrays.asList(), child.getChildren()); + } + + /** + * Test method removeChild with child that has no parent. + */ + @Test + public void testRemoveChildNoParent() { + TimeGraphEntry parent = new TimeGraphEntry("parent", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child = new TimeGraphEntry("child", SWT.DEFAULT, SWT.DEFAULT); + parent.removeChild(child); + assertEquals(null, parent.getParent()); + assertEquals(Arrays.asList(), parent.getChildren()); + assertEquals(null, child.getParent()); + assertEquals(Arrays.asList(), child.getChildren()); + } + + /** + * Test method removeChild with child that has an other parent. + */ + @Test + public void testRemoveChildOtherParent() { + TimeGraphEntry parent1 = new TimeGraphEntry("parent1", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry parent2 = new TimeGraphEntry("parent2", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child = new TimeGraphEntry("child", SWT.DEFAULT, SWT.DEFAULT); + parent1.addChild(child); + parent2.removeChild(child); + assertEquals(null, parent1.getParent()); + assertEquals(Arrays.asList(child), parent1.getChildren()); + assertEquals(null, parent2.getParent()); + assertEquals(Arrays.asList(), parent2.getChildren()); + assertEquals(parent1, child.getParent()); + assertEquals(Arrays.asList(), child.getChildren()); + } + + /** + * Test method sortChildren. + */ + @Test + public void testSortChildren() { + TimeGraphEntry parent = new TimeGraphEntry("parent", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child1 = new TimeGraphEntry("child1", 2, 2); + TimeGraphEntry child2 = new TimeGraphEntry("child2", 1, 1); + parent.addChild(child1); + parent.addChild(child2); + parent.sortChildren(Comparator.comparingLong(entry -> entry.getStartTime())); + assertEquals(null, parent.getParent()); + assertEquals(Arrays.asList(child2, child1), parent.getChildren()); + assertEquals(parent, child1.getParent()); + assertEquals(Arrays.asList(), child1.getChildren()); + assertEquals(parent, child2.getParent()); + assertEquals(Arrays.asList(), child2.getChildren()); + } + + /** + * Test method sortChildren followed by addChild. + */ + @Test + public void testSortChildrenThenAdd() { + TimeGraphEntry parent = new TimeGraphEntry("parent", SWT.DEFAULT, SWT.DEFAULT); + TimeGraphEntry child1 = new TimeGraphEntry("child1", 2, 2); + TimeGraphEntry child2 = new TimeGraphEntry("child2", 1, 1); + parent.sortChildren(Comparator.comparingLong(entry -> entry.getStartTime())); + parent.addChild(child1); + parent.addChild(child2); + assertEquals(null, parent.getParent()); + assertEquals(Arrays.asList(child2, child1), parent.getChildren()); + assertEquals(parent, child1.getParent()); + assertEquals(Arrays.asList(), child1.getChildren()); + assertEquals(parent, child2.getParent()); + assertEquals(Arrays.asList(), child2.getChildren()); + } + private static void assertIteratorsEqual(Iterator expected, Iterator actual) { int i = 0; while (expected.hasNext()) { diff --git a/tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/model/TimeGraphEntry.java b/tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/model/TimeGraphEntry.java index 8f36276a5e..5d06e92212 100644 --- a/tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/model/TimeGraphEntry.java +++ b/tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/model/TimeGraphEntry.java @@ -262,9 +262,8 @@ public class TimeGraphEntry implements ITimeGraphEntry { * The child entry */ public synchronized void addChild(@NonNull TimeGraphEntry child) { - child.setParent(this); if (fComparator == null) { - fChildren.add(child); + addChild(fChildren.size(), child); } else { int i; for (i = 0; i < fChildren.size(); i++) { @@ -273,7 +272,7 @@ public class TimeGraphEntry implements ITimeGraphEntry { break; } } - fChildren.add(i, child); + addChild(i, child); } } @@ -287,6 +286,12 @@ public class TimeGraphEntry implements ITimeGraphEntry { * @since 2.0 */ public synchronized void addChild(int index, @NonNull TimeGraphEntry child) { + if (child.getParent() == this) { + return; + } + if (child.getParent() != null) { + child.getParent().removeChild(child); + } child.setParent(this); fChildren.add(index, child); } @@ -299,7 +304,9 @@ public class TimeGraphEntry implements ITimeGraphEntry { * @since 2.0 */ public synchronized void removeChild(@NonNull TimeGraphEntry child) { - child.setParent(null); + if (child.getParent() == this) { + child.setParent(null); + } fChildren.remove(child); } -- 2.34.1