Skip to content

Commit 299f92c

Browse files
JiaLiPassionalxhub
JiaLiPassion
authored and
alxhub
committed
fix(zone.js): only one listener should also re-throw an error correctly (#41868)
Close #41867 In the previous commit #41562 (comment), 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
1 parent 665b986 commit 299f92c

File tree

4 files changed

+165
-14
lines changed

4 files changed

+165
-14
lines changed

‎goldens/size-tracking/integration-payloads.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
"uncompressed": {
1414
"runtime-es2015": 1205,
1515
"main-es2015": 17597,
16-
"polyfills-es2015": 36709
16+
"polyfills-es2015": 37127
1717
}
1818
}
1919
},

‎packages/zone.js/lib/common/events.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ export function patchEventTarget(
104104
const PREPEND_EVENT_LISTENER = 'prependListener';
105105
const PREPEND_EVENT_LISTENER_SOURCE = '.' + PREPEND_EVENT_LISTENER + ':';
106106

107-
const invokeTask = function(task: any, target: any, event: Event) {
107+
const invokeTask = function(task: any, target: any, event: Event): Error|undefined {
108108
// for better performance, check isRemoved which is set
109109
// by removeEventListener
110110
if (task.isRemoved) {
@@ -149,23 +149,30 @@ export function patchEventTarget(
149149
const target: any = context || event.target || _global;
150150
const tasks = target[zoneSymbolEventNames[event.type][isCapture ? TRUE_STR : FALSE_STR]];
151151
if (tasks) {
152+
const errors = [];
152153
// invoke all tasks which attached to current target with given event.type and capture = false
153154
// for performance concern, if task.length === 1, just invoke
154155
if (tasks.length === 1) {
155-
invokeTask(tasks[0], target, event);
156+
const err = invokeTask(tasks[0], target, event);
157+
err && errors.push(err);
156158
} else {
157159
// https://github.com/angular/zone.js/issues/836
158160
// copy the tasks array before invoke, to avoid
159161
// the callback will remove itself or other listener
160162
const copyTasks = tasks.slice();
161-
const errors = [];
162163
for (let i = 0; i < copyTasks.length; i++) {
163164
if (event && (event as any)[IMMEDIATE_PROPAGATION_SYMBOL] === true) {
164165
break;
165166
}
166167
const err = invokeTask(copyTasks[i], target, event);
167168
err && errors.push(err);
168169
}
170+
}
171+
// Since there is only one error, we don't need to schedule microTask
172+
// to throw the error.
173+
if (errors.length === 1) {
174+
throw errors[0];
175+
} else {
169176
for (let i = 0; i < errors.length; i++) {
170177
const err = errors[i];
171178
api.nativeScheduleMicroTask(() => {

‎packages/zone.js/test/browser/XMLHttpRequest.spec.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -132,9 +132,11 @@ describe('XMLHttpRequest', function() {
132132

133133
it('should invoke xhr task even onload listener throw error', function(done) {
134134
const oriWindowError = window.onerror;
135-
window.onerror = function() {};
135+
const logs: string[] = [];
136+
window.onerror = function(err: any) {
137+
logs.push(err);
138+
};
136139
try {
137-
const logs: string[] = [];
138140
const xhrZone = Zone.current.fork({
139141
name: 'xhr',
140142
onInvokeTask: (delegate, curr, target, task, applyThis, applyArgs) => {
@@ -156,7 +158,7 @@ describe('XMLHttpRequest', function() {
156158
throw new Error('test');
157159
};
158160
const unhandledRejection = (e: PromiseRejectionEvent) => {
159-
logs.push(e.reason.message);
161+
fail('should not be here');
160162
};
161163
window.addEventListener('unhandledrejection', unhandledRejection);
162164
req.addEventListener('load', () => {
@@ -165,8 +167,7 @@ describe('XMLHttpRequest', function() {
165167
expect(logs).toEqual([
166168
'hasTask true', 'invokeTask XMLHttpRequest.addEventListener:load', 'onload',
167169
'invokeTask XMLHttpRequest.addEventListener:load', 'onload1',
168-
'invokeTask XMLHttpRequest.send', 'hasTask false',
169-
'invokeTask Window.addEventListener:unhandledrejection', 'test'
170+
'invokeTask XMLHttpRequest.send', 'hasTask false', 'Uncaught Error: test'
170171
]);
171172
window.removeEventListener('unhandledrejection', unhandledRejection);
172173
window.onerror = oriWindowError;

‎packages/zone.js/test/browser/browser.spec.ts‎

Lines changed: 148 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2491,6 +2491,85 @@ describe('Zone', function() {
24912491
expect(logs).toEqual([]);
24922492
}));
24932493

2494+
it('should re-throw the error when the only listener throw error', function(done: DoneFn) {
2495+
// override global.onerror to prevent jasmine report error
2496+
let oriWindowOnError = window.onerror;
2497+
let logs: string[] = [];
2498+
window.onerror = function(err: any) {
2499+
logs.push(err);
2500+
};
2501+
try {
2502+
const listener1 = function() {
2503+
throw new Error('test1');
2504+
};
2505+
button.addEventListener('click', listener1);
2506+
2507+
const mouseEvent = document.createEvent('MouseEvent');
2508+
mouseEvent.initEvent('click', true, true);
2509+
2510+
const unhandledRejection = (e: PromiseRejectionEvent) => {
2511+
fail('should not be here');
2512+
};
2513+
window.addEventListener('unhandledrejection', unhandledRejection);
2514+
2515+
button.dispatchEvent(mouseEvent);
2516+
expect(logs).toEqual(['Uncaught Error: test1']);
2517+
2518+
setTimeout(() => {
2519+
expect(logs).toEqual(['Uncaught Error: test1']);
2520+
window.removeEventListener('unhandledrejection', unhandledRejection);
2521+
window.onerror = oriWindowOnError;
2522+
done()
2523+
});
2524+
} catch (e: any) {
2525+
window.onerror = oriWindowOnError;
2526+
}
2527+
});
2528+
2529+
it('should not re-throw the error when zone onHandleError handled the error and the only listener throw error',
2530+
function(done: DoneFn) {
2531+
// override global.onerror to prevent jasmine report error
2532+
let oriWindowOnError = window.onerror;
2533+
window.onerror = function() {};
2534+
try {
2535+
let logs: string[] = [];
2536+
const listener1 = function() {
2537+
throw new Error('test1');
2538+
};
2539+
const zone = Zone.current.fork({
2540+
name: 'error',
2541+
onHandleError: (delegate, curr, target, error) => {
2542+
logs.push('zone handled ' + target.name + ' ' + error.message);
2543+
return false;
2544+
}
2545+
});
2546+
2547+
zone.runGuarded(() => {
2548+
button.addEventListener('click', listener1);
2549+
});
2550+
2551+
const mouseEvent = document.createEvent('MouseEvent');
2552+
mouseEvent.initEvent('click', true, true);
2553+
2554+
const unhandledRejection = (e: PromiseRejectionEvent) => {
2555+
logs.push(e.reason.message);
2556+
};
2557+
window.addEventListener('unhandledrejection', unhandledRejection);
2558+
2559+
button.dispatchEvent(mouseEvent);
2560+
expect(logs).toEqual(['zone handled error test1']);
2561+
2562+
setTimeout(() => {
2563+
expect(logs).toEqual(['zone handled error test1']);
2564+
window.removeEventListener('unhandledrejection', unhandledRejection);
2565+
window.onerror = oriWindowOnError;
2566+
done();
2567+
});
2568+
} catch (e: any) {
2569+
window.onerror = oriWindowOnError;
2570+
}
2571+
});
2572+
24942573
it('should be able to continue to invoke remaining listeners even some listener throw error',
24952574
function(done: DoneFn) {
24962575
// override global.onerror to prevent jasmine report error
@@ -2540,13 +2619,77 @@ describe('Zone', function() {
25402619
}
25412620
});
25422621

2543-
it('should be able to continue to invoke remaining listeners even some listener throw error in the different zones',
2622+
it('should be able to continue to invoke remaining listeners even some listener throw error with onHandleError zone',
25442623
function(done: DoneFn) {
25452624
// override global.onerror to prevent jasmine report error
25462625
let oriWindowOnError = window.onerror;
25472626
window.onerror = function() {};
25482627
try {
2628+
const zone = Zone.current.fork({
2629+
name: 'error',
2630+
onHandleError: (delegate, curr, target, error) => {
2631+
logs.push('zone handled ' + target.name + ' ' + error.message);
2632+
return false;
2633+
}
2634+
});
25492635
let logs: string[] = [];
2636+
const listener1 = function() {
2637+
logs.push('listener1');
2638+
};
2639+
const listener2 = function() {
2640+
throw new Error('test1');
2641+
};
2642+
const listener3 = function() {
2643+
throw new Error('test2');
2644+
};
2645+
const listener4 = {
2646+
handleEvent: function() {
2647+
logs.push('listener2');
2648+
}
2649+
};
2650+
2651+
zone.runGuarded(() => {
2652+
button.addEventListener('click', listener1);
2653+
button.addEventListener('click', listener2);
2654+
button.addEventListener('click', listener3);
2655+
button.addEventListener('click', listener4);
2656+
});
2657+
2658+
const mouseEvent = document.createEvent('MouseEvent');
2659+
mouseEvent.initEvent('click', true, true);
2660+
2661+
const unhandledRejection = (e: PromiseRejectionEvent) => {
2662+
fail('should not be here');
2663+
};
2664+
window.addEventListener('unhandledrejection', unhandledRejection);
2665+
2666+
button.dispatchEvent(mouseEvent);
2667+
expect(logs).toEqual([
2668+
'listener1', 'zone handled error test1', 'zone handled error test2', 'listener2'
2669+
]);
2670+
2671+
setTimeout(() => {
2672+
expect(logs).toEqual([
2673+
'listener1', 'zone handled error test1', 'zone handled error test2', 'listener2'
2674+
]);
2675+
window.removeEventListener('unhandledrejection', unhandledRejection);
2676+
window.onerror = oriWindowOnError;
2677+
done();
2678+
});
2679+
} catch (e: any) {
2680+
window.onerror = oriWindowOnError;
2681+
}
2682+
});
2683+
2684+
it('should be able to continue to invoke remaining listeners even some listener throw error in the different zones',
2685+
function(done: DoneFn) {
2686+
// override global.onerror to prevent jasmine report error
2687+
let oriWindowOnError = window.onerror;
2688+
let logs: string[] = [];
2689+
window.onerror = function(err: any) {
2690+
logs.push(err);
2691+
};
2692+
try {
25502693
const zone1 = Zone.current.fork({
25512694
name: 'zone1',
25522695
onHandleError: (delegate, curr, target, error) => {
@@ -2580,18 +2723,18 @@ describe('Zone', function() {
25802723
mouseEvent.initEvent('click', true, true);
25812724

25822725
const unhandledRejection = (e: PromiseRejectionEvent) => {
2583-
logs.push(e.reason.message);
2726+
fail('should not be here');
25842727
};
25852728
window.addEventListener('unhandledrejection', unhandledRejection);
25862729

25872730
button.dispatchEvent(mouseEvent);
2588-
expect(logs).toEqual(['listener1', 'test1', 'listener2']);
2731+
expect(logs).toEqual(['listener1', 'test1', 'listener2', 'Uncaught Error: test2']);
25892732

25902733
setTimeout(() => {
2591-
expect(logs).toEqual(['listener1', 'test1', 'listener2', 'test2']);
2734+
expect(logs).toEqual(['listener1', 'test1', 'listener2', 'Uncaught Error: test2']);
25922735
window.removeEventListener('unhandledrejection', unhandledRejection);
25932736
window.onerror = oriWindowOnError;
2594-
done()
2737+
done();
25952738
});
25962739
} catch (e: any) {
25972740
window.onerror = oriWindowOnError;

0 commit comments

Comments
 (0)