fix(ngMessages): don't crash when first transcluded message is removed

[Narretz]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)

Other information:

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 only messages with an index lower than the starting message are considered
previous elements.

Fixes #14183

@gkalpak you triaged the original issue, do you mind reviewing this?

[gkalpak]

This line is too long. Please wrap at 100 chars 😁

[Narretz]

maxlength in jshint is 200, but I'll do it for you :)

[gkalpak]

I believe this fix could result in different behavior, since now the order of messages is determined by the order of registration and not their place in the DOM. E.g.:

<ng-messages for="someObj">
  <include-messages>
    <!-- Some directive with a `templateUrl` containing:
      <ng-message when="error1">...</ng-message>
      <ng-message when="error2">...</ng-message>
    -->
  </include-messages>
  <ng-message when="error3">...</ng-message>
  <ng-message when="error4">...</ng-message>
</ng-messages>

@Narretz, can you confirm that this is indeed tha case ?
If yes, can we avoid it ?

[Narretz]

@gkalpak Thanks for looking into this. As far as I can see, I don't think the order is compromised. I've added a test to make sure it doesn't happen. Does the test reflect the case you had in mind? You can also add another test, don't be shy ;>

I've also fixed a memory leak with ngTransclude in the test that happens when ngIf destroys the ngMessages container. If only one message is currently visible, ngMessages deregisters this message and then tries to render the second message. The second message never gets its destroy handler triggered, but somehow holds onto the transclusion scope, which can't be collected then. I'm not sure if that's an issue in the real world, though.

[gkalpak]

The problem becomes obvious when removing one of the messages. Here is a failing testcase:

  it('should properly detect the 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 nodeB = element[0].querySelector('[ng-message="b"]');
      angular.element(nodeB).remove();
      $rootScope.$apply('items.a = true');

      expect(element.text()).toBe('A');
    })
  );

(Removing the element like this is kind of contrived, but I am pretty sure the same issues could be reproduced with ngIf and a remote template for example.)

[Narretz]

Thanks for the test!
This is definitely a problem. I've pushed a new commit that makes this and the original test pass, but it only works if there's just one level of nesting when you remove the element directly:

        '<div ng-messages="col">' +
          '<div class="wrapper">' +
            '<div remove-me ng-message="required">Fill in the text field.</div>' +
            '<div ng-message="extra">Extra error message.</div>' +
          '</div>' +
        '</div>'

More levels of nesting still produce the same problem:

   '<div ng-messages="col">' +
      '<div class="another-wrapper">' +
        '<div class="wrapper">' +
          '<div remove-me ng-message="required">Fill in the text field.</div>' +
          '<div ng-message="extra">Extra error message.</div>' +
        '</div>' +
        '<div ng-message="different">Different error message.</div>' +
      '</div>' +
    '</div>'

It's quite difficult to maintain a notion of previous element, when the nesting becomes more complex.
But the problem will only happen if someone removes a single message from the DOM, something that shouldn't be necessary. The original issue - removing the complete ngMessages element with an ngIF - is already fixed by removing all message nodes when the ngMessages element is destroyed.
So we basically two options - just fix the original issue, or improve the querying for previousNodes. Maybe a top-down search is better then searching from the comment element itself.

[gkalpak]

TBH, I'm still not sure what caused the original issue. I read your description in the commit message:

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.

But I haven't looked into why/how can the 2nd message be identified as previous to the 1st one (is it because it becomes the ctrl.head) ? And even then, why does the 1st message become the next for the 2nd one ?
Maybe we could fix that logic.

[Narretz]

So the problem is in this function:

angular.js/src/ngMessages/messages.js

Line 444 in aa077e8

