fix(jqLite): .data()/.bind() memory leak

mhevery · IgorMinar · commit ec1c5dfaee32 · 2012-05-14T21:56:22.000-07:00
Since angular attaches scope/injector/controller
into DOM it should clean up after itself. No need
to complain about memory leaks, since they can
only happened on detached DOM. Detached DOM would
only be in tests, since in production the DOM
would be attached to render tree and removal
would automatically clear memory.
diff --git a/src/jqLite.js b/src/jqLite.js
@@ -74,7 +74,7 @@
  * @returns {Object} jQuery object.
  */
 
-var jqCache = {},
+var jqCache = JQLite.cache = {},
     jqName = JQLite.expando = 'ng-' + new Date().getTime(),
     jqId = 1,
     addEventListenerFn = (window.document.addEventListener
@@ -122,15 +122,15 @@ function JQLitePatchJQueryRemove(name, dispatchThis) {
         fireEvent = dispatchThis,
         set, setIndex, setLength,
         element, childIndex, childLength, children,
-        fns, data;
+        fns, events;
 
     while(list.length) {
       set = list.shift();
       for(setIndex = 0, setLength = set.length; setIndex < setLength; setIndex++) {
         element = jqLite(set[setIndex]);
         if (fireEvent) {
-          data = element.data('events');
-          if ( (fns = data && data.$destroy) ) {
+          events = element.data('events');
+          if ( (fns = events && events.$destroy) ) {
             forEach(fns, function(fn){
               fn.handler();
             });
@@ -185,19 +185,35 @@ function JQLiteDealoc(element){
   }
 }
 
+function JQLiteUnbind(element, type, fn) {
+  var events = JQLiteData(element, 'events'),
+      handle = JQLiteData(element, 'handle');
+
+  if (!handle) return; //no listeners registered
+
+  if (isUndefined(type)) {
+    forEach(events, function(eventHandler, type) {
+      removeEventListenerFn(element, type, eventHandler);
+      delete events[type];
+    });
+  } else {
+    if (isUndefined(fn)) {
+      removeEventListenerFn(element, type, events[type]);
+      delete events[type];
+    } else {
+      arrayRemove(events[type], fn);
+    }
+  }
+}
+
 function JQLiteRemoveData(element) {
   var cacheId = element[jqName],
       cache = jqCache[cacheId];
 
   if (cache) {
-    if (cache.bind) {
-      forEach(cache.bind, function(fn, type){
-        if (type == '$destroy') {
-          fn({});
-        } else {
-          removeEventListenerFn(element, type, fn);
-        }
-      });
+    if (cache.handle) {
+      cache.events.$destroy && cache.handle({}, '$destroy');
+      JQLiteUnbind(element);
     }
     delete jqCache[cacheId];
     element[jqName] = undefined; // ie does not allow deletion of attributes on elements.
@@ -499,8 +515,8 @@ forEach({
   };
 });
 
-function createEventHandler(element) {
-  var eventHandler = function (event) {
+function createEventHandler(element, events) {
+  var eventHandler = function (event, type) {
     if (!event.preventDefault) {
       event.preventDefault = function() {
         event.returnValue = false; //ie
@@ -530,8 +546,12 @@ function createEventHandler(element) {
       return event.defaultPrevented;
     };
 
-    forEach(eventHandler.fns, function(fn){
-      fn.call(element, event);
+    forEach(events[type || event.type], function(fn) {
+      try {
+        fn.call(element, event);
+      } catch (e) {
+        // Not much to do here since jQuery ignores these anyway
+      }
     });
 
     // Remove monkey-patched methods (IE),
@@ -548,7 +568,7 @@ function createEventHandler(element) {
       delete event.isDefaultPrevented;
     }
   };
-  eventHandler.fns = [];
+  eventHandler.elem = element;
   return eventHandler;
 }
 
@@ -563,61 +583,45 @@ forEach({
   dealoc: JQLiteDealoc,
 
   bind: function bindFn(element, type, fn){
-    var bind = JQLiteData(element, 'bind');
+    var events = JQLiteData(element, 'events'),
+        handle = JQLiteData(element, 'handle');
 
+    if (!events) JQLiteData(element, 'events', events = {});
+    if (!handle) JQLiteData(element, 'handle', handle = createEventHandler(element, events));
 
-    if (!bind) JQLiteData(element, 'bind', bind = {});
     forEach(type.split(' '), function(type){
-      var eventHandler = bind[type];
-
+      var eventFns = events[type];
 
-      if (!eventHandler) {
+      if (!eventFns) {
         if (type == 'mouseenter' || type == 'mouseleave') {
-          var mouseenter = bind.mouseenter = createEventHandler(element);
-          var mouseleave = bind.mouseleave = createEventHandler(element);
           var counter = 0;
 
+          events.mouseenter = [];
+          events.mouseleave = [];
 
           bindFn(element, 'mouseover', function(event) {
             counter++;
             if (counter == 1) {
-              mouseenter(event);
+              handle(event, 'mouseenter');
             }
           });
           bindFn(element, 'mouseout', function(event) {
             counter --;
             if (counter == 0) {
-              mouseleave(event);
+              handle(event, 'mouseleave');
             }
           });
-          eventHandler = bind[type];
         } else {
-          eventHandler = bind[type] = createEventHandler(element);
-          addEventListenerFn(element, type, eventHandler);
+          addEventListenerFn(element, type, handle);
+          events[type] = [];
         }
+        eventFns = events[type]
       }
-      eventHandler.fns.push(fn);
+      eventFns.push(fn);
     });
   },
 
-  unbind: function(element, type, fn) {
-    var bind = JQLiteData(element, 'bind');
-    if (!bind) return; //no listeners registered
-
-    if (isUndefined(type)) {
-      forEach(bind, function(eventHandler, type) {
-        removeEventListenerFn(element, type, eventHandler);
-        delete bind[type];
-      });
-    } else {
-      if (isUndefined(fn)) {
-        removeEventListenerFn(element, type, bind[type]);
-        delete bind[type];
-      } else {
-        arrayRemove(bind[type].fns, fn);
-      }
-    }
-  },
+  unbind: JQLiteUnbind,
 
   replaceWith: function(element, replaceNode) {
     var index, parent = element.parentNode;
diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js
@@ -1526,6 +1526,20 @@ angular.mock.e2e = {};
 angular.mock.e2e.$httpBackendDecorator = ['$delegate', '$browser', createHttpBackendMock];
 
 
+angular.mock.clearDataCache = function() {
+  var key,
+      cache = angular.element.cache;
+
+  for(key in cache) {
+    if (cache.hasOwnProperty(key)) {
+      var handle = cache[key].handle;
+
+      handle && angular.element(handle.elem).unbind();
+      delete cache[key];
+    }
+  }
+};
+
 
 window.jstestdriver && (function(window) {
   /**
@@ -1550,6 +1564,7 @@ window.jasmine && (function(window) {
     var spec = getCurrentSpec();
     spec.$injector = null;
     spec.$modules = null;
+    angular.mock.clearDataCache();
   });
 
   function getCurrentSpec() {
diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js
@@ -291,11 +291,37 @@ describe('jqLite', function() {
       expect(element.data()).toEqual({meLike: 'turtles', youLike: 'carrots', existing: 'val'});
       expect(element.data()).toBe(oldData); // merge into the old object
     });
+
+    describe('data cleanup', function() {
+      it('should remove data on element removal', function() {
+        var div = jqLite('<div><span>text</span></div>'),
+            span = div.find('span');
+
+        span.data('name', 'angular');
+        span.remove();
+        expect(span.data('name')).toBeUndefined();
+      });
+
+      it('should remove event listeners on element removal', function() {
+        var div = jqLite('<div><span>text</span></div>'),
+            span = div.find('span'),
+            log = '';
+
+        span.bind('click', function() { log+= 'click;'});
+        browserTrigger(span);
+        expect(log).toEqual('click;');
+
+        span.remove();
+
+        browserTrigger(span);
+        expect(log).toEqual('click;');
+      });
+    });
   });
 
 
   describe('attr', function() {
-    it('shoul read write and remove attr', function() {
+    it('should read write and remove attr', function() {
       var selector = jqLite([a, b]);
 
       expect(selector.attr('prop', 'value')).toEqual(selector);
@@ -667,7 +693,7 @@ describe('jqLite', function() {
       var jWindow = jqLite(window).bind('hashchange', function() {
         log = 'works!';
       });
-      eventFn({});
+      eventFn({type: 'hashchange'});
       expect(log).toEqual('works!');
       dealoc(jWindow);
     });
diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js
@@ -148,6 +148,7 @@ describe('ngMock', function() {
     });
   });
 
+
   describe('$log', function() {
     var $log;
     beforeEach(inject(['$log', function(log) {
@@ -229,6 +230,7 @@ describe('ngMock', function() {
     });
   });
 
+
   describe('defer', function() {
     var browser, log;
     beforeEach(inject(function($browser) {
@@ -341,6 +343,44 @@ describe('ngMock', function() {
     });
   });
 
+
+  describe('angular.mock.clearDataCache', function() {
+    function keys(obj) {
+      var keys = [];
+      for(var key in obj) {
+        if (obj.hasOwnProperty(key)) keys.push(key);
+      }
+      return keys.sort();
+    }
+
+    it('should remove data', function() {
+      expect(angular.element.cache).toEqual({});
+      var div = angular.element('<div></div>');
+      div.data('name', 'angular');
+      expect(keys(angular.element.cache)).not.toEqual([]);
+      angular.mock.clearDataCache();
+      expect(keys(angular.element.cache)).toEqual([]);
+    });
+
+    it('should deregister event handlers', function() {
+      expect(keys(angular.element.cache)).toEqual([]);
+
+      var div = angular.element('<div></div>');
+
+      div.bind('click', angular.noop);
+      div.bind('mousemove', angular.noop);
+      div.data('some', 'data');
+      expect(keys(angular.element.cache).length).toBe(1);
+
+      angular.mock.clearDataCache();
+      expect(keys(angular.element.cache)).toEqual([]);
+      expect(div.data('some')).toBeUndefined();
+
+      div.remove();
+    });
+  });
+
+
   describe('jasmine module and inject', function(){
     var log;
 
diff --git a/test/testabilityPatch.js b/test/testabilityPatch.js
@@ -42,7 +42,6 @@ afterEach(function() {
   var count = 0;
   forEachSorted(jqCache, function(value, key){
     count ++;
-    delete jqCache[key];
     forEach(value, function(value, key){
       if (value.$element) {
         dump('LEAK', key, value.$id, sortedHtml(value.$element));
