defer/setTimeout race conditions needs to be cleaned up

[mankz]

Seeing a few crashes in IE here and there, caused by improper use of Ext.defer/setTimeout. You need to add a new way to tie timers to the life span of the owning object. Just like you do with component.mon, let's add:

component.defer(someFn, 1);

// or

component.setTimeout(fn, 1)

So that you can easily kill these timers on component destroy.

// This code crashes for me in IE11, since button somehow managed to destroy itself before this timer fired.

onMouseDown: function(e) {
        var me = this;
        if (Ext.isIE || e.pointerType === 'touch') {
            // In IE the use of unselectable on the button's elements causes the element
            // to not receive focus, even when it is directly clicked.
            // On Touch devices, we need to explicitly focus on touchstart.
            Ext.defer(function() {
                var focusEl = me.getFocusEl();
                // Deferred to give other mousedown handlers the chance to preventDefault
                if (focusEl && !e.defaultPrevented) {
                    focusEl.focus();
                }
            }, 1);
        }
        if (!me.disabled && e.button === 0) {
            Ext.button.Manager.onButtonMousedown(me, e);
            me.addCls(me._pressedCls);
        }
    },

[mankz]

For anyone seeing this issue with Element#focus, here's a workaround.

/*
* Handle Ext JS internal race conditions, timers not destroyed when component is.
* @version ExtJS 6.2.0
*
 * */
Ext.define('Web.overrides.Element', {
    override   : 'Ext.Element',

    focus : function () {
        if (!this.destroyed) {
            return this.callParent(arguments);
        }
    }
});

[MaximGB]

If we consider a timeout/defered call as another type of event than maybe modifying on method logic is also an option:

Event name is number thus we can distinguish between timeout/interval value and event name, the following call should be handled with setInterval() since there's no { single : true } subscribing option given

component.on(1000, someFn);

The next call should be handled with setTimeout() instead

component.on(1000, someFn, null, { single : true })

0 timeouts/intervals should be handled the same way Ext.asap() does

[mitchellsimoens]

I've pushed this up.

@MaximGB, I don't know why we'd want that. The error here is that we do certain things which may create a racing condition with the component lifecycle especially the destruction. Not sure why changing the on/addListener signature would be related or wanted here.

[MaximGB]

Well, the Mat's idea is to automatically call clearTimeout() / clearInterval() for running defered operations upon component destruction. Since observable unregisters any listener upon destruction already, I though that it might cancel any defered operation as well. To destinguish between listener and defered call I proposed to use first argument type: numeric - defered, string - event listener.

[nohuhu]

@MaximGB,

One more obscure API won't solve the problem because a) nobody is going to be aware of it and hence won't use it, and b) it doesn't do anything for existing code with Ext.defer or setTimeout that is all over the place not only in the framework but also in apps built with the framework.

Instead we're working on tighter resource leak control in our unit tests, and that will include timers. Hopefully that will allow us to solve the problem at least on the framework side, which is where the majority of leaks are anyway.

[JEBriggs]

We're experiencing this same bug in multiple places. The Element override above works as a fix but I'm torn about whether to deploy the fix in production because I haven't see the error have any side effects other than show up in the console and trigger the IE debugger if the dev. console is open. Any thoughts about other side effects or whether a temporary workaround should be put in place?

[Animal]

There is a way to tie timers to the the lifecycle of the component.

It's Ext.Mixin.Bufferable

It just needs to be used a bit more. It was added in probably 6.5.0?