Merge pull request #10868 from adobe/pflynn/listener-leak-canary

Listener leak "canary" warning

Author: Marcel Gerber

src/extensions/default/JavaScriptCodeHints/ParameterHintManager.js

@@ -389,14 +389,22 @@ define(function (require, exports, module) {
      *      changes
      */
     function installListeners(editor) {
-        editor.on("keydown", function (event, editor, domEvent) {
+        editor.on("keydown.ParameterHints", function (event, editor, domEvent) {
             if (domEvent.keyCode === KeyEvent.DOM_VK_ESCAPE) {
                 dismissHint();
             }
-        }).on("scroll", function () {
+        }).on("scroll.ParameterHints", function () {
             dismissHint();
         });
     }
+    
+    /**
+     * Clean up after installListeners()
+     * @param {!Editor} editor
+     */
+    function uninstallListeners(editor) {
+        editor.off(".ParameterHints");
+    }
 
     /**
      * Add the function hint command at start up.
@@ -429,6 +437,7 @@ define(function (require, exports, module) {
     exports.addCommands             = addCommands;
     exports.dismissHint             = dismissHint;
     exports.installListeners        = installListeners;
+    exports.uninstallListeners      = uninstallListeners;
     exports.isHintDisplayed         = isHintDisplayed;
     exports.popUpHint               = popUpHint;
     exports.popUpHintAtOpenParen    = popUpHintAtOpenParen;

src/extensions/default/JavaScriptCodeHints/main.js

@@ -640,6 +640,7 @@ define(function (require, exports, module) {
         function uninstallEditorListeners(editor) {
             if (editor) {
                 editor.off(HintUtils.eventName("change"));
+                ParameterHintManager.uninstallListeners(editor);
             }
         }
 

src/utils/EventDispatcher.js

@@ -58,6 +58,8 @@ define(function (require, exports, module) {
     "use strict";
 
     var _ = require("thirdparty/lodash");
+    
+    var LEAK_WARNING_THRESHOLD = 15;
 
 
     /**
@@ -88,6 +90,10 @@ define(function (require, exports, module) {
         var eventsList = events.split(/\s+/).map(splitNs),
             i;
 
+        if (!fn) {
+            throw new Error("EventListener.on() called with no listener fn for event '" + events + "'");
+        }
+        
         // Check for deprecation warnings
         if (this._deprecatedEvents) {
             for (i = 0; i < eventsList.length; i++) {
@@ -113,6 +119,11 @@ define(function (require, exports, module) {
             }
             eventsList[i].handler = fn;
             this._eventHandlers[eventName].push(eventsList[i]);
+            
+            // Check for suspicious number of listeners being added to one object-event pair
+            if (this._eventHandlers[eventName].length > LEAK_WARNING_THRESHOLD) {
+                console.error("Possible memory leak: " + this._eventHandlers[eventName].length + " '" + eventName + "' listeners attached to", this);
+            }
         }
 
         return this;  // for chaining

test/spec/EventDispatcher-test.js

@@ -23,7 +23,7 @@
 
 
 /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
-/*global jasmine, define, describe, beforeEach, it, expect */
+/*global jasmine, define, describe, beforeEach, it, expect, spyOn */
 
 define(function (require, exports, module) {
     "use strict";
@@ -429,5 +429,27 @@ define(function (require, exports, module) {
             expect(fn1).toHaveBeenCalled();
             expect(fn2).toHaveBeenCalled();
         });
+        
+        
+        it("on() should print warnings when too many listeners attached", function () {
+            function makeStubListener() {  // avoids JSLint "don't make functions in a loop" complaint
+                return function () {};
+            }
+            
+            spyOn(console, "error");
+            var i;
+            for (i = 0; i < 15; i++) {
+                dispatcher.on("foo", makeStubListener());
+            }
+            expect(console.error).not.toHaveBeenCalled();
+            
+            // Prints warnings when number of listeners exceeds 15
+            dispatcher.on("foo", fn1);
+            expect(console.error).toHaveBeenCalled();
+            
+            // ...but still attaches listener anyway
+            dispatcher.trigger("foo");
+            expect(fn1).toHaveBeenCalled();
+        });
     });
 });