Safer event deleting (#2883)

made handler removal safer. Add edge cases for event handler adding and deleting.
fabricjs · Apr 13, 2016 · fd95729 · fd95729
1 parent 37b2099
commit fd95729
Show file tree

Hide file tree

Showing 3 changed files with 125 additions and 6 deletions.
diff --git a/src/mixins/observable.mixin.js b/src/mixins/observable.mixin.js
@@ -9,12 +9,12 @@
     if (!this.__eventListeners[eventName]) {
       return;
     }
-
+    var eventListener = this.__eventListeners[eventName];
     if (handler) {
-      fabric.util.removeFromArray(this.__eventListeners[eventName], handler);
+      eventListener[eventListener.indexOf(handler)] = false;
     }
     else {
-      this.__eventListeners[eventName].length = 0;
+      fabric.util.array.fill(eventListener, false);
     }
   }
 
@@ -65,7 +65,9 @@
 
     // remove all key/value pairs (event name -> event handler)
     if (arguments.length === 0) {
-      this.__eventListeners = { };
+      for (eventName in this.__eventListeners) {
+        _removeEventListener.call(this, eventName);
+      }
     }
     // one object with key/value pairs was passed
     else if (arguments.length === 1 && typeof arguments[0] === 'object') {
@@ -100,9 +102,11 @@
     }
 
     for (var i = 0, len = listenersForEvent.length; i < len; i++) {
-      // avoiding try/catch for perf. reasons
-      listenersForEvent[i].call(this, options || { });
+      listenersForEvent[i] && listenersForEvent[i].call(this, options || { });
     }
+    this.__eventListeners[eventName] = listenersForEvent.filter(function(value) {
+      return value !== false;
+    });
     return this;
   }
 

diff --git a/src/util/lang_array.js b/src/util/lang_array.js
@@ -211,6 +211,17 @@
     });
   }
 
+  /**
+   * @private
+   */
+  function fill(array, value) {
+    var k = array.length;
+    while (k--) {
+      array[k] = value;
+    }
+    return array;
+  }
+
   /**
    * @private
    */
@@ -242,6 +253,7 @@
    * @namespace fabric.util.array
    */
   fabric.util.array = {
+    fill: fill,
     invoke: invoke,
     min: min,
     max: max

diff --git a/test/unit/observable.js b/test/unit/observable.js
@@ -169,6 +169,109 @@ test('trigger', function() {
   equal(context, foo);
 });
 
+test('removal of past events', function() {
+  var foo = { },
+      event1Fired = false, event2Fired = false,
+      event3Fired = false, event4Fired = false,
+      handler1 = function() {
+        event1Fired = true;
+        foo.off('bar:baz', handler1);
+      },
+      handler2 = function() {
+        event2Fired = true;
+      },
+      handler3 = function() {
+        event3Fired = true;
+      },
+      handler4 = function() {
+        event4Fired = true;
+      };
+
+  fabric.util.object.extend(foo, fabric.Observable);
+  foo.on('bar:baz', handler1);
+  foo.on('bar:baz', handler2);
+  foo.on('bar:baz', handler3);
+  foo.on('bar:baz', handler4);
+  equal(foo.__eventListeners['bar:baz'].length, 4, 'There should be 4 events registered now');
+  foo.trigger('bar:baz');
+  equal(foo.__eventListeners['bar:baz'].length, 3, 'There should be 3 events registered now');
+  equal(event1Fired, true, 'Event 1 should fire');
+  equal(event2Fired, true, 'Event 2 should fire');
+  equal(event3Fired, true, 'Event 3 should fire');
+  equal(event4Fired, true, 'Event 4 should fire');
+});
+
+test('removal of past events inner loop', function() {
+  var foo = { },
+      event1Fired = 0, event2Fired = 0,
+      event3Fired = 0, event4Fired = 0,
+      handler1 = function() {
+        event1Fired++;
+        foo.off('bar:baz', handler1);
+        equal(foo.__eventListeners['bar:baz'].length, 4, 'There should be still 4 handlers registered');
+        equal(event1Fired, 1, 'Event 1 should fire once');
+        equal(event2Fired, 0, 'Event 2 should not be fired yet');
+        equal(event3Fired, 0, 'Event 3 should not be fired yet');
+        equal(event4Fired, 0, 'Event 4 should not be fired yet');        
+        foo.trigger('bar:baz');
+        equal(foo.__eventListeners['bar:baz'].length, 3, 'There should be 3 handlers registered now');
+      },
+      handler2 = function() {
+        event2Fired++;
+      },
+      handler3 = function() {
+        event3Fired++;
+      },
+      handler4 = function() {
+        event4Fired++;
+      };
+
+  fabric.util.object.extend(foo, fabric.Observable);
+  foo.on('bar:baz', handler1);
+  foo.on('bar:baz', handler2);
+  foo.on('bar:baz', handler3);
+  foo.on('bar:baz', handler4);
+  foo.trigger('bar:baz');
+  equal(event1Fired, 1, 'Event 1 should fire once');
+  equal(event2Fired, 2, 'Event 2 should fire twice');
+  equal(event3Fired, 2, 'Event 3 should fire twice');
+  equal(event4Fired, 2, 'Event 4 should fire twice');
+});
+
+test('adding events', function() {
+  var foo = { },
+      event1Fired = false, event2Fired = false,
+      event3Fired = false, event4Fired = false,
+      handler1 = function() {
+        event1Fired = true;
+        foo.off('bar:baz', handler1);
+        foo.on('bar:baz', handler3);
+        foo.on('bar:baz', handler4);
+      },
+      handler2 = function() {
+        event2Fired = true;
+      },
+      handler3 = function() {
+        event3Fired = true;
+      },
+      handler4 = function() {
+        event4Fired = true;
+      };
+
+  fabric.util.object.extend(foo, fabric.Observable);
+  foo.on('bar:baz', handler1);
+  foo.on('bar:baz', handler2);
+  foo.trigger('bar:baz');
+  equal(event1Fired, true, 'Event 1 should fire');
+  equal(event2Fired, true, 'Event 2 should fire');
+  equal(event3Fired, false, 'Event 3 should not fire');
+  equal(event4Fired, false, 'Event 4 should not fire');
+  foo.trigger('bar:baz');
+  equal(event3Fired, true, 'Event 3 should be triggered now');
+  equal(event4Fired, true, 'Event 4 should be triggered now');
+});
+
+
 test('chaining', function() {
   var foo = { };
   fabric.util.object.extend(foo, fabric.Observable);