fix(ngMessages): don't crash when nested messages are removed

Under specific circumstances, ngMessages would go into an infinite loop and crash the
browser / page:
- At least two ngMessage elements are wrapped inside another element (e.g. ngTransclude)
- The first message is currently visible
- The first message is removed (e.g. when the whole ngMessages element is removed by an ngIf)

When a message is removed, it looks for a previous message - in this specific case it would misidentify
the second message for a previous message, which would then cause the first message to be marked as the
second message's next message, resulting in an infinite loop, and crash.

This fix ensures that when searching for previous messages, ngMessage walks the DOM in a way so
that messages that come after the current message are never identified as previous messages.

This commit also detaches and destroys all child ngMessage elements when the ngMessages element is
destroyed, which should improve performance slightly.

Fixes #14183
Closes #14242

Author: Narretz

src/ngMessages/messages.js

@@ -410,6 +410,13 @@ angular.module('ngMessages', [])
 
          $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;
@@ -444,6 +451,7 @@ angular.module('ngMessages', [])
          function findPreviousMessage(parent, comment) {
            var prevNode = comment;
            var parentLookup = [];
+
            while (prevNode && prevNode !== parent) {
              var prevKey = prevNode.$$ngMessageNode;
              if (prevKey && prevKey.length) {
@@ -455,8 +463,11 @@ angular.module('ngMessages', [])
              if (prevNode.childNodes.length && parentLookup.indexOf(prevNode) == -1) {
                parentLookup.push(prevNode);
                prevNode = prevNode.childNodes[prevNode.childNodes.length - 1];
+             } else if (prevNode.previousSibling) {
+               prevNode = prevNode.previousSibling;
              } else {
-               prevNode = prevNode.previousSibling || prevNode.parentNode;
+               prevNode = prevNode.parentNode;
+               parentLookup.push(prevNode);
              }
            }
          }
@@ -669,8 +680,8 @@ function ngMessageDirectiveFactory() {
                 // when we are destroying the node later.
                 var $$attachId = currentElement.$$attachId = ngMessagesCtrl.getAttachId();
 
-                // in the event that the parent element is destroyed
-                // by any other structural directive then it's time
+                // in the event that the element or a parent element is destroyed
+                // by another structural directive then it's time
                 // to deregister the message from the controller
                 currentElement.on('$destroy', function() {
                   if (currentElement && currentElement.$$attachId === $$attachId) {

test/ngMessages/messagesSpec.js

@@ -485,6 +485,126 @@ describe('ngMessages', function() {
     });
   });
 
+  describe('ngMessage nested nested inside elements', function() {
+
+    it('should not crash or leak memory when the messages are transcluded, the first message is ' +
+      'visible, and ngMessages is removed by ngIf', function() {
+
+      module(function($compileProvider) {
+        $compileProvider.directive('messageWrap', function() {
+          return {
+            transclude: true,
+            scope: {
+              col: '=col'
+            },
+            template: '<div ng-messages="col"><ng-transclude></ng-transclude></div>'
+          };
+        });
+      });
+
+      inject(function($rootScope, $compile) {
+
+        element = $compile('<div><div ng-if="show"><div message-wrap col="col">' +
+                           '        <div ng-message="a">A</div>' +
+                           '        <div ng-message="b">B</div>' +
+                           '</div></div></div>')($rootScope);
+
+        $rootScope.$apply(function() {
+          $rootScope.show = true;
+          $rootScope.col = {
+            a: true,
+            b: true
+          };
+        });
+
+        expect(messageChildren(element).length).toBe(1);
+        expect(trim(element.text())).toEqual('A');
+
+        $rootScope.$apply('show = false');
+
+        expect(messageChildren(element).length).toBe(0);
+      });
+    });
+
+
+    it('should not crash when the first of two nested messages is removed', function() {
+      inject(function($rootScope, $compile) {
+
+        element = $compile(
+          '<div ng-messages="col">' +
+            '<div class="wrapper">' +
+              '<div remove-me ng-message="a">A</div>' +
+              '<div ng-message="b">B</div>' +
+            '</div>' +
+          '</div>'
+        )($rootScope);
+
+        $rootScope.$apply(function() {
+          $rootScope.col = {
+            a: true,
+            b: false
+          };
+        });
+
+        expect(messageChildren(element).length).toBe(1);
+        expect(trim(element.text())).toEqual('A');
+
+        var ctrl = element.controller('ngMessages');
+        var deregisterSpy = spyOn(ctrl, 'deregister').and.callThrough();
+
+        var nodeA = element[0].querySelector('[ng-message="a"]');
+        jqLite(nodeA).remove();
+        $rootScope.$digest(); // The next digest triggers the error
+
+        // Make sure removing the element triggers the deregistration in ngMessages
+        expect(trim(deregisterSpy.calls.mostRecent().args[0].nodeValue)).toBe('ngMessage: a');
+        expect(messageChildren(element).length).toBe(0);
+      });
+    });
+
+
+    it('should not crash, but show deeply nested messages correctly after a message ' +
+      'has been removed', function() {
+      inject(function($rootScope, $compile) {
+
+        element = $compile(
+          '<div ng-messages="col" ng-messages-multiple>' +
+            '<div class="another-wrapper">' +
+              '<div ng-message="a">A</div>' +
+              '<div class="wrapper">' +
+                '<div ng-message="b">B</div>' +
+                '<div ng-message="c">C</div>' +
+              '</div>' +
+              '<div ng-message="d">D</div>' +
+            '</div>' +
+          '</div>'
+        )($rootScope);
+
+        $rootScope.$apply(function() {
+          $rootScope.col = {
+            a: true,
+            b: true
+          };
+        });
+
+        expect(messageChildren(element).length).toBe(2);
+        expect(trim(element.text())).toEqual('AB');
+
+        var ctrl = element.controller('ngMessages');
+        var deregisterSpy = spyOn(ctrl, 'deregister').and.callThrough();
+
+        var nodeB = element[0].querySelector('[ng-message="b"]');
+        jqLite(nodeB).remove();
+        $rootScope.$digest(); // The next digest triggers the error
+
+        // Make sure removing the element triggers the deregistration in ngMessages
+        expect(trim(deregisterSpy.calls.mostRecent().args[0].nodeValue)).toBe('ngMessage: b');
+        expect(messageChildren(element).length).toBe(1);
+        expect(trim(element.text())).toEqual('A');
+      });
+    });
+  });
+
   describe('when including templates', function() {
     they('should work with a dynamic collection model which is managed by ngRepeat',
       {'<div ng-messages-include="...">': '<div ng-messages="item">' +
@@ -691,6 +811,37 @@ describe('ngMessages', function() {
       expect(trim(element.text())).toEqual("C");
     }));
 
+
+    it('should properly detect a previous message, even if it was registered later',
+      inject(function($compile, $rootScope, $templateCache) {
+        $templateCache.put('include.html', '<div ng-message="a">A</div>');
+        var html =
+            '<div ng-messages="items">' +
+              '<div ng-include="\'include.html\'"></div>' +
+              '<div ng-message="b">B</div>' +
+              '<div ng-message="c">C</div>' +
+            '</div>';
+
+        element = $compile(html)($rootScope);
+        $rootScope.$apply('items = {b: true, c: true}');
+
+        expect(element.text()).toBe('B');
+
+        var ctrl = element.controller('ngMessages');
+        var deregisterSpy = spyOn(ctrl, 'deregister').and.callThrough();
+
+        var nodeB = element[0].querySelector('[ng-message="b"]');
+        jqLite(nodeB).remove();
+
+        // Make sure removing the element triggers the deregistration in ngMessages
+        expect(trim(deregisterSpy.calls.mostRecent().args[0].nodeValue)).toBe('ngMessage: b');
+
+        $rootScope.$apply('items.a = true');
+
+        expect(element.text()).toBe('A');
+      })
+    );
+
   });
 
   describe('when multiple', function() {