Undefined Removal Order and Behavior caused by XF.off

[pegasus]

Affected version

2.3.4

When using the Javascript method XF.off to remove an event listener from a known element, the method removes a matching handler from XF.eventHandlers even though the evaluated handler may actually belong to a different element, usually when multiple elements share the same handler. At first glance this may seem safe, because there logically you would expect the same resulting number of handlers in the cache that can still be removed individually, if in an unexpected order.

However, when a handler is .on multiple elements, .off-ing it from one results in the cache for all elements using that handler being cleared (position of "return false"), but only the requested element actually has its handler removed. This is especially problematic when using the method without specifying a handler at all.

In practice, we tend to only remove listeners from a specific element under specific conditions (such as clicking a button, toggling its class, and giving it a different listener). When using XF.on/off in sequences on multiple elements, this leads to situations where some elements do not successfully have their handlers removed, resulting in ever-increasing stacks of handlers on the element that were expected to be removed but were not. This leads to undefined (and frustrating) behaviors.

The problem does not occur if XF.off is circumvented and element.removeEventListener is called directly.

The problem appears to be in js/xf/core.js:

                XF.eventHandlers[namespace][event] = XF.eventHandlers[namespace][event].filter(handlerData =>
                {
                    if (!handler || handler === handlerData.handler || handler === handlerData.handler.originalHandler)
                    {
                        element.removeEventListener(event, handlerData.handler, options)
                        return false
                    }
                    return true
                })

I have not seen the problem reoccur since making a small tweak to the above. The situation seems to be resolved when the inner condition is changed to only evaluate when handlerData.element === element.

                XF.eventHandlers[namespace][event] = XF.eventHandlers[namespace][event].filter(handlerData =>
                {
                    if (element === handlerData.element && (!handler || handler === handlerData.handler || handler === handlerData.handler.originalHandler))
                    {
                        element.removeEventListener(event, handlerData.handler, options)
                        return false
                    }
                    return true
                })

This bug was found in 2.3.4, but is also present in js/xf/core.js here on xenforo.com.

A similar issue exists elsewhere in the XF.off method when not specifying an event name. The result is that all handlers in a namespace are removed from cache, regardless of the specified element. This will lead to similar problems, although I have not tried that permutation.

                    Object.keys(XF.eventHandlers[namespace]).forEach(event =>
                    {
                        XF.eventHandlers[namespace][event].forEach(handlerData =>
                        {
                            element.removeEventListener(event, handlerData.handler, options)
                        })
                    })
                    delete XF.eventHandlers[namespace]

Should probably be:

                    Object.keys(XF.eventHandlers[namespace]).forEach(event =>
                    {
                        XF.off(element, event + "." + namespace, handler, options)
                    })
                    if (!Object.keys(XF.eventHandlers[namespace]).length)
                    {
                        delete XF.eventHandlers[namespace]
                    }

If the goal is to allow removal from all elements, then a special case must be added that specifically tests that.