Revision: 9928
Author: [email protected]
Date: Fri Apr 1 10:39:59 2011
Log: 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: [email protected]
http://code.google.com/p/google-web-toolkit/source/detail?r=9928
Modified:
/trunk/user/src/com/google/gwt/event/shared/ResettableEventBus.java
/trunk/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
=======================================
--- /trunk/user/src/com/google/gwt/event/shared/ResettableEventBus.java Fri
Sep 10 17:56:09 2010
+++ /trunk/user/src/com/google/gwt/event/shared/ResettableEventBus.java Fri
Apr 1 10:39:59 2011
@@ -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);
+ }
}
}
=======================================
---
/trunk/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
Tue Mar 29 12:31:22 2011
+++
/trunk/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
Fri Apr 1 10:39:59 2011
@@ -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());
+ }
+
+}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
