refactor($scope): prevent multiple calls to listener on $destroy

[lgalfaso]

Prevent isolated scopes from having listeners that get called multiple times when on $destroy

[btford]

I don't think this should be documented as a refactor. I think this should be a fix, since the behavior before is kind of unexpected.

[lgalfaso]

@btford the reason this is marked as a refactor is #8504 (comment)

[btford]

ah, okay. makes sense.

[caitp]

@lgalfaso if I understand your comment from #9199, this should fix the ngSwitch issue too yeah?

If that's the case, could we have a test which shows that the fix is working? (Yes I know there is a more general test case --- I just think it would be good to show how it's applicable to the raised issue)

[lgalfaso]

@caitp this PR has two commits 47f6d46 e21c2f5

The second one has a test that is exactly as the issue posted. The reason why these are in just one PR is that the second commit needs the first to work properly

[lgalfaso]

@petebacondarwin updated the patch

[petebacondarwin]

OK. I have one more thing to say here :-) (Sorry this is dragging out.)

It turns out that now that #9281 has landed, the second commit is enough on its own to ensure that the memory leak is resolved. The refactoring is not needed.

How about we land the mem leak fix and then continue the discussion about the refactoring separately?

If we accepted that double destroy is something that apps should be able to cope with then we would save more processing and memory by not wiring up all these destroy handlers on every scope.

[lgalfaso]

I think that the fix for the leak, as it is in this patch, would not fix the leak without the first patch. The patch for the leak checks if the scope it was asked to continue the compilation is marked with $destroyed, and if that is the case then stop. If in the scope hierarchy between the scope that was actually destroyed and the scope that is used to link the there is an isolated scope, then the scope that is being asked to continue with the linking would not have the $destroyed property (as the isolated scope would prevent this). The first patch adds this behavior.

I am ok with still having the double destroy, but the second patch (without the first patch) would need to go up the hierarchy to know if the received scope is in a branch that was actually destroyed or not

[petebacondarwin]

If you only add the "fix" commit and not the "refactor" commit to master the tests pass. Moreover the tests still pass even if you remove https://github.com/angular/angular.js/pull/9079/files#diff-a732922b631efed1b9f33a24082ae0dbR2042/.
So either the test is not strong enough or the single check for $destroyed is enough to fix the leak.

[lgalfaso]

you are right, the test is not strong enough. Let me update it with a stronger test

[lgalfaso]

Updated the test with a stronger version that shows the leak when the double destroy patch was not present. If it looks good, then I will split this into two clean commits

[lgalfaso]

Updated the PR by having this in only two commits and added some comments to make it clear what is going on

[tiff]

The Changelog says that this fix also made it into v1.2.26. So I just downloaded it from angularjs.org and didn't find this change in it...

Edit: Probably because of this commit

[petebacondarwin]

Ah yes, it was reverted so it should not have made it into the CHANGELOG. Sorry