PDA

View Full Version : [FIXED][3.0-r4884] Store destroyed twice



mjlecomte
29 Jul 2009, 7:37 AM
Ext version tested:


Ext 3.0 rev 4884



Adapter used:


ext



css used:


only default ext-all.css



Browser versions tested against:


FF3.5 (firebug 1.4 active)



Operating System:


WinXP Pro



Description:

Revision 4884 appears to introduce/expose a bug.

The essence of the problem is that when the grid with a paging toolbar is destroyed, both the PagingToolbar and the GridView are attempting to destroy the store. (Note: The problem was exposed by the cited revision (4884) because data is cleared the first time the store is destroyed. Revision 4884 merely exposes the underlying problem, the actual problem is that the store is destroyed twice.)

To interrogate the problem set a breakpoint in Store::destroy() method and monitor the store destroy method repeatedly get called for the same store (for the cited test case using grid filters demo set a watch expression for "this.storeId").

Test Case:

The problem can be most easily seen by going to examples/grid-filtering (recently uploaded so update your wc). That example has a window containing a grid, and the grid has a paging toolbar (therefore a store bound to the grid and the toolbar).

I'll post a modified array grid example which could also be used.

Note: oddly in my environment firebug only throws the error if NOT using ext-all.js (so non minified source of some kind for Store).


Steps to reproduce the problem:


Just load the example and close the window to see the error.



The result that was expected:



no error




The result that occurs instead:


"this.data is null" error



Screenshot or Video:


none


Debugging already done:

PagingToolbar destroy method is called first and destroys the store if autoDestroy is true:



onDestroy : function(){
this.bindStore(null);
...
},

bindStore : function(store, initial){

...
if(store !== this.store && this.store.autoDestroy){
this.store.destroy();
}
}


GridView is called later, so it should check before destroying the store again (via initData):



destroy : function(){
...
Ext.destroy(this.resizeMarker, this.resizeProxy, this.focusEl, this.mainBody,
this.scroller, this.mainHd, this.mainWrap, this.dragZone,
this.splitZone, this.columnDrag, this.columnDrop);
this.initData(null, null); // still call this method so colModel listeners are removed
Ext.EventManager.removeResizeListener(this.onWindowResize, this);
this.purgeListeners();
},

