tonytomov/jqGrid — pull request #308 by OlegKi (2012-04-21 17:01:36+00:00)
Title: Some improvements to reduse memory leaks

In "JQuery Core Style Guidelines" it will be recommended "Don't attach expandos to nodes - only attach data using .data()." (see [here](http://docs.jquery.com/JQuery_Core_Style_Guidelines#Nodes). One do can use expandos, but it's important to clear expandos and other references on the DOM elements before deleting ot from the page.

The current suggestions can't full eliminate all existing memory leaks, but it will be step in the correct direction. Additionally using call of `emptyRows` I tried to reduce the size of such leaks.

I try to explain the problem which I see on the following example:

In some methods like `refreshIndex` will be used reference to `ts` (see [here](https://github.com/tonytomov/jqGrid/blob/v4.3.2/js/grid.base.js#L1065) for exampele). On the same time the methods will be saved as expandos ot `ts` itself (see [here](https://github.com/tonytomov/jqGrid/blob/v4.3.2/js/grid.base.js#L2583)). If follows to the problem that the `

` element can't be removed inside of [GridDestroy](https://github.com/tonytomov/jqGrid/blob/v4.3.2/js/grid.custom.js#L63-74) or [GridUnload](https://github.com/tonytomov/jqGrid/blob/v4.3.2/js/grid.custom.js#L75-95) (by the line like [$("#gbox_"+gid).remove();](https://github.com/tonytomov/jqGrid/blob/v4.3.2/js/grid.custom.js#L93) for example) without memory leaks.

One get almost the whole grid as detached element instead of removing and the memory will be not free.

So I suggest to do the following:

1. call this.grid.emptyRows(this.grid.bDiv, true, true); at the beginning of `GridDestroy` and `GridUnload` to reduce the size of memory leaks till all places which are the origin of the leaks will be found.
2. set expendos like `this.refreshIndex`, `this.addJSONData` and so on to `null` inside of `GridDestroy` and `GridUnload` before call of `$("#gbox_"+gid).remove();`.
3. set methods saved in `this.grid` like `this.grid.dragEnd` or `this.grid.populate` in the same way to `null`.

It would be good to change the methods used as expendos so that no reference on external `ts` will be used. Instead of that one can use for example `this` which will be interpreted as **local** variable and don't make any mamory leaks. Alternatively one can use additional parameter initialized to `ts`. For example the current code of `refreshIndex` can be modified to the following

refreshIndex = function () {
var p = this.p, data = p.data, datalen = data.length,
index = p._index, idname, i, val,
ni = p.rownumbers === true ? 1 : 0,
gi = p.multiselect === true ? 1 : 0,
si = p.subGrid === true ? 1 : 0,
getAccessor = $.jgrid.getAccessor;

idname = p.keyIndex === false || p.loadonce === true ?
p.localReader.id :
p.colModel[p.keyIndex + gi + si + ni].name;

for (i = 0; i < datalen; i++) {
val = getAccessor(data[i], idname);
index[val] = i;
}
}

The only disadvantage is that one will have to change all calls of `refreshIndex` to `refreshIndex.call(ts,..,` which can be compatibility problem for users who use the method directly in his current code.
Nevertheless I see not so many alternatives. Probably one should change the sigtanur of the existing methods and describe the step in the documentation.

Best regards
Oleg

Signed-off-by: Dr. Oleg Kiriljuk