From a32dd5fa46d8969770f294f25858e6d1d60d85c2 Mon Sep 17 00:00:00 2001 From: Manolo Carrasco Date: Tue, 2 Oct 2012 12:26:47 +0200 Subject: Remove all references in the dataCache object whenever any queued function finishes. Remove empty references as well. Fixes issue132. --- .../java/com/google/gwt/query/client/Function.java | 2 + .../java/com/google/gwt/query/client/GQuery.java | 71 +++++++++++++--------- .../google/gwt/query/client/plugins/Effects.java | 22 ++++--- .../gwt/query/client/plugins/QueuePlugin.java | 12 ++-- .../client/plugins/effects/ClipAnimation.java | 1 - .../gwt/query/client/GQueryEffectsTestGwt.java | 60 ++++++++++++++++++ 6 files changed, 127 insertions(+), 41 deletions(-) diff --git a/gwtquery-core/src/main/java/com/google/gwt/query/client/Function.java b/gwtquery-core/src/main/java/com/google/gwt/query/client/Function.java index f8207a37..f6a7b82c 100644 --- a/gwtquery-core/src/main/java/com/google/gwt/query/client/Function.java +++ b/gwtquery-core/src/main/java/com/google/gwt/query/client/Function.java @@ -219,6 +219,8 @@ public abstract class Function { /** * Override this method for bound event handlers. + * + * @return boolean: false means stop propagation and prevent default */ public boolean f(Event e) { setEvent(e); diff --git a/gwtquery-core/src/main/java/com/google/gwt/query/client/GQuery.java b/gwtquery-core/src/main/java/com/google/gwt/query/client/GQuery.java index a2abd8ca..831e9b61 100644 --- a/gwtquery-core/src/main/java/com/google/gwt/query/client/GQuery.java +++ b/gwtquery-core/src/main/java/com/google/gwt/query/client/GQuery.java @@ -15,6 +15,11 @@ package com.google.gwt.query.client; import static com.google.gwt.query.client.plugins.QueuePlugin.Queue; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map; + import com.google.gwt.core.client.GWT; import com.google.gwt.core.client.JavaScriptObject; import com.google.gwt.core.client.JsArray; @@ -61,11 +66,6 @@ import com.google.gwt.user.client.Window; import com.google.gwt.user.client.ui.GqUi; import com.google.gwt.user.client.ui.Widget; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.Map; - /** * GwtQuery is a GWT clone of the popular jQuery library. */ @@ -123,9 +123,9 @@ public class GQuery implements Lazy { public static final BodyElement body = Document.get().getBody(); /** - * Object to store element data. + * Object to store element data (public so as we can access to it from tests). */ - protected static JsCache dataCache = null; + public static JsCache dataCache = null; /** * The document element in the current page. @@ -153,6 +153,8 @@ public class GQuery implements Lazy { public static Class GQUERY = GQuery.class; private static final String OLD_DATA_PREFIX = "old-"; + + private static final String OLD_DISPLAY = OLD_DATA_PREFIX + "display"; private static JsMap, Plugin> plugins; @@ -422,25 +424,32 @@ public class GQuery implements Lazy { return GQuery.data(e, key, null); } - protected static Object data(Element item, String name, S value) { + /** + * We store data in js object which has this structure: + * + * datacache [element_hash] [key] = value + * + * @return the value stored in the element with the given name + */ + protected static Object data(Element element, String key, S value) { if (dataCache == null) { windowData = JavaScriptObject.createObject().cast(); dataCache = JavaScriptObject.createObject().cast(); } - item = item == window || item.getNodeName() == null ? windowData : item; - if (item == null) { - return value; - } - int id = item.hashCode(); - if (name != null && !dataCache.exists(id)) { - dataCache.put(id, JsCache.createObject().cast()); - } + element = element == window || element.getNodeName() == null ? windowData : element; + if (element != null && key != null) { + int id = element.hashCode(); - JsCache d = dataCache.getCache(id); - if (name != null && value != null) { - d.put(name, value); + if (value == null) { + return dataCache.exists(id) ? dataCache.getCache(id).get(key) : null; + } + + if (!dataCache.exists(id)) { + dataCache.put(id, JsCache.createObject().cast()); + } + dataCache.getCache(id).put(key, value); } - return name != null ? d.get(name) : id; + return value; } /** @@ -2283,10 +2292,10 @@ public class GQuery implements Lazy { */ public GQuery hide() { for (Element e : elements) { - String currentDisplay = e.getStyle().getDisplay(); - Object old = data(e, "oldDisplay", null); - if (old == null && !"none".equals(currentDisplay)) { - data(e, "oldDisplay", getStyleImpl().curCSS(e, "display", false)); + String currentDisplay = getStyleImpl().curCSS(e, "display", false); + Object old = data(e, OLD_DISPLAY, null); + if (old == null && !currentDisplay.matches("(|none)")) { + data(e, OLD_DISPLAY, currentDisplay); } } @@ -3503,7 +3512,7 @@ public class GQuery implements Lazy { return this; } - private void removeData(Element item, String name) { + protected void removeData(Element item, String name) { if (dataCache == null) { windowData = JavaScriptObject.createObject().cast(); dataCache = JavaScriptObject.createObject().cast(); @@ -3518,6 +3527,7 @@ public class GQuery implements Lazy { removeData(item, null); } } else { + // when the element cache is empty we remove its entry to save memory (issue 132) dataCache.delete(id); } } @@ -3654,7 +3664,9 @@ public class GQuery implements Lazy { public void restoreCssAttrs(String... cssProps) { for (Element e : elements) { for (String a : cssProps) { - getStyleImpl().setStyleProperty(e, a, (String) data(e, OLD_DATA_PREFIX + a, null)); + String datakey = OLD_DATA_PREFIX + a; + getStyleImpl().setStyleProperty(e, a, (String) data(e, datakey, null)); + removeData(e, datakey); } } } @@ -3830,7 +3842,7 @@ public class GQuery implements Lazy { public GQuery show() { for (Element e : elements) { String currentDisplay = e.getStyle().getDisplay(); - String oldDisplay = (String) data(e, "oldDisplay", null); + String oldDisplay = (String) data(e, OLD_DISPLAY, null); // reset the display if (oldDisplay == null && "none".equals(currentDisplay)) { @@ -3841,7 +3853,7 @@ public class GQuery implements Lazy { // check if the stylesheet impose display: none. If it is the case, determine // the default display for the tag and store it at the element level if ("".equals(currentDisplay) && !getStyleImpl().isVisible(e)) { - data(e, "oldDisplay", getStyleImpl().defaultDisplay(e.getNodeName())); + data(e, OLD_DISPLAY, getStyleImpl().defaultDisplay(e.getNodeName())); } } @@ -3853,9 +3865,10 @@ public class GQuery implements Lazy { String currentDisplay = e.getStyle().getDisplay(); if ("".equals(currentDisplay) || "none".equals(currentDisplay)) { getStyleImpl().setStyleProperty(e, "display", - JsUtils.or((String) data(e, "oldDisplay", null), "")); + JsUtils.or((String) data(e, OLD_DISPLAY, null), "")); } } + removeData(OLD_DISPLAY); return this; } diff --git a/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/Effects.java b/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/Effects.java index e3f69e99..604ab6e6 100755 --- a/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/Effects.java +++ b/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/Effects.java @@ -21,10 +21,10 @@ import com.google.gwt.query.client.Function; import com.google.gwt.query.client.GQuery; import com.google.gwt.query.client.Properties; import com.google.gwt.query.client.plugins.effects.ClipAnimation; -import com.google.gwt.query.client.plugins.effects.Fx; -import com.google.gwt.query.client.plugins.effects.PropertiesAnimation; import com.google.gwt.query.client.plugins.effects.ClipAnimation.Action; import com.google.gwt.query.client.plugins.effects.ClipAnimation.Direction; +import com.google.gwt.query.client.plugins.effects.Fx; +import com.google.gwt.query.client.plugins.effects.PropertiesAnimation; import com.google.gwt.query.client.plugins.effects.PropertiesAnimation.Easing; /** @@ -36,16 +36,27 @@ public class Effects extends QueuePlugin { * Class to access protected methods in Animation. */ public static abstract class GQAnimation extends Animation { + + private static final String ACTUAL_ANIMATION = "EffectsRunnning"; + // Each Animation is associated to one element protected Element e; protected void onStart() { + // Mark this animation as actual, so as we can stop it in the GQuery.stop() method + $(e).data(ACTUAL_ANIMATION, this); super.onStart(); } protected void onComplete() { - $(e).remove(ACTUAL_ANIMATION); + // avoid memory leak (issue #132) + $(e).removeData(ACTUAL_ANIMATION); super.onComplete(); } + public void cancel() { + // avoid memory leak (issue #132) + $(e).removeData(ACTUAL_ANIMATION); + super.cancel(); + } } /** @@ -57,7 +68,6 @@ public class Effects extends QueuePlugin { public static final int SLOW = 600; } - private static final String ACTUAL_ANIMATION = "EffectsRunnning"; public static final Class Effects = GQuery.registerPlugin( Effects.class, new Plugin() { @@ -77,14 +87,12 @@ public class Effects extends QueuePlugin { } else { queue(e, DEFAULT_NAME, new Function() { public void cancel(Element e) { - Animation anim = (Animation) data(e, ACTUAL_ANIMATION, null); + Animation anim = (Animation) data(e, GQAnimation.ACTUAL_ANIMATION, null); if (anim != null) { - remove(ACTUAL_ANIMATION); anim.cancel(); } } public void f(Element e) { - data(e, ACTUAL_ANIMATION, anim); anim.run(duration); } }); diff --git a/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/QueuePlugin.java b/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/QueuePlugin.java index dc97b4b3..1c2a570a 100644 --- a/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/QueuePlugin.java +++ b/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/QueuePlugin.java @@ -137,7 +137,8 @@ public class QueuePlugin> extends GQuery { * in the named queue. */ public int queue(String name) { - return isEmpty() ? 0 : queue(get(0), name, null).size(); + Queue q = isEmpty() ? null : queue(get(0), name, null); + return q == null? 0 : q.size(); } /** @@ -264,6 +265,9 @@ public class QueuePlugin> extends GQuery { if (f instanceof Function) { ((Function) f).fe(elem); } + } else { + // if it is the last function remove the queue to avoid leaks (issue 132) + removeData(elem, name); } } } @@ -272,10 +276,10 @@ public class QueuePlugin> extends GQuery { protected Queue queue(Element elem, String name, S func) { if (elem != null) { Queue q = (Queue) data(elem, name, null); - if (q == null) { - q = (Queue) data(elem, name, new LinkedList()); - } if (func != null) { + if (q == null) { + q = (Queue) data(elem, name, new LinkedList()); + } q.add(func); if (q.size() == 1) { if (func instanceof Function) { diff --git a/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/effects/ClipAnimation.java b/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/effects/ClipAnimation.java index 3af40938..bb842815 100755 --- a/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/effects/ClipAnimation.java +++ b/gwtquery-core/src/main/java/com/google/gwt/query/client/plugins/effects/ClipAnimation.java @@ -53,7 +53,6 @@ public class ClipAnimation extends GQAnimation { Action action; Corner corner; Direction direction; - Element e; int percent; private GQuery back = Effects.$(); private Function[] funcs; diff --git a/gwtquery-core/src/test/java/com/google/gwt/query/client/GQueryEffectsTestGwt.java b/gwtquery-core/src/test/java/com/google/gwt/query/client/GQueryEffectsTestGwt.java index f286d6f9..a4ca5509 100644 --- a/gwtquery-core/src/test/java/com/google/gwt/query/client/GQueryEffectsTestGwt.java +++ b/gwtquery-core/src/test/java/com/google/gwt/query/client/GQueryEffectsTestGwt.java @@ -27,7 +27,9 @@ import com.google.gwt.query.client.plugins.effects.PropertiesAnimation; import com.google.gwt.query.client.plugins.effects.PropertiesAnimation.Easing; import com.google.gwt.user.client.Timer; import com.google.gwt.user.client.ui.HTML; +import com.google.gwt.user.client.ui.Label; import com.google.gwt.user.client.ui.RootPanel; +import com.google.gwt.user.client.ui.Widget; /** * Test class for testing gwtquery effects plugin api. @@ -424,4 +426,62 @@ public class GQueryEffectsTestGwt extends GWTTestCase { timerLongTime.schedule(2200); } + + + int animationRunCounter = 0; + public void testQueuesAndDataLeaks_issue132() { + + final Widget w = new Label("some animation"); + w.setVisible(false); + RootPanel.get().add(w); + w.getElement().setId("e"); + GQuery g = $(w); + + int test_duration = 1000; + int fx_duration = 200; + final int loops = test_duration / fx_duration; + + // Queue a set of effects which will use the data cache + for (int i = 0; i < loops ; i++) { + final char[] bulk = new char[5*1024*1024]; // let's leak 5MBs + g.fadeToggle(fx_duration, new Function() { + public void f() { + animationRunCounter ++; + bulk[0] = 0; // we keep it in handler + } + }); + } + + // Testing delay as well + g.delay(fx_duration, new Function(){ + public void f() { + animationRunCounter ++; + } + }); + + // We do the assertions after all effects have been run + g.queue(new Function() { + public void f() { + // after running queue method it is mandatory to call dequeue, + // otherwise the queue get stuck + $(this).dequeue(); + // Check that all animations and the delayed function has been run + assertEquals(loops + 1, animationRunCounter); + + // Check that nothings is left in the dataCache object + assertEquals(0, GQuery.dataCache.length()); + + // Check that getting queue size does not initialize the data + // object for this object + assertEquals(0, $(this).queue()); + assertEquals(0, GQuery.dataCache.length()); + + // Mark the test as success and stop delay timer + finishTest(); + }; + }); + + // delay the test enough to run all animations + delayTestFinish(test_duration * 2); + } } -- cgit v1.2.3