Revise handler stack implementation.

The old implementation:
- Wasn't actually checking whether handlers had been removed before
  calling them.
- Could end up calling the same handler twice (if a handler was removed
  further down the stack, and the stack elements moved due the resulting
  splice.

Solution:
- Mark elements as removed and check.  Set their ids to null.
- Don't splice stack.

Also, optimisation:
- Removing the element at the top of the stack is still O(1).
- In Modes, reverse handlers before removing (so, more likely to hit the
  optimisation above).

For the record, the stable stack length at the moment seems to be about
10-12 elements.

2 files changed, 22 insertions, 14 deletions

diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index 10b7bb2a..76b65a12 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -76,7 +76,9 @@ class Mode
 
   exit: ->
     console.log @count, "exit:", @name
-    handlerStack.remove handlerId for handlerId in @handlers
+    # We reverse @handlers, here.  That way, handlers are popped in the opposite order to that in which they
+    # were pushed.
+    handlerStack.remove handlerId for handlerId in @handlers.reverse()
     Mode.modes = Mode.modes.filter (mode) => mode != @
     Mode.updateBadge()
 
diff --git a/lib/handler_stack.coffee b/lib/handler_stack.coffee
index f05413d3..c8e9a035 100644
--- a/lib/handler_stack.coffee
+++ b/lib/handler_stack.coffee
@@ -14,24 +14,24 @@ class HandlerStack
     # processing should take place.
     @stopBubblingAndFalse = new Object()
 
-  genId: -> @counter = ++@counter
-
   # Adds a handler to the stack. Returns a unique ID for that handler that can be used to remove it later.
   push: (handler) ->
-    handler.id = @genId()
     @stack.push handler
-    handler.id
+    handler.id = ++@counter
 
-  # Called whenever we receive a key event. Each individual handler has the option to stop the event's
-  # propagation by returning a falsy value.
+  # Called whenever we receive a key or other event. Each individual handler has the option to stop the
+  # event's propagation by returning a falsy value, or stop bubbling by returning @stopBubblingAndFalse or
+  # @stopBubblingAndTrue.
   bubbleEvent: (type, event) ->
+    # extra is passed to each handler.  This allows handlers to pass information down the stack.
+    extra = {}
     for i in [(@stack.length - 1)..0] by -1
       handler = @stack[i]
       # We need to check for existence of handler because the last function call may have caused the release
       # of more than one handler.
-      if handler && handler[type]
+      if handler and handler.id and handler[type]
         @currentId = handler.id
-        passThrough = handler[type].call(@, event)
+        passThrough = handler[type].call @, event, extra
         if not passThrough
           DomUtils.suppressEvent(event) if @isChromeEvent event
           return false
@@ -40,11 +40,17 @@ class HandlerStack
     true
 
   remove: (id = @currentId) ->
-    for i in [(@stack.length - 1)..0] by -1
-      handler = @stack[i]
-      if handler.id == id
-        @stack.splice(i, 1)
-        break
+    if 0 < @stack.length and @stack[@stack.length-1].id == id
+      # A common case is to remove the handler at the top of the stack.  And we can this very efficiently.
+      # Tests suggest that this case arises more than half of the time.
+      @stack.pop().id = null
+    else
+      # Otherwise, we'll build a new stack.  This is better than splicing the existing stack since at can't
+      # interfere with any concurrent bubbleEvent.
+      @stack = @stack.filter (handler) ->
+        # Mark this handler as removed (for any active bubbleEvent call).
+        handler.id = null if handler.id == id
+        handler?.id?
 
   # The handler stack handles chrome events (which may need to be suppressed) and internal (fake) events.
   # This checks whether that the event at hand is a chrome event.