fix(ng:include): prevent race conditions by ignoring stale http callbacks

This fix is similar to what I've done in ng:view, if a new template has been requested before the
callback for the previous template returned, ignore it. Otherwise weird race conditions happen
and users might end up getting the content for the previous include rendered instead of the most
recent one.

2 files changed, 54 insertions, 31 deletions

diff --git a/src/widgets.js b/src/widgets.js
index 618de04d..155b8c08 100644
--- a/src/widgets.js
+++ b/src/widgets.js
@@ -94,42 +94,38 @@ angularWidget('ng:include', function(element){
     function($http,   $templateCache,   $autoScroll,   element) {
       var scope = this,
           changeCounter = 0,
-          releaseScopes = [],
-          childScope,
-          oldScope;
+          childScope;
 
       function incrementChange() { changeCounter++;}
       this.$watch(srcExp, incrementChange);
-      this.$watch(function(scope){
-        var newScope = scope.$eval(scopeExp);
-        if (newScope !== oldScope) {
-          oldScope = newScope;
-          incrementChange();
-        }
-      });
-      this.$watch(function() {return changeCounter;}, function(scope) {
+      this.$watch(function() {
+        var includeScope = scope.$eval(scopeExp);
+        if (includeScope) return includeScope.$id;
+      }, incrementChange);
+      this.$watch(function() {return changeCounter;}, function(scope, newChangeCounter) {
         var src = scope.$eval(srcExp),
             useScope = scope.$eval(scopeExp);
 
         function clearContent() {
-          childScope = null;
-          element.html('');
+          // if this callback is still desired
+          if (newChangeCounter === changeCounter) {
+            if (childScope) childScope.$destroy();
+            childScope = null;
+            element.html('');
+          }
         }
 
-        while(releaseScopes.length) {
-          releaseScopes.pop().$destroy();
-        }
         if (src) {
           $http.get(src, {cache: $templateCache}).success(function(response) {
-            element.html(response);
-            if (useScope) {
-              childScope = useScope;
-            } else {
-              releaseScopes.push(childScope = scope.$new());
+            // if this callback is still desired
+            if (newChangeCounter === changeCounter) {
+              element.html(response);
+              if (childScope) childScope.$destroy();
+              childScope = useScope ? useScope : scope.$new();
+              compiler.compile(element)(childScope);
+              $autoScroll();
+              scope.$eval(onloadExp);
             }
-            compiler.compile(element)(childScope);
-            $autoScroll();
-            scope.$eval(onloadExp);
           }).error(clearContent);
         } else {
           clearContent();
@@ -574,7 +570,10 @@ angularWidget('ng:view', function(element) {
         var template = $route.current && $route.current.template;
 
         function clearContent() {
-          element.html('');
+          // ignore callback if another route change occured since
+          if (newChangeCounter == changeCounter) {
+            element.html('');
+          }
         }
 
         if (template) {
diff --git a/test/widgetsSpec.js b/test/widgetsSpec.js
index a3f7282b..d2867d09 100644
--- a/test/widgetsSpec.js
+++ b/test/widgetsSpec.js
@@ -67,7 +67,7 @@ describe('widget', function() {
     it('should include on external file', inject(putIntoCache('myUrl', '{{name}}'),
         function($rootScope, $compile, $browser) {
       var element = jqLite('<ng:include src="url" scope="childScope"></ng:include>');
-      var element = $compile(element)($rootScope);
+      element = $compile(element)($rootScope);
       $rootScope.childScope = $rootScope.$new();
       $rootScope.childScope.name = 'misko';
       $rootScope.url = 'myUrl';
@@ -81,7 +81,7 @@ describe('widget', function() {
           putIntoCache('myUrl', '{{name}}'),
           function($rootScope, $compile, $browser) {
       var element = jqLite('<ng:include src="url" scope="childScope"></ng:include>');
-      var element = $compile(element)($rootScope);
+      element = $compile(element)($rootScope);
       $rootScope.childScope = $rootScope.$new();
       $rootScope.childScope.name = 'igor';
       $rootScope.url = 'myUrl';
@@ -100,7 +100,7 @@ describe('widget', function() {
     it('should allow this for scope', inject(putIntoCache('myUrl', '{{"abc"}}'),
           function($rootScope, $compile, $browser) {
       var element = jqLite('<ng:include src="url" scope="this"></ng:include>');
-      var element = $compile(element)($rootScope);
+      element = $compile(element)($rootScope);
       $rootScope.url = 'myUrl';
       $rootScope.$digest();
       $browser.defer.flush();
@@ -119,7 +119,7 @@ describe('widget', function() {
         putIntoCache('myUrl', 'my partial'),
         function($rootScope, $compile, $browser) {
       var element = jqLite('<ng:include src="url" onload="loaded = true"></ng:include>');
-      var element = $compile(element)($rootScope);
+      element = $compile(element)($rootScope);
 
       expect($rootScope.loaded).not.toBeDefined();
 
@@ -135,7 +135,7 @@ describe('widget', function() {
     it('should destroy old scope', inject(putIntoCache('myUrl', 'my partial'),
           function($rootScope, $compile, $browser) {
       var element = jqLite('<ng:include src="url"></ng:include>');
-      var element = $compile(element)($rootScope);
+      element = $compile(element)($rootScope);
 
       expect($rootScope.$$childHead).toBeFalsy();
 
@@ -199,6 +199,30 @@ describe('widget', function() {
       $browser.defer.flush();
       expect(element.text()).toBe('my partial');
     }));
+
+    it('should discard pending xhr callbacks if a new template is requested before the current ' +
+        'finished loading', inject(function($rootScope, $compile, $httpBackend) {
+      var element = jqLite("<ng:include src='templateUrl'></ng:include>"),
+          log = [];
+
+      $rootScope.templateUrl = 'myUrl1';
+      $rootScope.logger = function(msg) {
+        log.push(msg);
+      }
+      $compile(element)($rootScope);
+      expect(log.join('; ')).toEqual('');
+
+      $httpBackend.expect('GET', 'myUrl1').respond('<div>{{logger("url1")}}</div>');
+      $rootScope.$digest();
+      expect(log.join('; ')).toEqual('');
+      $rootScope.templateUrl = 'myUrl2';
+      $httpBackend.expect('GET', 'myUrl2').respond('<div>{{logger("url2")}}</div>');
+      $rootScope.$digest();
+      $httpBackend.flush(); // now that we have two requests pending, flush!
+
+      expect(log.join('; ')).toEqual('url2; url2'); // it's here twice because we go through at
+                                                    // least two digest cycles
+    }));
   }));
 
 
@@ -645,7 +669,7 @@ describe('widget', function() {
       $location.path('/bar');
       $httpBackend.expect('GET', 'myUrl2').respond('<div>{{1+1}}</div>');
       $rootScope.$digest();
-      $httpBackend.flush(); // now that we have to requests pending, flush!
+      $httpBackend.flush(); // now that we have two requests pending, flush!
 
       expect($rootScope.$element.text()).toEqual('2');
     }));