fix(ngMessages): prevent race condition with ngAnimate

petebacondarwin · petebacondarwin · commit 8366622bed00 · 2015-09-22T17:54:04.000+01:00
If `ngMessage` tried to add a message back in that was about to be removed after an animation, the NgMessageController got confused and tried to detach the newly added message, when the pending node was destroyed. This change applies a unique `attachId` to the message object and its DOM node when it is attached. This is then checked when a DOM node is being destroyed to prevent unwanted calls to `detach`. Closes angular#12856 Closes angular#12903
diff --git a/src/ngMessages/messages.js b/src/ngMessages/messages.js
@@ -325,6 +325,9 @@ angular.module('ngMessages', [])
        controller: ['$element', '$scope', '$attrs', function($element, $scope, $attrs) {
          var ctrl = this;
          var latestKey = 0;
+         var nextAttachId = 0;
+
+         this.getAttachId = function getAttachId() { return nextAttachId++; };
 
          var messages = this.messages = {};
          var renderLater, cachedCollection;
@@ -636,11 +639,15 @@ function ngMessageDirectiveFactory(restrict) {
                 $animate.enter(elm, null, element);
                 currentElement = elm;
 
+                // Each time we attach this node to a message we get a new id that we can match
+                // 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
                 // to deregister the message from the controller
                 currentElement.on('$destroy', function() {
-                  if (currentElement) {
+                  if (currentElement && currentElement.$$attachId === $$attachId) {
                     ngMessagesCtrl.deregister(commentNode);
                     messageCtrl.detach();
                   }
diff --git a/test/ngMessages/messagesSpec.js b/test/ngMessages/messagesSpec.js
@@ -372,6 +372,50 @@ describe('ngMessages', function() {
     expect(trim(element.text())).toEqual("Enter something");
   }));
 
+  // issue #12856
+  it('should only detach the message object that is associated with the message node being removed',
+    inject(function($rootScope, $compile, $animate) {
+
+    // We are going to spy on the `leave` method to give us control over
+    // when the element is actually removed
+    spyOn($animate, 'leave');
+
+    // Create a basic ng-messages set up
+    element = $compile('<div ng-messages="col">' +
+                       '  <div ng-message="primary">Enter something</div>' +
+                       '</div>')($rootScope);
+
+    // Trigger the message to be displayed
+    $rootScope.col = { primary: true };
+    $rootScope.$digest();
+    expect(messageChildren(element).length).toEqual(1);
+    var oldMessageNode = messageChildren(element)[0];
+
+    // Remove the message
+    $rootScope.col = { primary: undefined };
+    $rootScope.$digest();
+
+    // Since we have spied on the `leave` method, the message node is still in the DOM
+    expect($animate.leave).toHaveBeenCalledOnce();
+    var nodeToRemove = $animate.leave.mostRecentCall.args[0][0];
+    expect(nodeToRemove).toBe(oldMessageNode);
+    $animate.leave.reset();
+
+    // Add the message back in
+    $rootScope.col = { primary: true };
+    $rootScope.$digest();
+
+    // Simulate the animation completing on the node
+    jqLite(nodeToRemove).remove();
+
+    // We should not get another call to `leave`
+    expect($animate.leave).not.toHaveBeenCalled();
+
+    // There should only be the new message node
+    expect(messageChildren(element).length).toEqual(1);
+    var newMessageNode = messageChildren(element)[0];
+    expect(newMessageNode).not.toBe(oldMessageNode);
+  }));
 
   it('should render animations when the active/inactive classes are added/removed', function() {
     module('ngAnimate');