function findPreviousMessage(parent, comment) {

When you have DOM like this:

    '<div ng-messages="col">' +
      '<div class="wrapper">' +
        '<div remove-me ng-message="required">Fill in the text field.</div>' +
        '<div ng-message="extra">Extra error message.</div>' +
      '</div>' +
    '</div>'

and the "required" message element gets removed, findPreviousMessage looks upwards in the DOM to find a previous message. Since required has no previousSibling, the wrapper element is the next prevNode:

angular.js/src/ngMessages/messages.js

Line 459 in aa077e8

prevNode = prevNode.previousSibling || prevNode.parentNode;

. The code then assigns the last child of the wrapper as the prevNode, and since this is the "extra" message it is mis-identied as a previous node. Later, the "required" message is set as the next message for the "required" message:

angular.js/src/ngMessages/messages.js

Line 485 in aa077e8

match.next = messageNode.next;

The problem is really that the nesting inside ngMessages can be arbitrarily complicated, but this wasn't really planned, because you should only use ng-message and ng-message-include. I can understand if someone has an ng-transclude, but more nesting shouldn't really be necessary.

Searching for previous nodes can definitely be improved, but I'm not sure if it's worthwhile. One possibility is to start the search from the ngMessages element, and stop when you find the passed in the comment element.

[gkalpak]

From a quick look, it seems that the actual bug is on messages.js#L455: It shouldn't look into the comment element's parent, because it has already looked in there (since it started at comment.

I haven't tried it, but I believe that initializing parentLookup with comment.parentNode on messages.js#L446 should fix the bug. E.g.:

var parentLookup = [comment.parentNode];

[Narretz]

This fix works if there's only one level of nesting. It's the same as doing:

             if (prevNode.childNodes.length &&
                prevNode !== comment.parentNode && // this is new
                parentLookup.indexOf(prevNode) == -1
              ) {
               parentLookup.push(prevNode);
               prevNode = prevNode.childNodes[prevNode.childNodes.length - 1];
             } else {
               prevNode = prevNode.previousSibling || prevNode.parentNode;
             }

But both fixes don't work if there's a DOM like this:

    '<div ng-messages="col">' +
      '<div class="another-wrapper">' +
        '<div class="wrapper">' +
          '<div remove-me ng-message="required">Fill in the text field.</div>' +
          '<div ng-message="extra">Extra error message.</div>' +
        '</div>' +
        '<div ng-message="different">Different error message.</div>' +
      '</div>' +
    '</div>'

This is probably a very unusual case, but this will also cause an infinite loop, so we should probably fix it. Maybe merge the patch now and then work on this later?

[gkalpak]

Taking a more careful look, the problem seems to be that once we traverse all previousSiblings, we go to the parentNode and from there we start at the last childNode (which is essentially a nextSibling to the original comment).

Basically, if we reach at a parentNode after having examined one of its childNodes, we should never go back in, since it is guaranteed that we won't find an actual "previous message" in there.

This is how findPreviousMessage should look like:

function findPreviousMessage(parent, comment) {
  var prevNode = comment;
  var parentLookup = [];
  while (prevNode && prevNode !== parent) {
    var prevKey = prevNode.$$ngMessageNode;
    if (prevKey && prevKey.length) {
      return messages[prevKey];
    }

    // dive deeper into the DOM and examine its children for any ngMessage
    // comments that may be in an element that appears deeper in the list
    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.parentNode;
      parentLookup.push(prevNode);
    }
  }
}

[Narretz]

Taking a more careful look, the problem seems to be that once we traverse all previousSiblings, we go to the parentNode and from there we start at the last childNode (which is essentially a nextSibling to the original comment.

That's what I was trying to explain before. But it's probably easier experienced than explained ;)

And your solution seems to work!

[Narretz]

I'Ve updated the PR with your fix, PTAL

[gkalpak]

Is this test failing without the fix ?

[Narretz]

Actually ... since I converted the tests to remove the element via .remove() directly instead of inside of a watcher, the tests don't fail anymore without the patch. This issue is killing me ...

[Narretz]

That's because the $element.on('$destroy') is only triggered by a jqLite(element).remove, not when a node is removed via normal DOM apis. Interesting, didn't know this,

[gkalpak]

I left a couple of minor comments (plus the errors caught by Travis - good job Travis).

LGTM otherwise 👍

[Narretz]

I fixed up the tests, so that they actually fail without the fix, and put them into their own describe. I think that's it. This issue already made my shortlist for "worst issue of 2016"

[gkalpak]

LGTM

_{(And I also think this issue was fun 😃)}