From 70ff7a2639fc55936854ad04a6242a700ae71a02 Mon Sep 17 00:00:00 2001
From: Igor Minar
Date: Tue, 12 Oct 2010 05:36:38 +0800
Subject: fix memory leak caused by leftbehind $invalidWidgets references

- ng:switch should not clean up $invalidWidgets
- $invalidWidgets should be clean up after each eval
- add missing docs
---
 src/services.js        | 33 ++++++++++++++++++++++++++++++---
 src/widgets.js         |  2 --
 test/BinderTest.js     | 12 ++++++++----
 test/ValidatorsTest.js | 10 ++++------
 test/servicesSpec.js   | 13 +++++++++----
 test/widgetsSpec.js    | 18 +++++++++---------
 6 files changed, 60 insertions(+), 28 deletions(-)

diff --git a/src/services.js b/src/services.js
index 41755962..be73c6e9 100644
--- a/src/services.js
+++ b/src/services.js
@@ -157,18 +157,32 @@ angularService("$hover", function(browser, document) {
   });
 }, {inject:['$browser', '$document']});
 
+
+
+/* Keeps references to all invalid widgets found during validation. Can be queried to find if there
+ * are invalid widgets currently displayed
+ */
 angularService("$invalidWidgets", function(){
   var invalidWidgets = [];
+
+
+  /** Remove an element from the array of invalid widgets */
   invalidWidgets.markValid = function(element){
     var index = indexOf(invalidWidgets, element);
     if (index != -1)
       invalidWidgets.splice(index, 1);
   };
+
+
+  /** Add an element to the array of invalid widgets */
   invalidWidgets.markInvalid = function(element){
     var index = indexOf(invalidWidgets, element);
     if (index === -1)
       invalidWidgets.push(element);
   };
+
+
+  /** Return count of all invalid widgets that are currently visible */
   invalidWidgets.visible = function() {
     var count = 0;
     foreach(invalidWidgets, function(widget){
@@ -176,24 +190,37 @@ angularService("$invalidWidgets", function(){
     });
     return count;
   };
-  invalidWidgets.clearOrphans = function() {
+
+
+  /* At the end of each eval removes all invalid widgets that are not part of the current DOM. */
+  this.$onEval(PRIORITY_LAST, function() {
     for(var i = 0; i < invalidWidgets.length;) {
       var widget = invalidWidgets[i];
       if (isOrphan(widget[0])) {
-        invalidWidgets.splice(i, 1);
+        invalidWidgets.splice(i, 1)
+        if (widget.dealoc) widget.dealoc();
       } else {
         i++;
       }
     }
-  };
+  });
+
+
+  /**
+   * Traverses DOM element's (widget's) parents and considers the element to be an orphant if one of
+   * it's parents isn't the current window.document.
+   */
   function isOrphan(widget) {
     if (widget == window.document) return false;
     var parent = widget.parentNode;
     return !parent || isOrphan(parent);
   }
+
   return invalidWidgets;
 });
 
+
+
 function switchRouteMatcher(on, when, dstName) {
   var regex = '^' + when.replace(/[\.\\\(\)\^\$]/g, "\$1") + '$',
       params = [],
diff --git a/src/widgets.js b/src/widgets.js
index 127718ce..c1342943 100644
--- a/src/widgets.js
+++ b/src/widgets.js
@@ -324,8 +324,6 @@ var ngSwitch = angularWidget('ng:switch', function (element){
           element.append(caseElement);
           childScope.$tryEval(switchCase.change, element);
           switchCase.template(caseElement, childScope);
-          if (scope.$invalidWidgets)
-            scope.$invalidWidgets.clearOrphans();
           childScope.$init();
         }
       });
diff --git a/test/BinderTest.js b/test/BinderTest.js
index f38383ae..5d72206d 100644
--- a/test/BinderTest.js
+++ b/test/BinderTest.js
@@ -2,10 +2,14 @@ BinderTest = TestCase('BinderTest');
 
 BinderTest.prototype.setUp = function(){
   var self = this;
-  this.compile = function(html, initialScope, config) {
+
+  this.compile = function(html, initialScope, parent) {
     var compiler = new Compiler(angularTextMarkup, angularAttrMarkup, angularDirective, angularWidget);
     var element = self.element = jqLite(html);
     var scope = compiler.compile(element)(element);
+
+    if (parent) parent.append(element);
+
     extend(scope, initialScope);
     scope.$init();
     return {node:element, scope:scope};
@@ -506,7 +510,8 @@ BinderTest.prototype.testFillInOptionValueWhenMissing = function() {
 
 BinderTest.prototype.testValidateForm = function() {
   var c = this.compile('<div><input name="name" ng:required>' +
-          '<div ng:repeat="item in items"><input name="item.name" ng:required/></div></div>');
+          '<div ng:repeat="item in items"><input name="item.name" ng:required/></div></div>',
+          undefined, jqLite(document.body));
   var items = [{}, {}];
   c.scope.$set("items", items);
   c.scope.$eval();
@@ -534,8 +539,7 @@ BinderTest.prototype.testValidateForm = function() {
 };
 
 BinderTest.prototype.testValidateOnlyVisibleItems = function(){
-  var c = this.compile('<div><input name="name" ng:required><input ng:show="show" name="name" ng:required></div>');
-  jqLite(document.body).append(c.node);
+  var c = this.compile('<div><input name="name" ng:required><input ng:show="show" name="name" ng:required></div>', undefined, jqLite(document.body));
   c.scope.$set("show", true);
   c.scope.$eval();
   assertEquals(2, c.scope.$get("$invalidWidgets.length"));
diff --git a/test/ValidatorsTest.js b/test/ValidatorsTest.js
index ca8dad12..07f3e488 100644
--- a/test/ValidatorsTest.js
+++ b/test/ValidatorsTest.js
@@ -99,16 +99,13 @@ describe('Validator:asynchronous', function(){
   var value, fn;
 
   beforeEach(function(){
-    var invalidWidgets = angularService('$invalidWidgets')();
     value = null;
     fn = null;
-    self = {
-        $element:jqLite('<input />'),
-        $invalidWidgets:invalidWidgets,
-        $eval: noop
-    };
+    self = compile('<input />');
+    jqLite(document.body).append(self.$element);
     self.$element.data('$validate', noop);
     self.$root = self;
+    self.$init();
   });
 
   afterEach(function(){
@@ -121,6 +118,7 @@ describe('Validator:asynchronous', function(){
   it('should make a request and show spinner', function(){
     var value, fn;
     var scope = compile('<input type="text" name="name" ng:validate="asynchronous:asyncFn"/>');
+    jqLite(document.body).append(scope.$element);
     scope.$init();
     var input = scope.$element;
     scope.asyncFn = function(v,f){
diff --git a/test/servicesSpec.js b/test/servicesSpec.js
index 04aebce7..c0101098 100644
--- a/test/servicesSpec.js
+++ b/test/servicesSpec.js
@@ -150,22 +150,27 @@ describe("service", function(){
 
   describe("$invalidWidgets", function(){
     it("should count number of invalid widgets", function(){
-      var scope = compile('<input name="price" ng:required ng:validate="number"></input>').$init();
+      var scope = compile('<input name="price" ng:required ng:validate="number"></input>');
+      jqLite(document.body).append(scope.$element);
+      scope.$init();
       expect(scope.$invalidWidgets.length).toEqual(1);
+
       scope.price = 123;
       scope.$eval();
       expect(scope.$invalidWidgets.length).toEqual(0);
+
       scope.$element.remove();
       scope.price = 'abc';
       scope.$eval();
-      expect(scope.$invalidWidgets.length).toEqual(1);
+      expect(scope.$invalidWidgets.length).toEqual(0);
 
       jqLite(document.body).append(scope.$element);
-      scope.$invalidWidgets.clearOrphans();
+      scope.price = 'abcd'; //force revalidation, maybe this should be done automatically?
+      scope.$eval();
       expect(scope.$invalidWidgets.length).toEqual(1);
 
       jqLite(document.body).html('');
-      scope.$invalidWidgets.clearOrphans();
+      scope.$eval();
       expect(scope.$invalidWidgets.length).toEqual(0);
     });
   });
diff --git a/test/widgetsSpec.js b/test/widgetsSpec.js
index d2a39535..80acc928 100644
--- a/test/widgetsSpec.js
+++ b/test/widgetsSpec.js
@@ -5,10 +5,11 @@ describe("widget", function(){
     scope = null;
     element = null;
     var compiler = new Compiler(angularTextMarkup, angularAttrMarkup, angularDirective, angularWidget);
-    compile = function(html, before) {
+    compile = function(html, before, parent) {
       element = jqLite(html);
       scope = compiler.compile(element)(element);
       (before||noop).apply(scope);
+      if (parent) parent.append(element);
       scope.$init();
     };
   });
@@ -163,7 +164,8 @@ describe("widget", function(){
 
       describe("ng:validate", function(){
         it("should process ng:validate", function(){
-          compile('<input type="text" name="price" value="abc" ng:validate="number"/>');
+          compile('<input type="text" name="price" value="abc" ng:validate="number"/>',
+                  undefined, jqLite(document.body));
           expect(element.hasClass('ng-validation-error')).toBeTruthy();
           expect(element.attr('ng-validation-error')).toEqual('Not a number');
 
@@ -217,7 +219,7 @@ describe("widget", function(){
     });
 
     it("should process ng:required", function(){
-      compile('<input type="text" name="price" ng:required/>');
+      compile('<input type="text" name="price" ng:required/>', undefined, jqLite(document.body));
       expect(element.hasClass('ng-validation-error')).toBeTruthy();
       expect(element.attr('ng-validation-error')).toEqual('Required');
 
@@ -233,7 +235,8 @@ describe("widget", function(){
     });
 
     it('should allow conditions on ng:required', function() {
-      compile('<input type="text" name="price" ng:required="ineedz"/>');
+      compile('<input type="text" name="price" ng:required="ineedz"/>',
+              undefined, jqLite(document.body));
       scope.$set('ineedz', false);
       scope.$eval();
       expect(element.hasClass('ng-validation-error')).toBeFalsy();
@@ -372,7 +375,8 @@ describe("widget", function(){
         compile(
             '<select name="selection" ng:required>' +
               '<option value="{{$index}}" ng:repeat="opt in options">{{opt}}</option>' +
-            '</select>');
+            '</select>',
+            undefined, jqLite(document.body));
         scope.selection = 1;
         scope.options = ['one', 'two'];
         scope.$eval();
@@ -459,13 +463,9 @@ describe("widget", function(){
       var scope = angular.compile('<ng:switch on="url" change="name=\'works\'"><div ng:switch-when="a">{{name}}</div></ng:switch>');
       var cleared = false;
       scope.url = 'a';
-      scope.$invalidWidgets = {clearOrphans: function(){
-        cleared = true;
-      }};
       scope.$init();
       expect(scope.name).toEqual(undefined);
       expect(scope.$element.text()).toEqual('works');
-      expect(cleared).toEqual(true);
     });
   });
 
-- 
cgit v1.2.3