// private
initData : function(ds, cm){
if(this.ds && !this.ds.destroyed){
this.ds.un('load', this.onLoad, this);
this.ds.un('datachanged', this.onDataChange, this);
this.ds.un('add', this.onAdd, this);
this.ds.un('remove', this.onRemove, this);
this.ds.un('update', this.onUpdate, this);
this.ds.un('clear', this.onClear, this);
if(this.ds !== ds && this.ds.autoDestroy){
this.ds.destroy(); // store.destroy() purges listeners, so
// there is no need for all of the ds.un() above
// see more notes below [-1-]
}
}


Consistent with similar to recent revisions to Component suggest setting a destroy flag in Ext.data.Store.destroy() :



/**
* Destroys the store.
*/
destroy : function(){
if(this.storeId){
Ext.StoreMgr.unregister(this);
}
this.clearData();
this.data = null;
Ext.destroy(this.proxy);
this.reader = this.writer = null;
this.purgeListeners();
this.destroyed = true; // set flag to prevent repeat destruction
},


Referring to [-1-], GridView.initData() has some redundant code, destroying listeners twice, I think it should be adjusted a bit:



/* --------------------------------- Model Events and Handlers --------------------------------*/
// private
initData : function(ds, cm){
// ensure store not already destroyed
if(this.ds && !this.ds.destroyed) {

if(this.ds !== ds && this.ds.autoDestroy){
this.ds.destroy();
// if not destroying the store, remove listeners
} else {
this.ds.un('load', this.onLoad, this);
this.ds.un('datachanged', this.onDataChange, this);
this.ds.un('add', this.onAdd, this);
this.ds.un('remove', this.onRemove, this);
this.ds.un('update', this.onUpdate, this);
this.ds.un('clear', this.onClear, this);
}
}


Possible fix:



see debugging

mjlecomte
29 Jul 2009, 7:38 AM
Another example exposing the problem (again, you may need to use ext-all-debug to actually see the error?!):



Ext.onReady(function(){

// NOTE: This is an example showing simple state management. During development,
// it is generally best to disable state management as dynamically-generated ids
// can change across page loads, leading to unpredictable results. The developer
// should ensure that stable state ids are set for stateful components in real apps.
Ext.state.Manager.setProvider(new Ext.state.CookieProvider());

// sample static data for the store
var myData = [
['3m Co',71.72,0.02,0.03,'9/1 12:00am'],
['Alcoa Inc',29.01,0.42,1.47,'9/1 12:00am'],
['Altria Group Inc',83.81,0.28,0.34,'9/1 12:00am'],
['American Express Company',52.55,0.01,0.02,'9/1 12:00am'],
['American International Group, Inc.',64.13,0.31,0.49,'9/1 12:00am'],
['AT&T Inc.',31.61,-0.48,-1.54,'9/1 12:00am'],
['Boeing Co.',75.43,0.53,0.71,'9/1 12:00am'],
['Caterpillar Inc.',67.27,0.92,1.39,'9/1 12:00am'],
['Citigroup, Inc.',49.37,0.02,0.04,'9/1 12:00am'],
['E.I. du Pont de Nemours and Company',40.48,0.51,1.28,'9/1 12:00am'],
['Exxon Mobil Corp',68.1,-0.43,-0.64,'9/1 12:00am'],
['General Electric Company',34.14,-0.08,-0.23,'9/1 12:00am'],
['General Motors Corporation',30.27,1.09,3.74,'9/1 12:00am'],
['Hewlett-Packard Co.',36.53,-0.03,-0.08,'9/1 12:00am'],
['Honeywell Intl Inc',38.77,0.05,0.13,'9/1 12:00am'],
['Intel Corporation',19.88,0.31,1.58,'9/1 12:00am'],
['International Business Machines',81.41,0.44,0.54,'9/1 12:00am'],
['Johnson & Johnson',64.72,0.06,0.09,'9/1 12:00am'],
['JP Morgan & Chase & Co',45.73,0.07,0.15,'9/1 12:00am'],
['McDonald\'s Corporation',36.76,0.86,2.40,'9/1 12:00am'],
['Merck & Co., Inc.',40.96,0.41,1.01,'9/1 12:00am'],
['Microsoft Corporation',25.84,0.14,0.54,'9/1 12:00am'],
['Pfizer Inc',27.96,0.4,1.45,'9/1 12:00am'],
['The Coca-Cola Company',45.07,0.26,0.58,'9/1 12:00am'],
['The Home Depot, Inc.',34.64,0.35,1.02,'9/1 12:00am'],
['The Procter & Gamble Company',61.91,0.01,0.02,'9/1 12:00am'],
['United Technologies Corporation',63.26,0.55,0.88,'9/1 12:00am'],
['Verizon Communications',35.57,0.39,1.11,'9/1 12:00am'],
['Wal-Mart Stores, Inc.',45.45,0.73,1.63,'9/1 12:00am']
];

/**
* Custom function used for column renderer
* @param {Object} val
*/
function change(val){
if(val > 0){
return '<span style="color:green;">' + val + '</span>';
}else if(val < 0){
return '<span style="color:red;">' + val + '</span>';
}
return val;
}

/**
* Custom function used for column renderer
* @param {Object} val
*/
function pctChange(val){
if(val > 0){
return '<span style="color:green;">' + val + '%</span>';
}else if(val < 0){
return '<span style="color:red;">' + val + '%</span>';
}
return val;
}

// create the data store
var store = new Ext.data.ArrayStore({
autoDestroy: true, // required to expose the problem
fields: [
{name: 'company'},
{name: 'price', type: 'float'},
{name: 'change', type: 'float'},
{name: 'pctChange', type: 'float'},
{name: 'lastChange', type: 'date', dateFormat: 'n/j h:ia'}
]
});

// create the Grid
var grid = new Ext.grid.GridPanel({
store: store,
columns: [
{id:'company',header: 'Company', width: 160, sortable: true, dataIndex: 'company'},
{header: 'Price', width: 75, sortable: true, renderer: 'usMoney', dataIndex: 'price'},
{header: 'Change', width: 75, sortable: true, renderer: change, dataIndex: 'change'},
{header: '% Change', width: 75, sortable: true, renderer: pctChange, dataIndex: 'pctChange'},
{header: 'Last Updated', width: 85, sortable: true, renderer: Ext.util.Format.dateRenderer('m/d/Y'), dataIndex: 'lastChange'}
],
stripeRows: true,
autoExpandColumn: 'company',
height: 350,
width: 600,
bbar: new Ext.PagingToolbar({store: store}),
title: 'Array Grid',
// config options for stateful behavior
stateful: true,
stateId: 'grid'
});

// manually load local data
store.loadData(myData);

// a window is used to facilite destroying the grid.
var win = new Ext.Window({
width: 600,
height: 350,
layout: 'fit',
items: [grid]
}).show();

});

mystix
29 Jul 2009, 8:50 AM
i think it should suffice to simply perform a check in clearData():


Ext.override(Ext.data.Store, {
clearData: function() {
if (this.data && this.data.each) {
this.data.each(function(rec) {
rec.join(null);
});
this.data.clear();
}
}
});


as Components get more complex, it's inevitable they'll start sharing stuff (like the Store in this case) between themselves, so it's best to assume absolutely nothing about dependencies and simply make sure the destroy() code is able to handle redundant calls. i'd go with @mj's suggestion and put a destroyed check in there.

mjlecomte
29 Jul 2009, 9:44 AM
Yeah, so what might be a tad better is to just do the checking at Store::destroy() so any dependent classes need not do the check:


/**
* Destroys the store.
*/
destroy : function(){
if(!this.destroyed){
if(this.storeId){
Ext.StoreMgr.unregister(this);
}
this.clearData();
this.data = null;
Ext.destroy(this.proxy);
this.reader = this.writer = null;
this.purgeListeners();
this.destroyed = true; // set flag to prevent repeat destruction
}

},

Condor
29 Jul 2009, 10:11 AM
While we are on the subject, Ext.Component could use a destroyed flag also.

(some delayed event handlers, e.g. mimicBlur, should check the destroyed flag before continuing).

mjlecomte
29 Jul 2009, 10:18 AM
While we are on the subject, Ext.Component could use a destroyed flag also.

(some delayed event handlers, e.g. mimicBlur, should check the destroyed flag before continuing).

Ext.Component does have a destroy flag now! It was added rev 4894.

Any checks by subclasses have NOT been added (yet). If a subclass should be checking that flag I suggest you start another thread so they can be tracked properly.

Component now has:


destroy : function(){
if(!this.isDestroyed){
if(this.fireEvent('beforedestroy', this) !== false){
this.beforeDestroy();
if(this.rendered){
this.el.removeAllListeners();
this.el.remove();
if(this.actionMode == 'container' || this.removeMode == 'container'){
this.container.remove();
}
}
this.onDestroy();
Ext.ComponentMgr.unregister(this);
this.fireEvent('destroy', this);
this.purgeListeners();
this.isDestroyed = true;
}
}
},

evant
29 Jul 2009, 8:43 PM
A fix for this has been added to SVN.