397302: AbstractSourceProvider.removeSourceProviderListener causes

memory leak in many cases

Change-Id: I76113d30772c3415e2032fcf2f728590dc575acc

2 files changed, 176 insertions, 36 deletions

diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/AbstractSourceProvider.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/AbstractSourceProvider.java
index 89282e4784b..796773614cb 100644
--- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/AbstractSourceProvider.java
+++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/AbstractSourceProvider.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2005, 2008 IBM Corporation and others.
+ * Copyright (c) 2005, 2013 IBM Corporation 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
@@ -12,8 +12,8 @@
 package org.eclipse.ui;
 
 import java.util.Map;
-
 import org.eclipse.core.commands.util.Tracing;
+import org.eclipse.core.runtime.ListenerList;
 import org.eclipse.ui.internal.misc.Policy;
 import org.eclipse.ui.services.IServiceLocator;
 
@@ -38,15 +38,10 @@ public abstract class AbstractSourceProvider implements ISourceProvider {
 
 	/**
 	 * The listeners to this source provider. This value is never
-	 * <code>null</code>. {@link #listenerCount} should be consulted to get
-	 * the real length.
+	 * <code>null</code>.
 	 */
-	private ISourceProviderListener[] listeners = new ISourceProviderListener[7];
+	private final ListenerList listeners = new ListenerList(ListenerList.IDENTITY);
 
-	/**
-	 * The number of listeners in the array.
-	 */
-	private int listenerCount = 0;
 
 	public final void addSourceProviderListener(
 			final ISourceProviderListener listener) {
@@ -54,12 +49,7 @@ public abstract class AbstractSourceProvider implements ISourceProvider {
 			throw new NullPointerException("The listener cannot be null"); //$NON-NLS-1$
 		}
 
-		if (listenerCount == listeners.length) {
-			final ISourceProviderListener[] growArray = new ISourceProviderListener[listeners.length + 4];
-			System.arraycopy(listeners, 0, growArray, 0, listeners.length);
-			listeners = growArray;
-		}
-		listeners[listenerCount++] = listener;
+		listeners.add(listener);
 	}
 
 	/**
@@ -75,9 +65,9 @@ public abstract class AbstractSourceProvider implements ISourceProvider {
 	 */
 	protected final void fireSourceChanged(final int sourcePriority,
 			final String sourceName, final Object sourceValue) {
-		for (int i = 0; i < listenerCount; i++) {
-			final ISourceProviderListener listener = listeners[i];
-			listener.sourceChanged(sourcePriority, sourceName, sourceValue);
+		for (Object listener : listeners.getListeners()) {
+			((ISourceProviderListener) listener).sourceChanged(sourcePriority, sourceName,
+					sourceValue);
 		}
 	}
 
@@ -94,9 +84,9 @@ public abstract class AbstractSourceProvider implements ISourceProvider {
 	 */
 	protected final void fireSourceChanged(final int sourcePriority,
 			final Map sourceValuesByName) {
-		for (int i = 0; i < listenerCount; i++) {
-			final ISourceProviderListener listener = listeners[i];
-			listener.sourceChanged(sourcePriority, sourceValuesByName);
+
+		for (Object listener : listeners.getListeners()) {
+			((ISourceProviderListener) listener).sourceChanged(sourcePriority, sourceValuesByName);
 		}
 	}
 
@@ -122,21 +112,7 @@ public abstract class AbstractSourceProvider implements ISourceProvider {
 			throw new NullPointerException("The listener cannot be null"); //$NON-NLS-1$
 		}
 
-		int emptyIndex = -1;
-		for (int i = 0; i < listenerCount; i++) {
-			if (listeners[i] == listener) {
-				listeners[i] = null;
-				emptyIndex = i;
-			}
-		}
-
-		if (emptyIndex != -1) {
-			// Compact the array.
-			for (int i = emptyIndex + 1; i < listenerCount; i++) {
-				listeners[i - 1] = listeners[i];
-			}
-			listenerCount--;
-		}
+		listeners.remove(listener);
 	}
 
 	/**
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/leaks/Bug397302Tests.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/leaks/Bug397302Tests.java
new file mode 100644
index 00000000000..b82e567e658
--- /dev/null
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/leaks/Bug397302Tests.java
@@ -0,0 +1,164 @@
+/*******************************************************************************
+ * Copyright (c) 2013 IBM Corporation 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:
+ *     IBM Corporation - initial API and implementation
+ ******************************************************************************/
+
+package org.eclipse.ui.tests.leaks;
+
+import java.lang.ref.WeakReference;
+import java.util.Collections;
+import java.util.Map;
+
+import org.eclipse.ui.AbstractSourceProvider;
+import org.eclipse.ui.ISourceProviderListener;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * @since 3.5
+ *
+ */
+public class Bug397302Tests {
+	
+	/**
+	 * @since 3.5
+	 *
+	 */
+	private static class TestListener implements ISourceProviderListener {
+		
+		private long callCount = 0;
+		
+		public long getCallCount() {
+			return callCount;
+		}
+
+		/**
+		 */
+		public TestListener() {
+		}
+
+		/* (non-Javadoc)
+		 * @see org.eclipse.ui.ISourceProviderListener#sourceChanged(int, java.lang.String, java.lang.Object)
+		 */
+		public void sourceChanged(int sourcePriority, String sourceName,
+				Object sourceValue) {
+			++callCount;
+		}
+
+		/* (non-Javadoc)
+		 * @see org.eclipse.ui.ISourceProviderListener#sourceChanged(int, java.util.Map)
+		 */
+		public void sourceChanged(int sourcePriority, Map sourceValuesByName) {
+			++callCount;
+			}
+	}
+
+	private static final class TestSourceProvider extends AbstractSourceProvider {
+
+		/* (non-Javadoc)
+		 * @see org.eclipse.ui.ISourceProvider#dispose()
+		 */
+		public void dispose() {
+			// do nothing
+		}
+
+		/* (non-Javadoc)
+		 * @see org.eclipse.ui.ISourceProvider#getCurrentState()
+		 */
+		public Map getCurrentState() {
+			return Collections.EMPTY_MAP;
+		}
+
+		/* (non-Javadoc)
+		 * @see org.eclipse.ui.ISourceProvider#getProvidedSourceNames()
+		 */
+		public String[] getProvidedSourceNames() {
+			return new String[] {};
+		}
+
+		/**
+		 * 
+		 */
+		public void callOut() {
+			this.fireSourceChanged(0, Collections.EMPTY_MAP);
+		}
+		
+	}
+
+	/**
+	 * Reproduce the problem, as described in the bug report.
+	 */
+	@Test
+	public void testBugAsDescribed() {
+		final TestSourceProvider testSourceProvider = new TestSourceProvider();
+		TestListener a = new TestListener();
+		TestListener b = new TestListener();
+		// keep weak references so we can check on the GC status later on...
+		final WeakReference<TestListener> listenerARef = new WeakReference<TestListener>(a);
+		final WeakReference<TestListener> listenerBRef = new WeakReference<TestListener>(b);
+		
+		// add listeners, call out the them, and verify that they got called.
+		testSourceProvider.addSourceProviderListener(a);
+		testSourceProvider.addSourceProviderListener(b);
+		
+		testSourceProvider.callOut();
+		
+		Assert.assertEquals(1, a.getCallCount());
+		Assert.assertEquals(1, b.getCallCount());
+
+		// remove listeners, call out to them, and verify that they no longer got called
+		testSourceProvider.removeSourceProviderListener(a);
+		testSourceProvider.removeSourceProviderListener(b);
+		
+		testSourceProvider.callOut();
+		
+		Assert.assertEquals(1, a.getCallCount());
+		Assert.assertEquals(1, b.getCallCount());		
+
+		// loose our strong references to a & b, force a GC, and see whether either gets leaked.
+		// Test: The bug asserts that B has been leaked. Force a GC, and test whether
+		// our weak references have gone null of not. If there is no leak, then both 
+		// should be null.
+		a = null;
+		b = null;
+		
+		System.gc();
+		
+		Assert.assertNull("Reference A", listenerARef.get());
+		Assert.assertNull("Reference B", listenerBRef.get());
+		
+		// Need this to prevent the above GC call from sweeping everything up before we're ready. 
+		// See this only when NOT in debug.
+		testSourceProvider.callOut();
+		
+	}
+
+	/**
+	 * Test that removal during call out does not cause problems.
+	 */
+	@Test
+	public void testRemoveDuringCallOut() {
+		final TestSourceProvider testSourceProvider = new TestSourceProvider();
+		final TestListener testListener = new TestListener() {
+			/* (non-Javadoc)
+			 * @see org.eclipse.ui.tests.leaks.Bug397302Tests.TestListener#sourceChanged(int, java.util.Map)
+			 */
+			@Override
+			public void sourceChanged(int sourcePriority, Map sourceValuesByName) {
+				testSourceProvider.removeSourceProviderListener(this);
+			}
+			
+		};
+		testSourceProvider.addSourceProviderListener(testListener);
+		
+		// With improper protection, this was can through something like ConcurrentModificationException
+		testSourceProvider.callOut();
+		
+	}
+}