fix(ngInclude): do not compile template if original scope is destroyed #13543
Conversation
With slow internet connection scope may be destroyed before template is loaded. Previously in this case ngInclude compiled template that leaded to memory leaks and errors in some cases. Closes #13515
| @@ -253,6 +255,8 @@ var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate', | |||
| currentScope.$emit('$includeContentLoaded', src); | |||
| scope.$eval(onloadExp); | |||
| }, function() { | |||
| if (scope.$$destroyed) return; | |||
There was a problem hiding this comment.
looks like this code path is not tested
There was a problem hiding this comment.
Yes, it is not tested. I think it is quite internal logic, the aim is not to perform unnecessary cleanup. By the time when scope is destroyed element should be also removed, so no need to call cleanupLastIncludeContent, and $emit on destroyed scope is useless. There are no changes visible from outside.
If test is required, I can try to spy on scope and element and check that scope.$destroy, scope.$emit and element.remove are not being called.
There was a problem hiding this comment.
I do not think there is a need for a lot of spying, you can have another test just like the one you have with these differences
- same template but without the
ng-if - creating a new scope from
$rootScope, linking the template to it - registering a listener
- destroying the scope
- doing the http flush
There was a problem hiding this comment.
Do you mean listener to $includeContentError event? What should I expect in this case? this event will not fire anyway, because scope removes all listeners on destroy.
Or do you mean that we shouldn't make any expectations, just check that this code works?
There was a problem hiding this comment.
I think you are right, and nothing else is needed.
|
otherwise, LGTM |
|
So would it be merged? As far as I understand we agreed that no more test is needed. |
|
Sorry for the delay, this landed into master |
|
Thanks! Is it possible to merge it to stable branches 1.2, 1.3, 1.4 ? Should I create PRs to relevant branches? |
|
I would be ok with 1.4 (even when I would like to avoid this as we are already in the final stages for a 1.5 release). All the other branches are only receiving security fixes or big regressions fixes. @petebacondarwin WDYT? |
|
Backported to |
|
Thanks guys |
With slow internet connection scope may be destroyed before template is loaded.
Previously in this case ngInclude compiled template that leaded to memory leaks
and errors in some cases.
Closes #13515