fix(ngMessages): prevent memory leak from messages that are never att…

…ached

Closes angular#16389
Closes angular#16404

src/ngMessages/messages.js

@@ -417,13 +417,6 @@ angular.module('ngMessages', [], function initAngularHelpers() {
 
         $scope.$watchCollection($attrs.ngMessages || $attrs['for'], ctrl.render);
 
-        // If the element is destroyed, proactively destroy all the currently visible messages
-        $element.on('$destroy', function() {
-          forEach(messages, function(item) {
-            item.message.detach();
-          });
-        });
-
         this.reRender = function() {
           if (!renderLater) {
             renderLater = true;
@@ -498,6 +491,9 @@ angular.module('ngMessages', [], function initAngularHelpers() {
         function removeMessageNode(parent, comment, key) {
           var messageNode = messages[key];
 
+          // This message node may have already been removed by a call to deregister()
+          if (!messageNode) return;
+
           var match = findPreviousMessage(parent, comment);
           if (match) {
             match.next = messageNode.next;
@@ -702,6 +698,8 @@ function ngMessageDirectiveFactory() {
                 // by another structural directive then it's time
                 // to deregister the message from the controller
                 currentElement.on('$destroy', function() {
+                  // If the message element was removed via a call to `detach` then `currentElement` will be null
+                  // So this handler only handles cases where something else removed the message element.
                   if (currentElement && currentElement.$$attachId === $$attachId) {
                     ngMessagesCtrl.deregister(commentNode);
                     messageCtrl.detach();
@@ -719,6 +717,14 @@ function ngMessageDirectiveFactory() {
             }
           }
         });
+
+        // We need to ensure that this directive deregisters itself when it no longer exists
+        // Normally this is done when the attached element is destroyed; but if this directive
+        // gets removed before we attach the message to the DOM there is nothing to watch
+        // in which case we must deregister when the containing scope is destroyed.
+        scope.$on('$destroy', function() {
+          ngMessagesCtrl.deregister(commentNode);
+        });
       }
     };
   }];

test/ngMessages/messagesSpec.js

@@ -636,6 +636,30 @@ describe('ngMessages', function() {
     })
   );
 
+  it('should unregister the ngMessage even if it was never attached',
+    inject(function($compile, $rootScope) {
+      var html =
+        '<div ng-messages="items">' +
+          '<div ng-if="show"><div ng-message="x">ERROR</div></div>' +
+        '</div>';
+
+      element = $compile(html)($rootScope);
+
+      var ctrl = element.controller('ngMessages');
+
+      expect(messageChildren(element).length).toBe(0);
+      expect(Object.keys(ctrl.messages).length).toEqual(0);
+
+      $rootScope.$apply('show = true');
+      expect(messageChildren(element).length).toBe(0);
+      expect(Object.keys(ctrl.messages).length).toEqual(1);
+
+      $rootScope.$apply('show = false');
+      expect(messageChildren(element).length).toBe(0);
+      expect(Object.keys(ctrl.messages).length).toEqual(0);
+    })
+  );
+
 
   describe('when including templates', function() {
     they('should work with a dynamic collection model which is managed by ngRepeat',