Remove all references in the dataCache object whenever any queued function finishes. Remove empty references as well. Fixes issue132.

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<GQuery, LazyGQuery> {
   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<GQuery, LazyGQuery> {
   public static Class<GQuery> GQUERY = GQuery.class;

 

   private static final String OLD_DATA_PREFIX = "old-";

+  

+  private static final String OLD_DISPLAY = OLD_DATA_PREFIX + "display";

 

   private static JsMap<Class<? extends GQuery>, Plugin<? extends GQuery>> plugins;

 

@@ -422,25 +424,32 @@ public class GQuery implements Lazy<GQuery, LazyGQuery> {
     return GQuery.data(e, key, null);

   }

 

-  protected static <S> 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 <S> 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<GQuery, LazyGQuery> {
    */

   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<GQuery, LazyGQuery> {
     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<GQuery, LazyGQuery> {
         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<GQuery, LazyGQuery> {
   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<GQuery, LazyGQuery> {
   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<GQuery, LazyGQuery> {
       // 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<GQuery, LazyGQuery> {
       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<Effects> {
    * 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<Effects> {
     public static final int SLOW = 600;

   }

 

-  private static final String ACTUAL_ANIMATION = "EffectsRunnning";

   

   public static final Class<Effects> Effects = GQuery.registerPlugin(

       Effects.class, new Plugin<Effects>() {

@@ -77,14 +87,12 @@ public class Effects extends QueuePlugin<Effects> {
     } 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<T extends QueuePlugin<?>> extends GQuery {
    * in the named queue.
    */
   public int queue(String name) {
-    return isEmpty() ? 0 : queue(get(0), name, null).size();
+    Queue<Object> q = isEmpty() ? null : queue(get(0), name, null);
+    return q == null? 0 : q.size();
   }
   
   /**
@@ -264,6 +265,9 @@ public class QueuePlugin<T extends 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<T extends QueuePlugin<?>> extends GQuery {
   protected <S> Queue<S> queue(Element elem, String name, S func) {
     if (elem != null) {
       Queue<S> q = (Queue<S>) data(elem, name, null);
-      if (q == null) {
-        q = (Queue<S>) data(elem, name, new LinkedList<S>());
-      }
       if (func != null) {
+        if (q == null) {
+          q = (Queue<S>) data(elem, name, new LinkedList<S>());
+        }
         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);
+  }
 }