fix(event): fix memory leak for once, add more test cases (#841)

* fix(event): fix memory leak for once, add more test cases

* add comments

Author: JiaLiPassion

Diff for: lib/browser/browser.ts

@@ -43,8 +43,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) {
-    // TODO: @JiaLiPassion, add this back later.
-    api.patchEventTargetMethods(XMLHttpRequestEventTarget.prototype);
+    api.patchEventTarget(global, [XMLHttpRequestEventTarget.prototype]);
   }
   patchClass('MutationObserver');
   patchClass('WebKitMutationObserver');

Diff for: lib/browser/event-target.ts

@@ -101,7 +101,8 @@ export function eventTargetPatch(_global: any, api: _ZonePrivate) {
     const type = _global[apis[i]];
     apiTypes.push(type && type.prototype);
   }
-  patchEventTarget(api, _global, apiTypes, {validateHandler: checkIEAndCrossContext});
+  patchEventTarget(_global, apiTypes, {validateHandler: checkIEAndCrossContext});
+  api.patchEventTarget = patchEventTarget;
 
   return true;
 }

Diff for: lib/browser/shadydom.ts

@@ -14,11 +14,11 @@ Zone.__load_patch('shadydom', (global: any, Zone: ZoneType, api: _ZonePrivate) =
   if (windowPrototype && windowPrototype.hasOwnProperty('addEventListener')) {
     (windowPrototype as any)[Zone.__symbol__('addEventListener')] = null;
     (windowPrototype as any)[Zone.__symbol__('removeEventListener')] = null;
-    api.patchEventTargetMethods(windowPrototype);
+    api.patchEventTarget(global, [windowPrototype]);
   }
   if (Node.prototype.hasOwnProperty('addEventListener')) {
     (Node.prototype as any)[Zone.__symbol__('addEventListener')] = null;
     (Node.prototype as any)[Zone.__symbol__('removeEventListener')] = null;
-    api.patchEventTargetMethods(Node.prototype);
+    api.patchEventTarget(global, [Node.prototype]);
   }
 });

Diff for: lib/browser/webapis-media-query.ts

@@ -9,7 +9,7 @@ Zone.__load_patch('mediaQuery', (global: any, Zone: ZoneType, api: _ZonePrivate)
   if (!global['MediaQueryList']) {
     return;
   }
-  api.patchEventTargetMethods(
-      global['MediaQueryList'].prototype,
+  api.patchEventTarget(
+      global, [global['MediaQueryList'].prototype],
       {addEventListenerFnName: 'addListener', removeEventListenerFnName: 'removeListener'});
 });

Diff for: lib/common/events.ts

