summaryrefslogtreecommitdiffstatsabout
diff options
context:
space:
mode:
authorPaul Elder2013-04-08 11:48:35 (EDT)
committer Gerrit Code Review @ Eclipse.org2013-04-09 10:10:28 (EDT)
commitb0108276bfffc14e39f8a3f63cf42f0dc2b9e75f (patch)
tree9e4ba2b6b25083acf993d23865ca24250f362d2a
parente434b72fa7876b32d57dbd0e9367cc25faabcdf0 (diff)
downloadeclipse.platform.ui-b0108276bfffc14e39f8a3f63cf42f0dc2b9e75f.zip
eclipse.platform.ui-b0108276bfffc14e39f8a3f63cf42f0dc2b9e75f.tar.gz
eclipse.platform.ui-b0108276bfffc14e39f8a3f63cf42f0dc2b9e75f.tar.bz2
397302: AbstractSourceProvider.removeSourceProviderListener causesrefs/changes/29/11729/4
memory leak in many cases Change-Id: I76113d30772c3415e2032fcf2f728590dc575acc
-rw-r--r--bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/AbstractSourceProvider.java48
-rw-r--r--tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/leaks/Bug397302Tests.java164
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 89282e4..7967736 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 0000000..b82e567
--- /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();
+
+ }
+}