NgMessages: Messages that are not attached to the DOM before being dynamically removed don't deregister from the parent controller

[CodySchaaf]

I'm submitting a ...

• bug report
• feature request
• other

Current behavior:
When messages are removed from the dom via ng-if, if they haven't been rendered by ngMessages they do not unsubscribe from the controller. This causes a memory leak, and in my application errors which lead to infinite digests.

See Plnkr for repro:

https://plnkr.co/edit/vG7DxmXLjuzfZUF11ypB?p=preview

Expected / new behavior:
The messages that are removed should unsubscribe.

Minimal reproduction of the problem with instructions:
Add an ngMessages directive that adds a template with an ng-if to remove errors on input focus. This is done to fix a jaws issue where errors are continually repeated as a user is typing when they are only hidden via css.

https://plnkr.co/edit/vG7DxmXLjuzfZUF11ypB?p=preview

AngularJS version: 1.x.y
Still an issue.

Browser:
All

Anything else:
I have started working on a solution here CodySchaaf@20bbb17 , but I currently have failing tests. I will continue to play around with it but, any suggestions would be appreciated. I suspect it has something to do with the issue that was originally solved with the $$attachId.

[Narretz]

Hi @CodySchaaf thanks for reporting. I don't know right now what the problem is, but I suspect it could have to do with transclusion, as your directive, ngIf and ngMessage all use it.
If you open a PR from your branch, I can look into the CI to see what fails. You can also add a test case for this behavior, although I wonder if there's a better way to handle this JAWS problem.

[petebacondarwin]

I have played around with the Plunker and come up with this: https://plnkr.co/edit/1Ro6zHXRcmMXFJLWerYJ?p=preview
In this case there is no memory leak, as far as I can tell.

I think that @Narretz is correct that the leakage is likely due to the fact that in your Plunker you are overloading the ngMessages directive with your own, which is creating a new scope and using transclusion.

[CodySchaaf]

@petebacondarwin Yeah the memory leak is being highlighted by the transclude, but that is only because of the bug where messages that aren't attached aren't getting a destroy listener registered since the registration happens only in the attach method. This would also be an issue for ng-repeats ect. that were added inside ng-messages.

I opened a pull request #16404 for help with the failing tests. My change was to ensure that a destroy listener is always registered, but I think the breaking tests have something to do with the issue that was originally solved with the $$attachId

[petebacondarwin]

@CodySchaaf - can you create a reproduction of the bug that doesn't involve overloading the ngMessages directive?

[CodySchaaf]

@petebacondarwin - Sure, here is the most straight forward version I could think of https://plnkr.co/edit/MKwIiaKl2NJ5AdJ4nuyn?p=preview

[petebacondarwin]

Cool, I will take a look at your solution.