fix(jqLite): don't attach event handlers to comments or text nodes

petebacondarwin · petebacondarwin · commit 462dbb2016a2 · 2014-06-25T22:34:04.000+01:00
We were attaching handlers to comment nodes when setting up bound transclusion functions. But we don't clean up comments and text nodes when deallocating so there was a memory leak. Closes angular#7913 Closes angular#7942
diff --git a/src/jqLite.js b/src/jqLite.js
@@ -161,6 +161,12 @@ function jqLiteIsTextNode(html) {
   return !HTML_REGEXP.test(html);
 }
 
+function jqLiteAcceptsData(node) {
+  // The window object can accept data but has no nodeType
+  // Otherwise we are only interested in elements (1) and documents (9)
+  return !node.nodeType || node.nodeType === 1 || node.nodeType === 9;
+}
+
 function jqLiteBuildFragment(html, context) {
   var elem, tmp, tag, wrap,
       fragment = context.createDocumentFragment(),
@@ -319,7 +325,7 @@ function jqLiteData(element, key, value) {
 
   if (isSetter) {
     // set data only on Elements and Documents
-    if (element.nodeType === 1 || element.nodeType === 9) {
+    if (jqLiteAcceptsData(element)) {
       data[key] = value;
     }
   } else {
@@ -754,6 +760,11 @@ forEach({
   on: function onFn(element, type, fn, unsupported){
     if (isDefined(unsupported)) throw jqLiteMinErr('onargs', 'jqLite#on() does not support the `selector` or `eventData` parameters');
 
+    // Do not add event handlers to non-elements because they will not be cleaned up.
+    if (!jqLiteAcceptsData(element)) {
+      return;
+    }
+
     var events = jqLiteExpandoStore(element, 'events'),
         handle = jqLiteExpandoStore(element, 'handle');
 
diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js
@@ -993,6 +993,16 @@ describe('jqLite', function() {
       expect(log).toEqual('click on: A;click on: B;');
     });
 
+    it('should not bind to comment or text nodes', function() {
+      var nodes = jqLite('<!-- some comment -->Some text');
+      var someEventHandler = jasmine.createSpy('someEventHandler');
+
+      nodes.on('someEvent', someEventHandler);
+      nodes.triggerHandler('someEvent');
+
+      expect(someEventHandler).not.toHaveBeenCalled();
+    });
+
     it('should bind to all events separated by space', function() {
       var elm = jqLite(a),
           callback = jasmine.createSpy('callback');
