Description Brian de Alwis 2011-01-31 16:48:12 EST

ModelAssembler#topoSort(), introduced in bug 314761, is used to order model contributions based on their bundle dependency graph.  It was added to ensure that fragments and processors could rely on values defined by other bundles.

I've discovered that I made a mistake in a variable reference in the algorithm implementation.  The algorithm sorts the elements by their # required items, and must periodically re-sort the items.  The check for when to re-sort mistakenly checks the # dependencies rather than the # of requirements.

The bug shows up in a case where I have a chain of bundles (A <- B <- C).  The initial sort, based on the # requirements, ordered [A, C, B], which was correct.  A was correctly removed, but requires(C) > 0, and so the array should be re-sorted.  But the check (below) mistakenly uses depends(C), which being the end of the chain is equals to 0, and so the array isn't re-sorted.

The impact is two-fold:

  1. The contributions may are not sorted in topo order.  This means bug 314761 isn't actually fixed.

  2. There are two assertions using the Java assert keyword in the code, one of which may be triggered because of this bug.

The immediate fix, shown below, is tiny.  I'm running with it currently.

I'd suggest removing the assertions too.

--- bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/ModelAssembler.java	2010-07-20 19:38:00 +0000
+++ bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/ModelAssembler.java	2011-01-31 21:39:18 +0000
@@ -378,7 +378,7 @@
 		while (!sortedByOutdegree.isEmpty()) {
 			// don't sort unnecessarily: the current ordering is fine providing
 			// item #0 still has no dependencies
-			if (!depends.get(sortedByOutdegree.get(0)).isEmpty()) {
+			if (!requires.get(sortedByOutdegree.get(0)).isEmpty()) {
 				Collections.sort(sortedByOutdegree, outdegreeSorter);
 			}
 			String bundleId = sortedByOutdegree.remove(0);