Plug memory leak in ResettableEventBus, fix for
http://code.google.com/p/google-web-toolkit/issues/detail?id=5700
Review by rjrjr at http://gwt-code-reviews.appspot.com/1388804/
Review by: jlabanca@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@9928 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/event/shared/ResettableEventBus.java b/user/src/com/google/gwt/event/shared/ResettableEventBus.java
index 5769a79..50055d6 100644
--- a/user/src/com/google/gwt/event/shared/ResettableEventBus.java
+++ b/user/src/com/google/gwt/event/shared/ResettableEventBus.java
@@ -18,6 +18,7 @@
import com.google.gwt.event.shared.GwtEvent.Type;
import java.util.HashSet;
+import java.util.Iterator;
import java.util.Set;
/**
@@ -25,6 +26,7 @@
* easily all be cleared at once.
*/
public class ResettableEventBus extends EventBus {
+
private final EventBus wrapped;
private final Set<HandlerRegistration> registrations = new HashSet<HandlerRegistration>();
@@ -33,20 +35,16 @@
}
@Override
- public <H extends EventHandler> HandlerRegistration addHandler(Type<H> type,
- H handler) {
+ public <H extends EventHandler> HandlerRegistration addHandler(Type<H> type, H handler) {
HandlerRegistration rtn = wrapped.addHandler(type, handler);
- registrations.add(rtn);
- return rtn;
+ return doRegisterHandler(rtn);
}
@Override
- public <H extends EventHandler> HandlerRegistration addHandlerToSource(
- GwtEvent.Type<H> type, Object source, H handler) {
- HandlerRegistration rtn = wrapped.addHandlerToSource(type, source,
- handler);
- registrations.add(rtn);
- return rtn;
+ public <H extends EventHandler> HandlerRegistration addHandlerToSource(GwtEvent.Type<H> type,
+ Object source, H handler) {
+ HandlerRegistration rtn = wrapped.addHandlerToSource(type, source, handler);
+ return doRegisterHandler(rtn);
}
@Override
@@ -63,9 +61,38 @@
* Remove all handlers that have been added through this wrapper.
*/
public void removeHandlers() {
- for (HandlerRegistration r : registrations) {
+ Iterator<HandlerRegistration> it = registrations.iterator();
+ while (it.hasNext()) {
+ HandlerRegistration r = it.next();
+
+ /*
+ * must remove before we call removeHandler. Might have come from nested
+ * ResettableEventBus
+ */
+ it.remove();
+
r.removeHandler();
}
- registrations.clear();
+ }
+
+ // Visible for testing
+ int getRegistrationSize() {
+ return registrations.size();
+ }
+
+ private HandlerRegistration doRegisterHandler(final HandlerRegistration registration) {
+ registrations.add(registration);
+ return new HandlerRegistration() {
+ public void removeHandler() {
+ doUnregisterHandler(registration);
+ }
+ };
+ }
+
+ private void doUnregisterHandler(HandlerRegistration registration) {
+ if (registrations.contains(registration)) {
+ registration.removeHandler();
+ registrations.remove(registration);
+ }
}
}
diff --git a/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java b/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
index de3fd36..9e14a8d 100644
--- a/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
+++ b/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
@@ -50,7 +50,7 @@
assertFired(mouse1, mouse2, mouse3);
reset();
-
+
subject.removeHandlers();
assertEquals(0, wrapped.getCount(type));
@@ -58,30 +58,30 @@
});
assertNotFired(mouse1, mouse2, mouse3);
}
-
+
public void testNestedResetInnerFirst() {
CountingEventBus wrapped = new CountingEventBus();
ResettableEventBus wideScope = new ResettableEventBus(wrapped);
ResettableEventBus narrowScope = new ResettableEventBus(wideScope);
-
+
Type<MouseDownHandler> type = MouseDownEvent.getType();
-
+
wideScope.addHandler(type, mouse1);
narrowScope.addHandler(type, mouse2);
-
+
wrapped.fireEvent(new MouseDownEvent() {
});
assertFired(mouse1, mouse2);
-
+
reset();
/*
- * When I remove handlers from the narrow resettable, it should have no effect
- * on handlers registered with the wider instance.
+ * When I remove handlers from the narrow resettable, it should have no
+ * effect on handlers registered with the wider instance.
*/
-
+
narrowScope.removeHandlers();
-
+
wrapped.fireEvent(new MouseDownEvent() {
});
assertFired(mouse1);
@@ -92,28 +92,90 @@
CountingEventBus wrapped = new CountingEventBus();
ResettableEventBus wideScope = new ResettableEventBus(wrapped);
ResettableEventBus narrowScope = new ResettableEventBus(wideScope);
-
+
Type<MouseDownHandler> type = MouseDownEvent.getType();
-
+
wideScope.addHandler(type, mouse1);
narrowScope.addHandler(type, mouse2);
-
+
wrapped.fireEvent(new MouseDownEvent() {
});
assertFired(mouse1, mouse2);
-
+
reset();
-
+
/*
- * When I remove handlers from the first resettable, handlers registered
- * by the narrower scoped one that wraps it should also be severed.
+ * When I remove handlers from the first resettable, handlers registered by
+ * the narrower scoped one that wraps it should also be severed.
*/
-
+
wideScope.removeHandlers();
-
+
wrapped.fireEvent(new MouseDownEvent() {
});
assertNotFired(mouse1);
assertNotFired(mouse2);
}
+
+ public void testManualRemoveMemory() {
+ SimpleEventBus eventBus = new SimpleEventBus();
+ ResettableEventBus subject = new ResettableEventBus(eventBus);
+
+ Type<MouseDownHandler> type = MouseDownEvent.getType();
+
+ HandlerRegistration registration1 = subject.addHandler(type, mouse1);
+ HandlerRegistration registration2 = subject.addHandler(type, mouse2);
+ HandlerRegistration registration3 = subject.addHandler(type, mouse3);
+
+ registration1.removeHandler();
+ registration2.removeHandler();
+ registration3.removeHandler();
+
+ /*
+ * removing handlers manually should remove registration from the internal
+ * set.
+ */
+
+ assertEquals(0, subject.getRegistrationSize());
+
+ subject.removeHandlers();
+
+ // Expect nothing to happen. Especially no exceptions.
+ registration1.removeHandler();
+ }
+
+ public void testNestedRemoveMemory() {
+ SimpleEventBus eventBus = new SimpleEventBus();
+ ResettableEventBus wideScope = new ResettableEventBus(eventBus);
+ ResettableEventBus narrowScope = new ResettableEventBus(wideScope);
+
+ Type<MouseDownHandler> type = MouseDownEvent.getType();
+ wideScope.addHandler(type, mouse1);
+ narrowScope.addHandler(type, mouse2);
+ narrowScope.addHandler(type, mouse3);
+
+ narrowScope.removeHandlers();
+ wideScope.removeHandlers();
+
+ /*
+ * Internal registeration should be empty after calling removeHandlers
+ */
+
+ assertEquals(0, wideScope.getRegistrationSize());
+ assertEquals(0, narrowScope.getRegistrationSize());
+
+ wideScope.addHandler(type, mouse1);
+ narrowScope.addHandler(type, mouse2);
+
+ /*
+ * Reverse remove order
+ */
+
+ wideScope.removeHandlers();
+ narrowScope.removeHandlers();
+
+ assertEquals(0, wideScope.getRegistrationSize());
+ assertEquals(0, narrowScope.getRegistrationSize());
+ }
+
}
