From 299f92c3b62a43c94cff4a204f9e41c46a159efc Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Thu, 29 Apr 2021 09:05:25 +0800 Subject: [PATCH] fix(zone.js): only one listener should also re-throw an error correctly (#41868) Close #41867 In the previous commit https://github.com/angular/angular/pull/41562#issuecomment-822696973, the error thrown in the event listener will be caught and re-thrown, but there is a bug in the commit, if there is only one listener for the specified event name, the error will not be re-thrown, so this commit fixes the issue and make sure the error is re-thrown. PR Close #41868 --- .../size-tracking/integration-payloads.json | 2 +- packages/zone.js/lib/common/events.ts | 13 +- .../test/browser/XMLHttpRequest.spec.ts | 11 +- packages/zone.js/test/browser/browser.spec.ts | 153 +++++++++++++++++- 4 files changed, 165 insertions(+), 14 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index 6b19296f5f..7bd7d5bd9f 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -13,7 +13,7 @@ "uncompressed": { "runtime-es2015": 1205, "main-es2015": 17597, - "polyfills-es2015": 36709 + "polyfills-es2015": 37127 } } }, diff --git a/packages/zone.js/lib/common/events.ts b/packages/zone.js/lib/common/events.ts index 0cd86f9b92..186773e39c 100644 --- a/packages/zone.js/lib/common/events.ts +++ b/packages/zone.js/lib/common/events.ts @@ -104,7 +104,7 @@ export function patchEventTarget( const PREPEND_EVENT_LISTENER = 'prependListener'; const PREPEND_EVENT_LISTENER_SOURCE = '.' + PREPEND_EVENT_LISTENER + ':'; - const invokeTask = function(task: any, target: any, event: Event) { + const invokeTask = function(task: any, target: any, event: Event): Error|undefined { // for better performance, check isRemoved which is set // by removeEventListener if (task.isRemoved) { @@ -149,16 +149,17 @@ export function patchEventTarget( const target: any = context || event.target || _global; const tasks = target[zoneSymbolEventNames[event.type][isCapture ? TRUE_STR : FALSE_STR]]; if (tasks) { + const errors = []; // invoke all tasks which attached to current target with given event.type and capture = false // for performance concern, if task.length === 1, just invoke if (tasks.length === 1) { - invokeTask(tasks[0], target, event); + const err = invokeTask(tasks[0], target, event); + err && errors.push(err); } else { // https://github.com/angular/zone.js/issues/836 // copy the tasks array before invoke, to avoid // the callback will remove itself or other listener const copyTasks = tasks.slice(); - const errors = []; for (let i = 0; i < copyTasks.length; i++) { if (event && (event as any)[IMMEDIATE_PROPAGATION_SYMBOL] === true) { break; @@ -166,6 +167,12 @@ export function patchEventTarget( const err = invokeTask(copyTasks[i], target, event); err && errors.push(err); } + } + // Since there is only one error, we don't need to schedule microTask + // to throw the error. + if (errors.length === 1) { + throw errors[0]; + } else { for (let i = 0; i < errors.length; i++) { const err = errors[i]; api.nativeScheduleMicroTask(() => { diff --git a/packages/zone.js/test/browser/XMLHttpRequest.spec.ts b/packages/zone.js/test/browser/XMLHttpRequest.spec.ts index ee370c31dc..2c21c49f41 100644 --- a/packages/zone.js/test/browser/XMLHttpRequest.spec.ts +++ b/packages/zone.js/test/browser/XMLHttpRequest.spec.ts @@ -132,9 +132,11 @@ describe('XMLHttpRequest', function() { it('should invoke xhr task even onload listener throw error', function(done) { const oriWindowError = window.onerror; - window.onerror = function() {}; + const logs: string[] = []; + window.onerror = function(err: any) { + logs.push(err); + }; try { - const logs: string[] = []; const xhrZone = Zone.current.fork({ name: 'xhr', onInvokeTask: (delegate, curr, target, task, applyThis, applyArgs) => { @@ -156,7 +158,7 @@ describe('XMLHttpRequest', function() { throw new Error('test'); }; const unhandledRejection = (e: PromiseRejectionEvent) => { - logs.push(e.reason.message); + fail('should not be here'); }; window.addEventListener('unhandledrejection', unhandledRejection); req.addEventListener('load', () => { @@ -165,8 +167,7 @@ describe('XMLHttpRequest', function() { expect(logs).toEqual([ 'hasTask true', 'invokeTask XMLHttpRequest.addEventListener:load', 'onload', 'invokeTask XMLHttpRequest.addEventListener:load', 'onload1', - 'invokeTask XMLHttpRequest.send', 'hasTask false', - 'invokeTask Window.addEventListener:unhandledrejection', 'test' + 'invokeTask XMLHttpRequest.send', 'hasTask false', 'Uncaught Error: test' ]); window.removeEventListener('unhandledrejection', unhandledRejection); window.onerror = oriWindowError; diff --git a/packages/zone.js/test/browser/browser.spec.ts b/packages/zone.js/test/browser/browser.spec.ts index 3e7c6f9c24..b73ddbf351 100644 --- a/packages/zone.js/test/browser/browser.spec.ts +++ b/packages/zone.js/test/browser/browser.spec.ts @@ -2491,6 +2491,85 @@ describe('Zone', function() { expect(logs).toEqual([]); })); + it('should re-throw the error when the only listener throw error', function(done: DoneFn) { + // override global.onerror to prevent jasmine report error + let oriWindowOnError = window.onerror; + let logs: string[] = []; + window.onerror = function(err: any) { + logs.push(err); + }; + try { + const listener1 = function() { + throw new Error('test1'); + }; + button.addEventListener('click', listener1); + + const mouseEvent = document.createEvent('MouseEvent'); + mouseEvent.initEvent('click', true, true); + + const unhandledRejection = (e: PromiseRejectionEvent) => { + fail('should not be here'); + }; + window.addEventListener('unhandledrejection', unhandledRejection); + + button.dispatchEvent(mouseEvent); + expect(logs).toEqual(['Uncaught Error: test1']); + + setTimeout(() => { + expect(logs).toEqual(['Uncaught Error: test1']); + window.removeEventListener('unhandledrejection', unhandledRejection); + window.onerror = oriWindowOnError; + done() + }); + } catch (e: any) { + window.onerror = oriWindowOnError; + } + }); + + it('should not re-throw the error when zone onHandleError handled the error and the only listener throw error', + function(done: DoneFn) { + // override global.onerror to prevent jasmine report error + let oriWindowOnError = window.onerror; + window.onerror = function() {}; + try { + let logs: string[] = []; + const listener1 = function() { + throw new Error('test1'); + }; + const zone = Zone.current.fork({ + name: 'error', + onHandleError: (delegate, curr, target, error) => { + logs.push('zone handled ' + target.name + ' ' + error.message); + return false; + } + }); + + zone.runGuarded(() => { + button.addEventListener('click', listener1); + }); + + const mouseEvent = document.createEvent('MouseEvent'); + mouseEvent.initEvent('click', true, true); + + const unhandledRejection = (e: PromiseRejectionEvent) => { + logs.push(e.reason.message); + }; + window.addEventListener('unhandledrejection', unhandledRejection); + + button.dispatchEvent(mouseEvent); + expect(logs).toEqual(['zone handled error test1']); + + setTimeout(() => { + expect(logs).toEqual(['zone handled error test1']); + window.removeEventListener('unhandledrejection', unhandledRejection); + window.onerror = oriWindowOnError; + done(); + }); + } catch (e: any) { + window.onerror = oriWindowOnError; + } + }); + it('should be able to continue to invoke remaining listeners even some listener throw error', function(done: DoneFn) { // override global.onerror to prevent jasmine report error @@ -2540,13 +2619,77 @@ describe('Zone', function() { } }); - it('should be able to continue to invoke remaining listeners even some listener throw error in the different zones', + it('should be able to continue to invoke remaining listeners even some listener throw error with onHandleError zone', function(done: DoneFn) { // override global.onerror to prevent jasmine report error let oriWindowOnError = window.onerror; window.onerror = function() {}; try { + const zone = Zone.current.fork({ + name: 'error', + onHandleError: (delegate, curr, target, error) => { + logs.push('zone handled ' + target.name + ' ' + error.message); + return false; + } + }); let logs: string[] = []; + const listener1 = function() { + logs.push('listener1'); + }; + const listener2 = function() { + throw new Error('test1'); + }; + const listener3 = function() { + throw new Error('test2'); + }; + const listener4 = { + handleEvent: function() { + logs.push('listener2'); + } + }; + + zone.runGuarded(() => { + button.addEventListener('click', listener1); + button.addEventListener('click', listener2); + button.addEventListener('click', listener3); + button.addEventListener('click', listener4); + }); + + const mouseEvent = document.createEvent('MouseEvent'); + mouseEvent.initEvent('click', true, true); + + const unhandledRejection = (e: PromiseRejectionEvent) => { + fail('should not be here'); + }; + window.addEventListener('unhandledrejection', unhandledRejection); + + button.dispatchEvent(mouseEvent); + expect(logs).toEqual([ + 'listener1', 'zone handled error test1', 'zone handled error test2', 'listener2' + ]); + + setTimeout(() => { + expect(logs).toEqual([ + 'listener1', 'zone handled error test1', 'zone handled error test2', 'listener2' + ]); + window.removeEventListener('unhandledrejection', unhandledRejection); + window.onerror = oriWindowOnError; + done(); + }); + } catch (e: any) { + window.onerror = oriWindowOnError; + } + }); + + it('should be able to continue to invoke remaining listeners even some listener throw error in the different zones', + function(done: DoneFn) { + // override global.onerror to prevent jasmine report error + let oriWindowOnError = window.onerror; + let logs: string[] = []; + window.onerror = function(err: any) { + logs.push(err); + }; + try { const zone1 = Zone.current.fork({ name: 'zone1', onHandleError: (delegate, curr, target, error) => { @@ -2580,18 +2723,18 @@ describe('Zone', function() { mouseEvent.initEvent('click', true, true); const unhandledRejection = (e: PromiseRejectionEvent) => { - logs.push(e.reason.message); + fail('should not be here'); }; window.addEventListener('unhandledrejection', unhandledRejection); button.dispatchEvent(mouseEvent); - expect(logs).toEqual(['listener1', 'test1', 'listener2']); + expect(logs).toEqual(['listener1', 'test1', 'listener2', 'Uncaught Error: test2']); setTimeout(() => { - expect(logs).toEqual(['listener1', 'test1', 'listener2', 'test2']); + expect(logs).toEqual(['listener1', 'test1', 'listener2', 'Uncaught Error: test2']); window.removeEventListener('unhandledrejection', unhandledRejection); window.onerror = oriWindowOnError; - done() + done(); }); } catch (e: any) { window.onerror = oriWindowOnError;