@@ -43,7 +43,24 @@ export interface PatchEventTargetOptions {
 }
 
 export function patchEventTarget(
-    api: _ZonePrivate, _global: any, apis: any[], patchOptions?: PatchEventTargetOptions) {
+    _global: any, apis: any[], patchOptions?: PatchEventTargetOptions) {
+  const ADD_EVENT_LISTENER =
+      (patchOptions && patchOptions.addEventListenerFnName) || 'addEventListener';
+  const REMOVE_EVENT_LISTENER =
+      (patchOptions && patchOptions.removeEventListenerFnName) || 'removeEventListener';
+
+  const LISTENERS_EVENT_LISTENER =
+      (patchOptions && patchOptions.listenersFnName) || 'eventListeners';
+  const REMOVE_ALL_LISTENERS_EVENT_LISTENER =
+      (patchOptions && patchOptions.removeAllFnName) || 'removeAllListeners';
+
+  const zoneSymbolAddEventListener = zoneSymbol(ADD_EVENT_LISTENER);
+
+  const ADD_EVENT_LISTENER_SOURCE = '.' + ADD_EVENT_LISTENER + ':';
+
+  const PREPEND_EVENT_LISTENER = 'prependListener';
+  const PREPEND_EVENT_LISTENER_SOURCE = '.' + PREPEND_EVENT_LISTENER + ':';
+
   const invokeTask = function(task: any, target: any, event: Event) {
     // for better performance, check isRemoved which is set
     // by removeEventListener
@@ -52,12 +69,20 @@ export function patchEventTarget(
     }
     const delegate = task.callback;
     if (typeof delegate === OBJECT_TYPE && delegate.handleEvent) {
-      // create the bind version of handleEvnet when invoke
+      // create the bind version of handleEvent when invoke
       task.callback = (event: Event) => delegate.handleEvent(event);
       task.originalDelegate = delegate;
     }
     // invoke static task.invoke
     task.invoke(task, target, [event]);
+    const options = task.options;
+    if (options && typeof options === 'object' && options.once) {
+      // if options.once is true, after invoke once remove listener here
+      // only browser need to do this, nodejs eventEmitter will cal removeListener
+      // inside EventEmitter.once
+      const delegate = task.originalDelegate ? task.originalDelegate : task.callback;
+      target[REMOVE_EVENT_LISTENER].apply(target, [event.type, delegate, options]);
+    }
   };
 
   // global shared zoneAwareCallback to handle all event callback with capture = false
@@ -67,6 +92,7 @@ export function patchEventTarget(
     const tasks = target[zoneSymbolEventNames[event.type][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
       if (tasks.length === 1) {
         invokeTask(tasks[0], target, event);
       } else {
@@ -88,6 +114,7 @@ export function patchEventTarget(
     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 {
@@ -106,22 +133,6 @@ export function patchEventTarget(
     if (!obj) {
       return false;
     }
-    const ADD_EVENT_LISTENER =
-        (patchOptions && patchOptions.addEventListenerFnName) || 'addEventListener';
-    const REMOVE_EVENT_LISTENER =
-        (patchOptions && patchOptions.removeEventListenerFnName) || 'removeEventListener';
-
-    const LISTENERS_EVENT_LISTENER =
-        (patchOptions && patchOptions.listenersFnName) || 'eventListeners';
-    const REMOVE_ALL_LISTENERS_EVENT_LISTENER =
-        (patchOptions && patchOptions.removeAllFnName) || 'removeAllListeners';
-
-    const zoneSymbolAddEventListener = zoneSymbol(ADD_EVENT_LISTENER);
-
-    const ADD_EVENT_LISTENER_SOURCE = '.' + ADD_EVENT_LISTENER + ':';
-
-    const PREPEND_EVENT_LISTENER = 'prependListener';
-    const PREPEND_EVENT_LISTENER_SOURCE = '.' + PREPEND_EVENT_LISTENER + ':';
 
     let useGlobalCallback = true;
     if (patchOptions && patchOptions.useGlobalCallback !== undefined) {
@@ -188,6 +199,34 @@ export function patchEventTarget(
     };
 
     const customCancelGlobal = function(task: any) {
+      // if task is not marked as isRemoved, this call is directly
+      // from Zone.prototype.cancelTask, we should remove the task
+      // from tasksList of target first
+      if (!task.isRemoved) {
+        const symbolEventNames = zoneSymbolEventNames[task.eventName];
+        let symbolEventName;
+        if (symbolEventNames) {
+          symbolEventName = symbolEventNames[task.capture ? TRUE_STR : FALSE_STR];
+        }
+        const existingTasks = symbolEventName && task.target[symbolEventName];
+        if (existingTasks) {
+          for (let i = 0; i < existingTasks.length; i++) {
+            const existingTask = existingTasks[i];
+            if (existingTask === task) {
+              existingTasks.splice(i, 1);
+              // set isRemoved to data for faster invokeTask check
+              task.isRemoved = true;
+              if (existingTasks.length === 0) {
+                // all tasks for the eventName + capture have gone,
+                // remove globalZoneAwareCallback and remove the task cache from target
+                task.allRemoved = true;
+                task.target[symbolEventName] = null;
+              }
+              break;
+            }
+          }
+        }
+      }
       // if all tasks for the eventName + capture have gone,
       // we will really remove the global event callback,
       // if not, return
@@ -262,6 +301,7 @@ export function patchEventTarget(
         const options = arguments[2];
 
         let capture;
+        let once = false;
         if (options === undefined) {
           capture = false;
         } else if (options === true) {
@@ -270,6 +310,7 @@ export function patchEventTarget(
           capture = false;
         } else {
           capture = options ? !!options.capture : false;
+          once = options ? !!options.once : false;
         }
 
         const zone = Zone.current;
@@ -316,6 +357,12 @@ export function patchEventTarget(
         // do not create a new object as task.data to pass those things
         // just use the global shared one
         taskData.options = options;
+        if (once) {
+          // if addEventListener with once options, we don't pass it to
+          // native addEventListener, instead we keep the once setting
+          // and handle ourselves.
+          taskData.options.once = false;
+        }
         taskData.target = target;
         taskData.capture = capture;
         taskData.eventName = eventName;
@@ -327,6 +374,9 @@ export function patchEventTarget(
 
         // have to save those information to task in case
         // application may call task.zone.cancelTask() directly
+        if (once) {
+          options.once = true;
+        }
         task.options = options;
         task.target = target;
         task.capture = capture;
@@ -434,10 +484,15 @@ export function patchEventTarget(
           const prop = keys[i];
           const match = EVENT_NAME_SYMBOL_REGX.exec(prop);
           let evtName = match && match[1];
+          // in nodejs EventEmitter, removeListener event is
+          // used for monitoring the removeListener call,
+          // so just keep removeListener eventListener until
+          // all other eventListeners are removed
           if (evtName && evtName !== 'removeListener') {
             this[REMOVE_ALL_LISTENERS_EVENT_LISTENER].apply(this, [evtName]);
           }
         }
+        // remove removeListener listener finally
         this[REMOVE_ALL_LISTENERS_EVENT_LISTENER].apply(this, ['removeListener']);
       } else {
         const symbolEventNames = zoneSymbolEventNames[eventName];
@@ -486,7 +541,6 @@ export function patchEventTarget(
     results[i] = patchEventTargetMethods(apis[i], patchOptions);
   }
 
-  api.patchEventTargetMethods = patchEventTargetMethods;
   return results;
 }
 

Diff for: lib/node/events.ts

@@ -33,7 +33,7 @@ Zone.__load_patch('EventEmitter', (global: any, Zone: ZoneType, api: _ZonePrivat
   };
 
   function patchEventEmitterMethods(obj: any) {
-    const result = patchEventTarget(api, global, [obj], {
+    const result = patchEventTarget(global, [obj], {
       useGlobalCallback: false,
       addEventListenerFnName: EE_ADD_LISTENER,
       removeEventListenerFnName: EE_REMOVE_LISTENER,

Diff for: lib/zone.ts

@@ -320,7 +320,7 @@ interface _ZonePrivate {
   onUnhandledError: (error: Error) => void;
   microtaskDrainDone: () => void;
   showUncaughtError: () => boolean;
-  patchEventTargetMethods: (obj: any, options?: any) => boolean;
+  patchEventTarget: (global: any, apis: any[], options?: any) => boolean[];
   patchOnProperties: (obj: any, properties: string[]) => void;
   patchMethod:
       (target: any, name: string,
@@ -1307,7 +1307,7 @@ const Zone: ZoneType = (function(global: any) {
     microtaskDrainDone: noop,
     scheduleMicroTask: scheduleMicroTask,
     showUncaughtError: () => !(Zone as any)[__symbol__('ignoreConsoleErrorUncaughtError')],
-    patchEventTargetMethods: () => false,
+    patchEventTarget: () => [],
     patchOnProperties: noop,
     patchMethod: () => noop,
   };