From e3b38159642a5d073a6914ff4ad834f3849c4d7f Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Fri, 30 Sep 2016 20:43:31 +0300 Subject: [PATCH] fix($q): treat thrown errors as regular rejections Previously, errors thrown in a promise's `onFulfilled` or `onRejected` handlers were treated in a slightly different manner than regular rejections: They were passed to the `$exceptionHandler()` (in addition to being converted to rejections). The reasoning for this behavior was that an uncaught error is different than a regular rejection, as it can be caused by a programming error, for example. In practice, this turned out to be confusing or undesirable for users, since neither native promises nor any other popular promise library distinguishes thrown errors from regular rejections. (Note: While this behavior does not go against the Promises/A+ spec, it is not prescribed either.) This commit removes the distinction, by skipping the call to `$exceptionHandler()`, thus treating thrown errors as regular rejections. **Note:** Unless explicitly turned off, possibly unhandled rejections will still be caught and passed to the `$exceptionHandler()`, so errors thrown due to programming errors and not otherwise handled (with a subsequent `onRejected` handler) will not go unnoticed. BREAKING CHANGE: Previously, throwing an error from a promise's `onFulfilled` or `onRejection` handlers, would result in passing the error to the `$exceptionHandler()` (in addition to rejecting the promise with the error as reason). Now, a thrown error is treated exactly the same as a regular rejection. This applies to all services/controllers/filters etc that rely on `$q` (including built-in services, such as `$http` and `$route`). For example, `$http`'s `transformRequest/Response` functions or a route's `redirectTo` function as well as functions specified in a route's `resolve` object, will no longer result in a call to `$exceptionHandler()` if they throw an error. Other than that, everything will continue to behave in the same way; i.e. the promises will be rejected, route transition will be cancelled, `$routeChangeError` events will be broadcasted etc. Fixes #3174 Fixes #14745 --- src/ng/compile.js | 12 +++-- src/ng/q.js | 13 ++---- src/ng/templateRequest.js | 84 ++++++++++++++++++---------------- test/ng/compileSpec.js | 76 ++++++++++++++++-------------- test/ng/httpSpec.js | 14 ++---- test/ng/qSpec.js | 63 ++++--------------------- test/ng/templateRequestSpec.js | 20 ++++---- test/ngRoute/routeSpec.js | 32 ++++++++----- 8 files changed, 143 insertions(+), 171 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index df611d315c3d..67496c981e67 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -3052,8 +3052,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { $compileNode.empty(); - $templateRequest(templateUrl) - .then(function(content) { + $templateRequest(templateUrl). + then(function(content) { var compileNode, tempTemplateAttrs, $template, childBoundTranscludeFn; content = denormalizeTemplate(content); @@ -3131,7 +3131,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { childBoundTranscludeFn); } linkQueue = null; - }).catch(noop); + }). + catch(function(error) { + if (error instanceof Error) { + $exceptionHandler(error); + } + }). + catch(noop); return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) { var childBoundTranscludeFn = boundTranscludeFn; diff --git a/src/ng/q.js b/src/ng/q.js index 7b9de29b0d52..55829db5ffd4 100644 --- a/src/ng/q.js +++ b/src/ng/q.js @@ -9,8 +9,8 @@ * A service that helps you run functions asynchronously, and use their return values (or exceptions) * when they are done processing. * - * This is an implementation of promises/deferred objects inspired by - * [Kris Kowal's Q](https://github.com/kriskowal/q). + * This is a [Promises/A+](https://promisesaplus.com/)-compliant implementation of promises/deferred + * objects inspired by [Kris Kowal's Q](https://github.com/kriskowal/q). * * $q can be used in two fashions --- one which is more similar to Kris Kowal's Q or jQuery's Deferred * implementations, and the other which resembles ES6 (ES2015) promises to some degree. @@ -366,7 +366,6 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) { } } catch (e) { deferred.reject(e); - exceptionHandler(e); } } } finally { @@ -417,18 +416,15 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) { } else { this.$$resolve(val); } - }, $$resolve: function(val) { - var then; var that = this; var done = false; try { - if ((isObject(val) || isFunction(val))) then = val && val.then; - if (isFunction(then)) { + if ((isObject(val) || isFunction(val)) && isFunction(val.then)) { this.promise.$$state.status = -1; - then.call(val, resolvePromise, rejectPromise, simpleBind(this, this.notify)); + val.then(resolvePromise, rejectPromise, simpleBind(this, this.notify)); } else { this.promise.$$state.value = val; this.promise.$$state.status = 1; @@ -436,7 +432,6 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) { } } catch (e) { rejectPromise(e); - exceptionHandler(e); } function resolvePromise(val) { diff --git a/src/ng/templateRequest.js b/src/ng/templateRequest.js index e7c1007beddc..1046f0223a92 100644 --- a/src/ng/templateRequest.js +++ b/src/ng/templateRequest.js @@ -60,53 +60,59 @@ function $TemplateRequestProvider() { * * @property {number} totalPendingRequests total amount of pending template requests being downloaded. */ - this.$get = ['$templateCache', '$http', '$q', '$sce', function($templateCache, $http, $q, $sce) { + this.$get = ['$exceptionHandler', '$templateCache', '$http', '$q', '$sce', + function($exceptionHandler, $templateCache, $http, $q, $sce) { - function handleRequestFn(tpl, ignoreRequestError) { - handleRequestFn.totalPendingRequests++; + function handleRequestFn(tpl, ignoreRequestError) { + handleRequestFn.totalPendingRequests++; - // We consider the template cache holds only trusted templates, so - // there's no need to go through whitelisting again for keys that already - // are included in there. This also makes Angular accept any script - // directive, no matter its name. However, we still need to unwrap trusted - // types. - if (!isString(tpl) || isUndefined($templateCache.get(tpl))) { - tpl = $sce.getTrustedResourceUrl(tpl); - } + // We consider the template cache holds only trusted templates, so + // there's no need to go through whitelisting again for keys that already + // are included in there. This also makes Angular accept any script + // directive, no matter its name. However, we still need to unwrap trusted + // types. + if (!isString(tpl) || isUndefined($templateCache.get(tpl))) { + tpl = $sce.getTrustedResourceUrl(tpl); + } - var transformResponse = $http.defaults && $http.defaults.transformResponse; + var transformResponse = $http.defaults && $http.defaults.transformResponse; - if (isArray(transformResponse)) { - transformResponse = transformResponse.filter(function(transformer) { - return transformer !== defaultHttpResponseTransform; - }); - } else if (transformResponse === defaultHttpResponseTransform) { - transformResponse = null; - } + if (isArray(transformResponse)) { + transformResponse = transformResponse.filter(function(transformer) { + return transformer !== defaultHttpResponseTransform; + }); + } else if (transformResponse === defaultHttpResponseTransform) { + transformResponse = null; + } - return $http.get(tpl, extend({ - cache: $templateCache, - transformResponse: transformResponse - }, httpOptions)) - .finally(function() { - handleRequestFn.totalPendingRequests--; - }) - .then(function(response) { - $templateCache.put(tpl, response.data); - return response.data; - }, handleError); + return $http.get(tpl, extend({ + cache: $templateCache, + transformResponse: transformResponse + }, httpOptions)) + .finally(function() { + handleRequestFn.totalPendingRequests--; + }) + .then(function(response) { + $templateCache.put(tpl, response.data); + return response.data; + }, handleError); - function handleError(resp) { - if (!ignoreRequestError) { - throw $templateRequestMinErr('tpload', 'Failed to load template: {0} (HTTP status: {1} {2})', - tpl, resp.status, resp.statusText); + function handleError(resp) { + if (!ignoreRequestError) { + resp = $templateRequestMinErr('tpload', + 'Failed to load template: {0} (HTTP status: {1} {2})', + tpl, resp.status, resp.statusText); + + $exceptionHandler(resp); + } + + return $q.reject(resp); } - return $q.reject(resp); } - } - handleRequestFn.totalPendingRequests = 0; + handleRequestFn.totalPendingRequests = 0; - return handleRequestFn; - }]; + return handleRequestFn; + } + ]; } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 492741a4496a..ef26f747f1e5 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -1854,17 +1854,18 @@ describe('$compile', function() { )); - it('should throw an error and clear element content if the template fails to load', inject( - function($compile, $httpBackend, $rootScope) { - $httpBackend.expect('GET', 'hello.html').respond(404, 'Not Found!'); - element = $compile('
content
')($rootScope); + it('should throw an error and clear element content if the template fails to load', + inject(function($compile, $exceptionHandler, $httpBackend, $rootScope) { + $httpBackend.expect('GET', 'hello.html').respond(404, 'Not Found!'); + element = $compile('
content
')($rootScope); - expect(function() { - $httpBackend.flush(); - }).toThrowMinErr('$compile', 'tpload', 'Failed to load template: hello.html'); - expect(sortedHtml(element)).toBe('
'); - } - )); + $httpBackend.flush(); + + expect(sortedHtml(element)).toBe('
'); + expect($exceptionHandler.errors[0].message).toMatch( + /^\[\$compile:tpload] Failed to load template: hello\.html/); + }) + ); it('should prevent multiple templates per element', function() { @@ -1878,13 +1879,15 @@ describe('$compile', function() { templateUrl: 'template.html' })); }); - inject(function($compile, $httpBackend) { + inject(function($compile, $exceptionHandler, $httpBackend) { $httpBackend.whenGET('template.html').respond('

template.html

'); - expect(function() { - $compile('
'); - $httpBackend.flush(); - }).toThrowMinErr('$compile', 'multidir', 'Multiple directives [async, sync] asking for template on: ' + - '
'); + + $compile('
'); + $httpBackend.flush(); + + expect($exceptionHandler.errors[0].message).toMatch(new RegExp( + '^\\[\\$compile:multidir] Multiple directives \\[async, sync] asking for ' + + 'template on:
')); }); }); @@ -2667,14 +2670,15 @@ describe('$compile', function() { ); it('should not allow more than one isolate/new scope creation per element regardless of `templateUrl`', - inject(function($httpBackend) { + inject(function($exceptionHandler, $httpBackend) { $httpBackend.expect('GET', 'tiscope.html').respond('
Hello, world !
'); - expect(function() { - compile('
'); - $httpBackend.flush(); - }).toThrowMinErr('$compile', 'multidir', 'Multiple directives [scopeB, tiscopeA] ' + - 'asking for new/isolated scope on:
'); + compile('
'); + $httpBackend.flush(); + + expect($exceptionHandler.errors[0].message).toMatch(new RegExp( + '^\\[\\$compile:multidir] Multiple directives \\[scopeB, tiscopeA] ' + + 'asking for new/isolated scope on:
')); }) ); @@ -8875,28 +8879,29 @@ describe('$compile', function() { '
' + '
', transclude: true - })); $compileProvider.directive('noTransBar', valueFn({ templateUrl: 'noTransBar.html', transclude: false - })); }); - inject(function($compile, $rootScope, $templateCache) { + inject(function($compile, $exceptionHandler, $rootScope, $templateCache) { $templateCache.put('noTransBar.html', '
' + // This ng-transclude is invalid. It should throw an error. '
' + '
'); - expect(function() { - element = $compile('
content
')($rootScope); - $rootScope.$apply(); - }).toThrowMinErr('ngTransclude', 'orphan', - 'Illegal use of ngTransclude directive in the template! No parent directive that requires a transclusion found. Element:
'); + element = $compile('
content
')($rootScope); + $rootScope.$digest(); + + expect($exceptionHandler.errors[0][1]).toBe('
'); + expect($exceptionHandler.errors[0][0].message).toMatch(new RegExp( + '^\\[ngTransclude:orphan] Illegal use of ngTransclude directive in the ' + + 'template! No parent directive that requires a transclusion found. Element: ' + + '
')); }); }); @@ -9706,12 +9711,15 @@ describe('$compile', function() { transclude: 'element' })); }); - inject(function($compile, $httpBackend) { + inject(function($compile, $exceptionHandler, $httpBackend) { $httpBackend.expectGET('template.html').respond('

template.html

'); + $compile('
'); - expect(function() { - $httpBackend.flush(); - }).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on:

throw(oops); error2(oops)->reject(oops)'); - expect(mockExceptionLogger.log).toEqual(['oops']); + expect(mockExceptionLogger.log).toEqual([]); }); @@ -2095,12 +2095,13 @@ describe('q', function() { }); - it('should log exceptions thrown in a errback and reject the derived promise', function() { + it('should NOT log exceptions thrown in an errback but reject the derived promise', + function() { var error1 = error(1, 'oops', true); promise.then(null, error1).then(success(2), error(2)).catch(noop); syncReject(deferred, 'nope'); expect(logStr()).toBe('error1(nope)->throw(oops); error2(oops)->reject(oops)'); - expect(mockExceptionLogger.log).toEqual(['oops']); + expect(mockExceptionLogger.log).toEqual([]); }); @@ -2127,13 +2128,13 @@ describe('q', function() { describe('in when', function() { - it('should log exceptions thrown in a success callback and reject the derived promise', + it('should NOT log exceptions thrown in a success callback but reject the derived promise', function() { var success1 = success(1, 'oops', true); q.when('hi', success1, error()).then(success(), error(2)).catch(noop); mockNextTick.flush(); expect(logStr()).toBe('success1(hi)->throw(oops); error2(oops)->reject(oops)'); - expect(mockExceptionLogger.log).toEqual(['oops']); + expect(mockExceptionLogger.log).toEqual([]); }); @@ -2145,12 +2146,12 @@ describe('q', function() { }); - it('should log exceptions thrown in a errback and reject the derived promise', function() { + it('should NOT log exceptions thrown in a errback but reject the derived promise', function() { var error1 = error(1, 'oops', true); q.when(q.reject('sorry'), success(), error1).then(success(), error(2)).catch(noop); mockNextTick.flush(); expect(logStr()).toBe('error1(sorry)->throw(oops); error2(oops)->reject(oops)'); - expect(mockExceptionLogger.log).toEqual(['oops']); + expect(mockExceptionLogger.log).toEqual([]); }); @@ -2207,50 +2208,4 @@ describe('q', function() { expect(exceptionHandlerStr()).toBe(''); }); }); - - - describe('when exceptionHandler rethrows exceptions, ', function() { - var originalLogExceptions, deferred, errorSpy, exceptionExceptionSpy; - - beforeEach(function() { - // Turn off exception logging for these particular tests - originalLogExceptions = mockNextTick.logExceptions; - mockNextTick.logExceptions = false; - - // Set up spies - exceptionExceptionSpy = jasmine.createSpy('rethrowExceptionHandler') - .and.callFake(function rethrowExceptionHandler(e) { - throw e; - }); - errorSpy = jasmine.createSpy('errorSpy'); - - - q = qFactory(mockNextTick.nextTick, exceptionExceptionSpy); - deferred = q.defer(); - }); - - - afterEach(function() { - // Restore the original exception logging mode - mockNextTick.logExceptions = originalLogExceptions; - }); - - - it('should still reject the promise, when exception is thrown in success handler, even if exceptionHandler rethrows', function() { - deferred.promise.then(function() { throw new Error('reject'); }).then(null, errorSpy); - deferred.resolve('resolve'); - mockNextTick.flush(); - expect(exceptionExceptionSpy).toHaveBeenCalled(); - expect(errorSpy).toHaveBeenCalled(); - }); - - - it('should still reject the promise, when exception is thrown in error handler, even if exceptionHandler rethrows', function() { - deferred.promise.then(null, function() { throw new Error('reject again'); }).then(null, errorSpy); - deferred.reject('reject'); - mockNextTick.flush(); - expect(exceptionExceptionSpy).toHaveBeenCalled(); - expect(errorSpy).toHaveBeenCalled(); - }); - }); }); diff --git a/test/ng/templateRequestSpec.js b/test/ng/templateRequestSpec.js index b064b51875f7..4bf426700ca2 100644 --- a/test/ng/templateRequestSpec.js +++ b/test/ng/templateRequestSpec.js @@ -115,19 +115,17 @@ describe('$templateRequest', function() { })); it('should throw an error when the template is not found', - inject(function($rootScope, $templateRequest, $httpBackend) { - - $httpBackend.expectGET('tpl.html').respond(404, '', {}, 'Not found'); - - $templateRequest('tpl.html'); - - $rootScope.$digest(); + inject(function($exceptionHandler, $httpBackend, $rootScope, $templateRequest) { + $httpBackend.expectGET('tpl.html').respond(404, '', {}, 'Not found'); - expect(function() { - $rootScope.$digest(); + $templateRequest('tpl.html').catch(noop); $httpBackend.flush(); - }).toThrowMinErr('$compile', 'tpload', 'Failed to load template: tpl.html (HTTP status: 404 Not found)'); - })); + + expect($exceptionHandler.errors[0].message).toMatch(new RegExp( + '^\\[\\$compile:tpload] Failed to load template: tpl\\.html ' + + '\\(HTTP status: 404 Not found\\)')); + }) + ); it('should not throw when the template is not found and ignoreRequestError is true', inject(function($rootScope, $templateRequest, $httpBackend) { diff --git a/test/ngRoute/routeSpec.js b/test/ngRoute/routeSpec.js index dd325e10e580..9884a199f1d2 100644 --- a/test/ngRoute/routeSpec.js +++ b/test/ngRoute/routeSpec.js @@ -782,11 +782,20 @@ describe('$route', function() { }); inject(function($route, $location, $rootScope) { + var onError = jasmine.createSpy('onError'); + var onSuccess = jasmine.createSpy('onSuccess'); + + $rootScope.$on('$routeChangeError', onError); + $rootScope.$on('$routeChangeSuccess', onSuccess); + $location.path('/foo'); - expect(function() { - $rootScope.$digest(); - }).toThrowMinErr('$sce', 'insecurl', 'Blocked loading resource from url not allowed by ' + - '$sceDelegate policy. URL: http://example.com/foo.html'); + $rootScope.$digest(); + + expect(onSuccess).not.toHaveBeenCalled(); + expect(onError).toHaveBeenCalled(); + expect(onError.calls.mostRecent().args[3].message).toMatch(new RegExp( + '^\\[\\$sce:insecurl] Blocked loading resource from url not allowed by ' + + '\\$sceDelegate policy\\. URL: http:\\/\\/example\\.com\\/foo\\.html')); }); }); @@ -903,8 +912,7 @@ describe('$route', function() { it('should catch local factory errors', function() { var myError = new Error('MyError'); - module(function($routeProvider, $exceptionHandlerProvider) { - $exceptionHandlerProvider.mode('log'); + module(function($routeProvider) { $routeProvider.when('/locals', { resolve: { a: function($q) { @@ -914,10 +922,14 @@ describe('$route', function() { }); }); - inject(function($location, $route, $rootScope, $exceptionHandler) { + inject(function($location, $route, $rootScope) { + spyOn($rootScope, '$broadcast').and.callThrough(); + $location.path('/locals'); $rootScope.$digest(); - expect($exceptionHandler.errors).toEqual([myError]); + + expect($rootScope.$broadcast).toHaveBeenCalledWith( + '$routeChangeError', jasmine.any(Object), undefined, myError); }); }); }); @@ -1182,8 +1194,7 @@ describe('$route', function() { it('should broadcast `$routeChangeError` when redirectTo throws', function() { var error = new Error('Test'); - module(function($exceptionHandlerProvider, $routeProvider) { - $exceptionHandlerProvider.mode('log'); + module(function($routeProvider) { $routeProvider.when('/foo', {redirectTo: function() { throw error; }}); }); @@ -1196,7 +1207,6 @@ describe('$route', function() { var lastCallArgs = $rootScope.$broadcast.calls.mostRecent().args; expect(lastCallArgs[0]).toBe('$routeChangeError'); expect(lastCallArgs[3]).toBe(error); - expect($exceptionHandler.errors[0]).toBe(error); }); });