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 <patrick.tasse@gmail.com>
Reviewed-on: https://git.eclipse.org/r/83961
Reviewed-by: Hudson CI
Reviewed-by: Matthew Khouzam <matthew.khouzam@ericsson.com>

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 25310183360aba71ab1ebb8b68963cf345907eee..c135bef5fc3aca19d0355f855cb784c1e86e5884 100644 (file)
--- 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 d8d056d3e4476e26452b26e688a1404f6febfb1b..0b8bd3c8bad9611ea52edeec95d76275bc4b23b2 100644 (file)
--- 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<ITimeEvent> expected, Iterator<ITimeEvent> 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 8f36276a5ef45b8f47e970872b4e649b308b8abc..5d06e922121d4da7b2e07eda1199da8dea7fdb77 100644 (file)
--- 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);
     }