fix($timeout): clean deferreds immediately after callback exec/cancel

Make sure $timeout callbacks are forgotten about immediately after
execution or cancellation.

Previously when passing invokeApply=false, the cleanup used $q and so
would be pending until the next $digest was triggered. This does not
make a large functional difference, but can be very visible when
looking at memory consumption of an app or debugging around the
$$asyncQueue - these callbacks can have a big retaining tree.

2 files changed, 55 insertions, 5 deletions

diff --git a/src/ng/timeout.js b/src/ng/timeout.js
index 81d09e89..6cb62d7a 100644
--- a/src/ng/timeout.js
+++ b/src/ng/timeout.js
@@ -45,17 +45,15 @@ function $TimeoutProvider() {
           deferred.reject(e);
           $exceptionHandler(e);
         }
+        finally {
+          delete deferreds[promise.$$timeoutId];
+        }
 
         if (!skipApply) $rootScope.$apply();
       }, delay);
 
-      cleanup = function() {
-        delete deferreds[promise.$$timeoutId];
-      };
-
       promise.$$timeoutId = timeoutId;
       deferreds[timeoutId] = deferred;
-      promise.then(cleanup, cleanup);
 
       return promise;
     }
@@ -77,6 +75,7 @@ function $TimeoutProvider() {
     timeout.cancel = function(promise) {
       if (promise && promise.$$timeoutId in deferreds) {
         deferreds[promise.$$timeoutId].reject('canceled');
+        delete deferreds[promise.$$timeoutId];
         return $browser.defer.cancel(promise.$$timeoutId);
       }
       return false;
diff --git a/test/ng/timeoutSpec.js b/test/ng/timeoutSpec.js
index 5ee99d98..832528e9 100644
--- a/test/ng/timeoutSpec.js
+++ b/test/ng/timeoutSpec.js
@@ -68,6 +68,27 @@ describe('$timeout', function() {
   }));
 
 
+  it('should forget references to deferreds when callback called even if skipApply is true',
+      inject(function($timeout, $browser) {
+    // $browser.defer.cancel is only called on cancel if the deferred object is still referenced
+    var cancelSpy = spyOn($browser.defer, 'cancel').andCallThrough();
+
+    var promise1 = $timeout(function() {}, 0, false);
+    var promise2 = $timeout(function() {}, 100, false);
+    expect(cancelSpy).not.toHaveBeenCalled();
+
+    $timeout.flushNext(0);
+
+    // Promise1 deferred object should already be removed from the list and not cancellable
+    $timeout.cancel(promise1);
+    expect(cancelSpy).not.toHaveBeenCalled();
+
+    // Promise2 deferred object should not have been called and should be cancellable
+    $timeout.cancel(promise2);
+    expect(cancelSpy).toHaveBeenCalled();
+  }));
+
+
   describe('exception handling', function() {
 
     beforeEach(module(function($exceptionHandlerProvider) {
@@ -106,6 +127,20 @@ describe('$timeout', function() {
 
       expect(log).toEqual('error: Some Error');
     }));
+
+
+    it('should forget references to relevant deferred even when exception is thrown',
+        inject(function($timeout, $browser) {
+      // $browser.defer.cancel is only called on cancel if the deferred object is still referenced
+      var cancelSpy = spyOn($browser.defer, 'cancel').andCallThrough();
+
+      var promise = $timeout(function() { throw "Test Error"; }, 0, false);
+      $timeout.flush();
+
+      expect(cancelSpy).not.toHaveBeenCalled();
+      $timeout.cancel(promise);
+      expect(cancelSpy).not.toHaveBeenCalled();
+    }));
   });
 
 
@@ -147,5 +182,21 @@ describe('$timeout', function() {
     it('should not throw a runtime exception when given an undefined promise', inject(function($timeout) {
       expect($timeout.cancel()).toBe(false);
     }));
+
+
+    it('should forget references to relevant deferred', inject(function($timeout, $browser) {
+      // $browser.defer.cancel is only called on cancel if the deferred object is still referenced
+      var cancelSpy = spyOn($browser.defer, 'cancel').andCallThrough();
+
+      var promise = $timeout(function() {}, 0, false);
+
+      expect(cancelSpy).not.toHaveBeenCalled();
+      $timeout.cancel(promise);
+      expect(cancelSpy).toHaveBeenCalledOnce();
+
+      // Promise deferred object should already be removed from the list and not cancellable again
+      $timeout.cancel(promise);
+      expect(cancelSpy).toHaveBeenCalledOnce();
+    }));
   });
 });