From 9e39bb655cd280955487df61bee6aeeeb64e9080 Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sun, 11 Apr 2021 14:19:23 +0800 Subject: [PATCH] fix(zone.js): should continue to executue listeners when throw error (#41562) Close #41522 `zone.js` patches event listeners and run all event listeners together, if one event handler throws error, the listeners afterward may not be invoked. Reproduction: ``` export class AppComponent implements AfterViewInit { @ViewChild('btn') btn: ElementRef; title = 'event-error'; constructor(private ngZone: NgZone) {} ngAfterViewInit() { this.ngZone.runOutsideAngular(() => { this.btn.nativeElement.addEventListener('click', () => { throw new Error('test1'); }); this.btn.nativeElement.addEventListener('click', () => { console.log('add eventlistener click'); }); }); } } ``` Until now no Angular users report this issue becuase in the `ngZone`, all error will be caught and will not rethrow, so the event listeners afterward will still continue to execute, but if the event handlers are outside of `ngZone`, the error will break the execution. This commit catch all errors, and after all event listeners finished invocation, rethrow the errors in seperate `microTasks`, the reason I am using `microTask` here is to handle multiple errors case. PR Close #41562 --- packages/zone.js/lib/browser/browser.ts | 2 +- .../lib/browser/event-target-legacy.ts | 2 +- packages/zone.js/lib/browser/event-target.ts | 2 +- packages/zone.js/lib/browser/shadydom.ts | 2 +- .../browser/webapis-rtc-peer-connection.ts | 2 +- packages/zone.js/lib/browser/websocket.ts | 2 +- packages/zone.js/lib/common/events.ts | 64 ++++++------- packages/zone.js/lib/extra/socket-io.ts | 2 +- packages/zone.js/lib/node/events.ts | 4 +- packages/zone.js/lib/zone.ts | 42 +++++---- packages/zone.js/test/browser/browser.spec.ts | 94 ++++++++++++++++++- 11 files changed, 154 insertions(+), 64 deletions(-) diff --git a/packages/zone.js/lib/browser/browser.ts b/packages/zone.js/lib/browser/browser.ts index a6190c42ef4..88521a71f66 100644 --- a/packages/zone.js/lib/browser/browser.ts +++ b/packages/zone.js/lib/browser/browser.ts @@ -66,7 +66,7 @@ Zone.__load_patch('EventTarget', (global: any, Zone: ZoneType, api: _ZonePrivate // patch XMLHttpRequestEventTarget's addEventListener/removeEventListener const XMLHttpRequestEventTarget = (global as any)['XMLHttpRequestEventTarget']; if (XMLHttpRequestEventTarget && XMLHttpRequestEventTarget.prototype) { - api.patchEventTarget(global, [XMLHttpRequestEventTarget.prototype]); + api.patchEventTarget(global, api, [XMLHttpRequestEventTarget.prototype]); } }); diff --git a/packages/zone.js/lib/browser/event-target-legacy.ts b/packages/zone.js/lib/browser/event-target-legacy.ts index 9cf7a939f92..5bebb2c395e 100644 --- a/packages/zone.js/lib/browser/event-target-legacy.ts +++ b/packages/zone.js/lib/browser/event-target-legacy.ts @@ -112,7 +112,7 @@ export function eventTargetLegacyPatch(_global: any, api: _ZonePrivate) { } // vh is validateHandler to check event handler // is valid or not(for security check) - api.patchEventTarget(_global, apiTypes, { + api.patchEventTarget(_global, api, apiTypes, { vh: checkIEAndCrossContext, transferEventName: (eventName: string) => { const pointerEventName = pointerEventsMap[eventName]; diff --git a/packages/zone.js/lib/browser/event-target.ts b/packages/zone.js/lib/browser/event-target.ts index 8ef5418f2a8..215ac3d13ef 100644 --- a/packages/zone.js/lib/browser/event-target.ts +++ b/packages/zone.js/lib/browser/event-target.ts @@ -29,7 +29,7 @@ export function eventTargetPatch(_global: any, api: _ZonePrivate) { if (!EVENT_TARGET || !EVENT_TARGET.prototype) { return; } - api.patchEventTarget(_global, [EVENT_TARGET && EVENT_TARGET.prototype]); + api.patchEventTarget(_global, api, [EVENT_TARGET && EVENT_TARGET.prototype]); return true; } diff --git a/packages/zone.js/lib/browser/shadydom.ts b/packages/zone.js/lib/browser/shadydom.ts index ca7d5ba86ce..ef3e0c9192d 100644 --- a/packages/zone.js/lib/browser/shadydom.ts +++ b/packages/zone.js/lib/browser/shadydom.ts @@ -20,7 +20,7 @@ Zone.__load_patch('shadydom', (global: any, Zone: ZoneType, api: _ZonePrivate) = if (proto && proto.hasOwnProperty('addEventListener')) { proto[Zone.__symbol__('addEventListener')] = null; proto[Zone.__symbol__('removeEventListener')] = null; - api.patchEventTarget(global, [proto]); + api.patchEventTarget(global, api, [proto]); } }); }); diff --git a/packages/zone.js/lib/browser/webapis-rtc-peer-connection.ts b/packages/zone.js/lib/browser/webapis-rtc-peer-connection.ts index 1b31e0c85c8..9ee71dea5c8 100644 --- a/packages/zone.js/lib/browser/webapis-rtc-peer-connection.ts +++ b/packages/zone.js/lib/browser/webapis-rtc-peer-connection.ts @@ -22,5 +22,5 @@ Zone.__load_patch('RTCPeerConnection', (global: any, Zone: ZoneType, api: _ZoneP RTCPeerConnection.prototype[addSymbol] = null; RTCPeerConnection.prototype[removeSymbol] = null; - api.patchEventTarget(global, [RTCPeerConnection.prototype], {useG: false}); + api.patchEventTarget(global, api, [RTCPeerConnection.prototype], {useG: false}); }); diff --git a/packages/zone.js/lib/browser/websocket.ts b/packages/zone.js/lib/browser/websocket.ts index d11432c8213..fecb8149664 100644 --- a/packages/zone.js/lib/browser/websocket.ts +++ b/packages/zone.js/lib/browser/websocket.ts @@ -13,7 +13,7 @@ export function apply(api: _ZonePrivate, _global: any) { // On Safari window.EventTarget doesn't exist so need to patch WS add/removeEventListener // On older Chrome, no need since EventTarget was already patched if (!(_global).EventTarget) { - api.patchEventTarget(_global, [WS.prototype]); + api.patchEventTarget(_global, api, [WS.prototype]); } (_global).WebSocket = function(x: any, y: any) { const socket = arguments.length > 1 ? new WS(x, y) : new WS(x); diff --git a/packages/zone.js/lib/common/events.ts b/packages/zone.js/lib/common/events.ts index 7d70918eca8..d96100fb2ae 100644 --- a/packages/zone.js/lib/common/events.ts +++ b/packages/zone.js/lib/common/events.ts @@ -86,7 +86,7 @@ export interface PatchEventTargetOptions { } export function patchEventTarget( - _global: any, apis: any[], patchOptions?: PatchEventTargetOptions) { + _global: any, api: _ZonePrivate, apis: any[], patchOptions?: PatchEventTargetOptions) { const ADD_EVENT_LISTENER = (patchOptions && patchOptions.add) || ADD_EVENT_LISTENER_STR; const REMOVE_EVENT_LISTENER = (patchOptions && patchOptions.rm) || REMOVE_EVENT_LISTENER_STR; @@ -114,7 +114,15 @@ export function patchEventTarget( task.originalDelegate = delegate; } // invoke static task.invoke - task.invoke(task, target, [event]); + // need to try/catch error here, otherwise, the error in one event listener + // will break the executions of the other event listeners. Also error will + // not remove the event listener when `once` options is true. + let error; + try { + task.invoke(task, target, [event]); + } catch (err) { + error = err; + } const options = task.options; if (options && typeof options === 'object' && options.once) { // if options.once is true, after invoke once remove listener here @@ -123,10 +131,10 @@ export function patchEventTarget( const delegate = task.originalDelegate ? task.originalDelegate : task.callback; target[REMOVE_EVENT_LISTENER].call(target, event.type, delegate, options); } + return error; }; - // global shared zoneAwareCallback to handle all event callback with capture = false - const globalZoneAwareCallback = function(this: unknown, event: Event) { + function globalCallback(context: unknown, event: Event, isCapture: boolean) { // https://github.com/angular/zone.js/issues/911, in IE, sometimes // event will be undefined, so we need to use window.event event = event || _global.event; @@ -135,8 +143,8 @@ export function patchEventTarget( } // event.target is needed for Samsung TV and SourceBuffer // || global is needed https://github.com/angular/zone.js/issues/190 - const target: any = this || event.target || _global; - const tasks = target[zoneSymbolEventNames[event.type][FALSE_STR]]; + const target: any = context || event.target || _global; + const tasks = target[zoneSymbolEventNames[event.type][isCapture ? TRUE_STR : FALSE_STR]]; if (tasks) { // invoke all tasks which attached to current target with given event.type and capture = false // for performance concern, if task.length === 1, just invoke @@ -147,46 +155,32 @@ export function patchEventTarget( // 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; } - invokeTask(copyTasks[i], target, event); + const err = invokeTask(copyTasks[i], target, event); + err && errors.push(err); + } + for (let i = 0; i < errors.length; i++) { + const err = errors[i]; + api.nativeScheduleMicroTask(() => { + throw err; + }); } } } + } + + // global shared zoneAwareCallback to handle all event callback with capture = false + const globalZoneAwareCallback = function(this: unknown, event: Event) { + return globalCallback(this, event, false); }; // global shared zoneAwareCallback to handle all event callback with capture = true const globalZoneAwareCaptureCallback = function(this: unknown, event: Event) { - // https://github.com/angular/zone.js/issues/911, in IE, sometimes - // event will be undefined, so we need to use window.event - event = event || _global.event; - if (!event) { - return; - } - // event.target is needed for Samsung TV and SourceBuffer - // || global is needed https://github.com/angular/zone.js/issues/190 - const target: any = this || event.target || _global; - const tasks = target[zoneSymbolEventNames[event.type][TRUE_STR]]; - if (tasks) { - // 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); - } 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(); - for (let i = 0; i < copyTasks.length; i++) { - if (event && (event as any)[IMMEDIATE_PROPAGATION_SYMBOL] === true) { - break; - } - invokeTask(copyTasks[i], target, event); - } - } - } + return globalCallback(this, event, true); }; function patchEventTargetMethods(obj: any, patchOptions?: PatchEventTargetOptions) { diff --git a/packages/zone.js/lib/extra/socket-io.ts b/packages/zone.js/lib/extra/socket-io.ts index b3cbdd31041..696065aeab5 100644 --- a/packages/zone.js/lib/extra/socket-io.ts +++ b/packages/zone.js/lib/extra/socket-io.ts @@ -8,7 +8,7 @@ Zone.__load_patch('socketio', (global: any, Zone: ZoneType, api: _ZonePrivate) => { (Zone as any)[Zone.__symbol__('socketio')] = function patchSocketIO(io: any) { // patch io.Socket.prototype event listener related method - api.patchEventTarget(global, [io.Socket.prototype], { + api.patchEventTarget(global, api, [io.Socket.prototype], { useG: false, chkDup: false, rt: true, diff --git a/packages/zone.js/lib/node/events.ts b/packages/zone.js/lib/node/events.ts index f09f8a42167..d2e6832bba5 100644 --- a/packages/zone.js/lib/node/events.ts +++ b/packages/zone.js/lib/node/events.ts @@ -8,7 +8,7 @@ import {patchEventTarget} from '../common/events'; -Zone.__load_patch('EventEmitter', (global: any) => { +Zone.__load_patch('EventEmitter', (global: any, Zone: ZoneType, api: _ZonePrivate) => { // For EventEmitter const EE_ADD_LISTENER = 'addListener'; const EE_PREPEND_LISTENER = 'prependListener'; @@ -34,7 +34,7 @@ Zone.__load_patch('EventEmitter', (global: any) => { }; function patchEventEmitterMethods(obj: any) { - const result = patchEventTarget(global, [obj], { + const result = patchEventTarget(global, api, [obj], { useG: false, add: EE_ADD_LISTENER, rm: EE_REMOVE_LISTENER, diff --git a/packages/zone.js/lib/zone.ts b/packages/zone.js/lib/zone.ts index 92bd2f2bbf7..59f23745b80 100644 --- a/packages/zone.js/lib/zone.ts +++ b/packages/zone.js/lib/zone.ts @@ -337,7 +337,7 @@ interface _ZonePrivate { onUnhandledError: (error: Error) => void; microtaskDrainDone: () => void; showUncaughtError: () => boolean; - patchEventTarget: (global: any, apis: any[], options?: any) => boolean[]; + patchEventTarget: (global: any, api: _ZonePrivate, apis: any[], options?: any) => boolean[]; patchOnProperties: (obj: any, properties: string[]|null, prototype?: any) => void; patchThen: (ctro: Function) => void; patchMethod: @@ -359,6 +359,7 @@ interface _ZonePrivate { filterProperties: (target: any, onProperties: string[], ignoreProperties: any[]) => string[]; attachOriginToPatched: (target: any, origin: any) => void; _redefineProperty: (target: any, callback: string, desc: any) => void; + nativeScheduleMicroTask: (func: Function) => void; patchCallbacks: (api: _ZonePrivate, target: any, targetName: string, method: string, callbacks: string[]) => void; @@ -1343,27 +1344,31 @@ const Zone: ZoneType = (function(global: any) { let _isDrainingMicrotaskQueue: boolean = false; let nativeMicroTaskQueuePromise: any; + function nativeScheduleMicroTask(func: Function) { + if (!nativeMicroTaskQueuePromise) { + if (global[symbolPromise]) { + nativeMicroTaskQueuePromise = global[symbolPromise].resolve(0); + } + } + if (nativeMicroTaskQueuePromise) { + let nativeThen = nativeMicroTaskQueuePromise[symbolThen]; + if (!nativeThen) { + // native Promise is not patchable, we need to use `then` directly + // issue 1078 + nativeThen = nativeMicroTaskQueuePromise['then']; + } + nativeThen.call(nativeMicroTaskQueuePromise, func); + } else { + global[symbolSetTimeout](func, 0); + } + } + function scheduleMicroTask(task?: MicroTask) { // if we are not running in any task, and there has not been anything scheduled // we must bootstrap the initial task creation by manually scheduling the drain if (_numberOfNestedTaskFrames === 0 && _microTaskQueue.length === 0) { // We are not running in Task, so we need to kickstart the microtask queue. - if (!nativeMicroTaskQueuePromise) { - if (global[symbolPromise]) { - nativeMicroTaskQueuePromise = global[symbolPromise].resolve(0); - } - } - if (nativeMicroTaskQueuePromise) { - let nativeThen = nativeMicroTaskQueuePromise[symbolThen]; - if (!nativeThen) { - // native Promise is not patchable, we need to use `then` directly - // issue 1078 - nativeThen = nativeMicroTaskQueuePromise['then']; - } - nativeThen.call(nativeMicroTaskQueuePromise, drainMicroTaskQueue); - } else { - global[symbolSetTimeout](drainMicroTaskQueue, 0); - } + nativeScheduleMicroTask(drainMicroTaskQueue); } task && _microTaskQueue.push(task); } @@ -1428,7 +1433,8 @@ const Zone: ZoneType = (function(global: any) { filterProperties: () => [], attachOriginToPatched: () => noop, _redefineProperty: () => noop, - patchCallbacks: () => noop + patchCallbacks: () => noop, + nativeScheduleMicroTask: nativeScheduleMicroTask }; let _currentZoneFrame: _ZoneFrame = {parent: null, zone: new Zone(null, null)}; let _currentTask: Task|null = null; diff --git a/packages/zone.js/test/browser/browser.spec.ts b/packages/zone.js/test/browser/browser.spec.ts index 127bc9a2fbb..7eea07815f3 100644 --- a/packages/zone.js/test/browser/browser.spec.ts +++ b/packages/zone.js/test/browser/browser.spec.ts @@ -348,7 +348,7 @@ describe('Zone', function() { (HTMLSpanElement.prototype as any)[zoneSymbol('addEventListener')] = null; - patchEventTarget(window, [HTMLSpanElement.prototype]); + patchEventTarget(window, null as any, [HTMLSpanElement.prototype]); const span = document.createElement('span'); document.body.appendChild(span); @@ -1037,7 +1037,7 @@ describe('Zone', function() { })); it('should change options to boolean if not support passive', () => { - patchEventTarget(window, [TestEventListener.prototype]); + patchEventTarget(window, null as any, [TestEventListener.prototype]); const testEventListener = new TestEventListener(); const listener = function() {}; @@ -2490,6 +2490,96 @@ describe('Zone', function() { expect(hookSpy).not.toHaveBeenCalled(); expect(logs).toEqual([]); })); + + it('should be able to continue to invoke remaining listeners even some listener throw error', + function(done: DoneFn) { + 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'); + } + }; + + 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) => { + logs.push(e.reason.message); + }; + window.addEventListener('unhandledrejection', unhandledRejection); + + button.dispatchEvent(mouseEvent); + expect(logs).toEqual(['listener1', 'listener2']); + + setTimeout(() => { + expect(logs).toEqual(['listener1', 'listener2', 'test1', 'test2']); + window.removeEventListener('unhandledrejection', unhandledRejection); + done() + }); + }); + + it('should be able to continue to invoke remaining listeners even some listener throw error in the different zones', + function(done: DoneFn) { + let logs: string[] = []; + const zone1 = Zone.current.fork({ + name: 'zone1', + onHandleError: (delegate, curr, target, error) => { + logs.push(error.message); + return false; + } + }); + 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'); + } + }; + + button.addEventListener('click', listener1); + zone1.run(() => { + 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) => { + logs.push(e.reason.message); + }; + window.addEventListener('unhandledrejection', unhandledRejection); + + button.dispatchEvent(mouseEvent); + expect(logs).toEqual(['listener1', 'test1', 'listener2']); + + setTimeout(() => { + expect(logs).toEqual(['listener1', 'test1', 'listener2', 'test2']); + done() + }); + }); }); // TODO: Re-enable via https://github.com/angular/angular/pull/41526