From 1ec77d0de7f64d8cd395dcd8ea1b653b6fd5d35d Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 20 Jun 2017 22:12:17 -0700 Subject: [PATCH] fix($timeout/$interval): do not trigger a digest on cancel Previously `.catch(noop)` was used on the rejected timeout/interval to prevent an unhandled rejection error. However this would schedule a deferred task to run the `noop`. If the cancelling was outside a digest this could cause a new digest such as with the ng-model `debounce` option. Fixes #16057 --- src/.eslintrc.json | 3 +++ src/ng/interval.js | 2 +- src/ng/q.js | 18 ++++++++++++++---- src/ng/timeout.js | 2 +- test/ng/directive/ngModelOptionsSpec.js | 6 ++++++ test/ng/intervalSpec.js | 11 +++++++++++ test/ng/timeoutSpec.js | 11 +++++++++++ test/ngAnimate/animateCssSpec.js | 3 +-- 8 files changed, 48 insertions(+), 8 deletions(-) diff --git a/src/.eslintrc.json b/src/.eslintrc.json index 3f8163dcc9af..64e8b866e6c8 100644 --- a/src/.eslintrc.json +++ b/src/.eslintrc.json @@ -168,6 +168,9 @@ /* ng/compile.js */ "directiveNormalize": false, + /* ng/q.js */ + "markQExceptionHandled": false, + /* ng/directive/directives.js */ "ngDirective": false, diff --git a/src/ng/interval.js b/src/ng/interval.js index 673df2d4ef51..e5f2538f8b7b 100644 --- a/src/ng/interval.js +++ b/src/ng/interval.js @@ -190,7 +190,7 @@ function $IntervalProvider() { interval.cancel = function(promise) { if (promise && promise.$$intervalId in intervals) { // Interval cancels should not report as unhandled promise. - intervals[promise.$$intervalId].promise.catch(noop); + markQExceptionHandled(intervals[promise.$$intervalId].promise); intervals[promise.$$intervalId].reject('canceled'); $window.clearInterval(promise.$$intervalId); delete intervals[promise.$$intervalId]; diff --git a/src/ng/q.js b/src/ng/q.js index 52dd39c8e037..c2fb12361c29 100644 --- a/src/ng/q.js +++ b/src/ng/q.js @@ -351,7 +351,7 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) { state.pending = undefined; try { for (var i = 0, ii = pending.length; i < ii; ++i) { - state.pur = true; + markQStateExceptionHandled(state); promise = pending[i][0]; fn = pending[i][state.status]; try { @@ -378,8 +378,8 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) { // eslint-disable-next-line no-unmodified-loop-condition while (!queueSize && checkQueue.length) { var toCheck = checkQueue.shift(); - if (!toCheck.pur) { - toCheck.pur = true; + if (!isStateExceptionHandled(toCheck)) { + markQStateExceptionHandled(toCheck); var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value); if (isError(toCheck.value)) { exceptionHandler(toCheck.value, errorMessage); @@ -391,7 +391,7 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) { } function scheduleProcessQueue(state) { - if (errorOnUnhandledRejections && !state.pending && state.status === 2 && !state.pur) { + if (errorOnUnhandledRejections && !state.pending && state.status === 2 && !isStateExceptionHandled(state)) { if (queueSize === 0 && checkQueue.length === 0) { nextTick(processChecks); } @@ -671,3 +671,13 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) { return $Q; } + +function isStateExceptionHandled(state) { + return !!state.pur; +} +function markQStateExceptionHandled(state) { + state.pur = true; +} +function markQExceptionHandled(q) { + markQStateExceptionHandled(q.$$state); +} diff --git a/src/ng/timeout.js b/src/ng/timeout.js index 4f7b26be6c06..5c22c8b9ba55 100644 --- a/src/ng/timeout.js +++ b/src/ng/timeout.js @@ -85,7 +85,7 @@ function $TimeoutProvider() { timeout.cancel = function(promise) { if (promise && promise.$$timeoutId in deferreds) { // Timeout cancels should not report an unhandled promise. - deferreds[promise.$$timeoutId].promise.catch(noop); + markQExceptionHandled(deferreds[promise.$$timeoutId].promise); deferreds[promise.$$timeoutId].reject('canceled'); delete deferreds[promise.$$timeoutId]; return $browser.defer.cancel(promise.$$timeoutId); diff --git a/test/ng/directive/ngModelOptionsSpec.js b/test/ng/directive/ngModelOptionsSpec.js index 62ff8779f3b2..f4142ae217ee 100644 --- a/test/ng/directive/ngModelOptionsSpec.js +++ b/test/ng/directive/ngModelOptionsSpec.js @@ -459,8 +459,14 @@ describe('ngModelOptions', function() { $rootScope.$watch(watchSpy); helper.changeInputValueTo('a'); + $timeout.flush(2000); + expect(watchSpy).not.toHaveBeenCalled(); + + helper.changeInputValueTo('b'); + $timeout.flush(2000); expect(watchSpy).not.toHaveBeenCalled(); + helper.changeInputValueTo('c'); $timeout.flush(10000); expect(watchSpy).toHaveBeenCalled(); }); diff --git a/test/ng/intervalSpec.js b/test/ng/intervalSpec.js index 039dc78121e8..47281429e0b2 100644 --- a/test/ng/intervalSpec.js +++ b/test/ng/intervalSpec.js @@ -339,6 +339,17 @@ describe('$interval', function() { inject(function($interval) { expect($interval.cancel()).toBe(false); })); + + + it('should not trigger digest when cancelled', inject(function($interval, $rootScope, $browser) { + var watchSpy = jasmine.createSpy('watchSpy'); + $rootScope.$watch(watchSpy); + + var t = $interval(); + $interval.cancel(t); + expect(function() {$browser.defer.flush();}).toThrowError('No deferred tasks to be flushed'); + expect(watchSpy).not.toHaveBeenCalled(); + })); }); describe('$window delegation', function() { diff --git a/test/ng/timeoutSpec.js b/test/ng/timeoutSpec.js index f82905fc1668..648c39663c0d 100644 --- a/test/ng/timeoutSpec.js +++ b/test/ng/timeoutSpec.js @@ -299,5 +299,16 @@ describe('$timeout', function() { $timeout.cancel(promise); expect(cancelSpy).toHaveBeenCalledOnce(); })); + + + it('should not trigger digest when cancelled', inject(function($timeout, $rootScope, $browser) { + var watchSpy = jasmine.createSpy('watchSpy'); + $rootScope.$watch(watchSpy); + + var t = $timeout(); + $timeout.cancel(t); + expect(function() {$browser.defer.flush();}).toThrowError('No deferred tasks to be flushed'); + expect(watchSpy).not.toHaveBeenCalled(); + })); }); }); diff --git a/test/ngAnimate/animateCssSpec.js b/test/ngAnimate/animateCssSpec.js index 588b6ea8e0c1..b274de05f18a 100644 --- a/test/ngAnimate/animateCssSpec.js +++ b/test/ngAnimate/animateCssSpec.js @@ -1328,8 +1328,7 @@ describe('ngAnimate $animateCss', function() { animator.end(); expect(element.data(ANIMATE_TIMER_KEY)).toBeUndefined(); - $timeout.flush(); - expect(function() {$timeout.verifyNoPendingTasks();}).not.toThrow(); + $timeout.verifyNoPendingTasks(); })); });