PDA

View Full Version : [FIXED-512] Ext.layout.ContainerLayout: race condition in resizeTask



Jaroslav Snajdr
18 Dec 2009, 4:39 AM
Ext version tested:

Ext 3.0.3
Ext 3.1.0


Browser versions tested against:

FF3 (firebug 1.3.0.10 installed)


Operating System:

Snow Leopard


Description:

When ContainerLayout is being resized, it creates a DelayedTask to call method runLayout 50ms later.

However, when the component with this layout is destroyed before the task is run, the runLayout method is called on already destroyed object. This leads to a JavaScript exception about undefined DOM elements somewhere down the call stack. (Or some other error - quite random things can happen.)

We discovered the bug in our application when solving bug report like "when I click around fast enough in the app navigation, sooner or later I always get a JS error"

Possible fix:

Cancel the resizeTask in the ContainerLayout.destroy method:



destroy: function() {
if (this.resizeTask) {
this.resizeTask.cancel();
}
/* ... rest of the method ... */
}

Animal
18 Dec 2009, 7:38 AM
I would recommend setting Ext.Container.prototype.bufferResize = false

Jaroslav Snajdr
18 Dec 2009, 7:54 AM
That would considerably slow down my application when resizing, wouldn't it?

Or is bufferResize some kind of obsolete feature that doesn't really improve performance and is not worth fixing?

Condor
18 Dec 2009, 8:28 AM
There are more instances in the Ext code that create an Ext.util.DelayedTask, but do not destroy it when the component is destroyed:


Ext.layout.ContainerLayout.onResize - resizeTask
Ext.layout.BorderLayout.Region.initAutoHide - st
Ext.direct.RemotingProvider.queueTransaction - callTask
Ext.chart.Chart.delayRefresh - refreshTask

Animal
18 Dec 2009, 11:36 AM
That would considerably slow down my application when resizing, wouldn't it?

Or is bufferResize some kind of obsolete feature that doesn't really improve performance and is not worth fixing?


I don't think it improves performance.

bsaks
1 Feb 2010, 4:42 PM
I've also noticed a couple of similar cases involving the defer() method instead of Ext.util.DelayedTask:



Ext.grid.GridPanel.afterRender - v.afterRender.defer(10, this.view);
Ext.grid.GridView.onLoad - this.scrollToTop.defer(Ext.isGecko ? 1 : 0, this);


In both cases, storing the return value and passing it to clearTimeout() when the "destroy" event fired seemed to fix the problem.

Jamie Avins
1 Feb 2010, 5:59 PM
We will look at this around a 3.2 timeframe to address all cases. Any test cases that can be provided where we can reproduce the issue consistently would be of great help. We may need an approach which tackles this from both a removal side and a sanity check in case the delayed function is called after for any reason.

Jamie Avins
18 Feb 2010, 5:32 PM
Fixed in SVN 6112. Updated destroy methods for ContainerLayout, BorderLayout, Charts, GridView, and GridPanel. Direct Providers do not have a lifecycle.