Fixing memory leaks with extensions (issue with destructors)

magicxcian

My company's web application has multiple tab-sets that constantly destroy and initialize big tables, on the order of MBs. I noticed that I was running into memory leaks with DataTables, even after I explicitly called $(elem).DataTables().destroy(true). They did not occur when I used vanilla DataTables, no extensions; however, any of ColVis (extension), input-pagination (plugin), or ColumnFilter (separate plugin) would introduce memory leaks.

If any of the extensions or plugins fail to remove a single DOM element, or fails to unbind a single listener, the whole DataTable cannot be garbage collected, because of dangling references!

I had to modify jquery.dataTables.js, and dataTables.colVis.js to fix the leaks. In jquery.dataTables.js, find and modify:

            // Fire off the destroy callbacks for plug-ins etc
            _fnCallbackFire( settings, "aoDestroyCallback", "destroy", [settings] );

            // ---------- Begin added code snippet -JC ----------
            function destroyDOM(obj) { // Try to nuke obj and all descendants
                if (obj == null) return;
                $(obj).find("*").addBack().unbind(); // Unbind all jQuery-added listeners from obj and all descendants
                $(obj).empty().remove(); // Remove obj and all descendants
            }

            for (var feature in settings.aanFeatures) { // Extensions/plugins are causing memory leaks, we attempt to destroy their DOM elements manually.
                destroyDOM(settings.aanFeatures[feature][0]);
            }
            // ---------- End added code snippet -JC ----------

            // If not being removed from the document, make all columns visible
            if ( ! remove ) {
                new _Api( settings ).columns().visible( true );
            }
    
            // Blitz all `DT` namespaced events (these are internal events, the
            // lowercase, `dt` events are user subscribed and they are responsible
            // for removing them
            jqWrapper.unbind('.DT').find(':not(tbody *)').unbind('.DT');
            $(window).unbind('.DT-'+settings.sInstance);

            // ---------- Begin added code snippet -JC ----------
            jqWrapper.find('*').addBack().unbind(); // Extensions/plugins aren't removing their events, we attempt to destroy them here.
            // ---------- End added code snippet -JC ----------

I realize I am explicitly going against the developer's wishes with jqWrapper.find('*').addBack().unbind(), so take that with a grain of salt. It worked, it didn't break anything else, and that was enough for me.

In dataTables.colVis.js, find and modify:

        this.dom.catcher = this._fnDomCatcher();
        this.dom.collection = this._fnDomCollection();
        this.dom.background = this._fnDomBackground();

        // ---------- Begin added code snippet -JC ----------
        // Not only does this extension fail to clean up its DOM mess, it doesn't even attach it to the main DataTable wrapper.
        // We attach it here so that our custom destroy() can clean this up.
        $(this.dom.catcher).appendTo(this.dom.wrapper);
        $(this.dom.collection).appendTo(this.dom.wrapper);
        $(this.dom.background).appendTo(this.dom.wrapper);
        // ---------- End added code snippet -JC ----------

A little later in the same file, find and modify:

        $(this.s.dt.oInstance).bind('destroy.dt', function () {
            // ---------- Begin added code snippet -JC ----------
            // The DataTable is being destroyed, try to remove everything
            function destroyDOM(obj) { // Try to nuke obj and all descendants
                if (obj == null) return;
                $(obj).find("*").addBack().unbind(); // Unbind all jQuery-added listeners from obj and all descendants
                $(obj).empty().remove(); // Remove obj and all descendants
            }
            destroyDOM(that.dom.catcher);
            destroyDOM(that.dom.collection);
            destroyDOM(that.dom.background);
            destroyDOM(that.dom.wrapper);
            // This extension creates a global object that references the DataTable, and doesn't clean it up
            // We do the cleanup here (look for elts. of the global object that match the DataTable being destroyed, and splice them out).
            for (var i = ColVis.aInstances.length - 1; i >= 0; i--) {
                if (ColVis.aInstances[i].s.dt.nTable === this) {
                    ColVis.aInstances.splice(i, 1);
                }
            }
            // It also fails to unbind a custom listener; but this line is actually unnecessary since we nuke all listeners in our custom destroy().
            $(this).unbind('column-reorder.dt');
            // ---------- End added code snippet -JC ----------
        } );

Hope this helps someone else. Sorry for not including a test case, it would be too much work for me to create one currently.

Note: there are actually still (small) leaks with ColVis..for some reason it attaches things directly to the document! The file is a mix of jQuery and plain Javascript, it's a bit horrific.

Edit: Changed the last snippet to do more cleanup with ColVis.

reno1979

Thank you for the snippet, the colvis extension has some more issues that are scheduled to be rewritten. (the new init method and the newest jquery don't seem to work as expected )

So I hope this will be me merged in the new version

allan

There shouldn't be any need to modify DataTables - the extensions should unbind their bound events when the destroy event is triggered.

Could you confirm which version of ColVis you are using? I have recently completely rewritten it, and will be releasing the update in the next few weeks.

for some reason it attaches things directly to the document!

I'm not sure I see the issue with that? It makes sense for position:fixed and it makes some other aspects easier. It should remove those items when the COlVis menu is hidden.

The file is a mix of jQuery and plain Javascript, it's a bit horrific.

Harsh but fair :-). It is legacy code. The rewrite uses the new Buttons extension (also unreleased and coming soon) and is much tidier!

Allan

reno1979

The given snippet does not seem complete, I tried to extend to make it work again:
In some places the code still added the components to the body element, causing non responsive elements.

In the file dataTables.colVis.js find the function _fnCollectionHide :

"_fnCollectionHide": function (  )
    {
        var that = this;

        if ( !this.s.hidden && this.dom.collection !== null )
        {
            this.s.hidden = true;

            $(this.dom.collection).animate({"opacity": 0}, that.s.iOverlayFade, function (e) {
                this.style.display = "none";
            } );

            $(this.dom.background).animate({"opacity": 0}, that.s.iOverlayFade, function (e) {
                            // ---------- Begin added code snippet -KMNS ----------
                            $(that.dom.wrapper).find(that.dom.catcher).remove();
                            // Do not remove the background, it has an attached listener
                            var nBackground = that.dom.background;
                            nBackground.style.bottom = null;
                            nBackground.style.right = null;
                            // ---------- End added code snippet -KMNS ----------
                            //document.body.removeChild( that.dom.background );
                            //document.body.removeChild( that.dom.catcher );
                              
            } );
        }
    },

Still in the same file, find the function _fnCollectionShow :
And modify like this

var oStyle = this.dom.catcher.style;
        oStyle.height = $(this.dom.button).outerHeight()+"px";
        oStyle.width = $(this.dom.button).outerWidth()+"px";
        oStyle.top = oPos.top+"px";
        oStyle.left = iDivX+"px";
                
                // --------------- Original lines now commented - KMNS
        //document.body.appendChild( nBackground );
        //document.body.appendChild( nHidden );
        //document.body.appendChild( this.dom.catcher );
                // --------------- End of Original lines 
                // 
                // ---------- Begin added code snippet -KMNS ----------
                $(nHidden).appendTo(this.dom.wrapper);
                $(this.dom.catcher).appendTo(this.dom.wrapper);
                // The background is not removed (has an attached listener), only the style is changed (bottom, right = null) 
                // So adding it again would be wrong
                // ---------- End added code snippet -KMNS ----------

Open the dataTables.colVis.css and find the background styling

div.ColVis_collectionBackground {
    position: fixed;
    top: 0;
    left: 0;
        // ---------- Begin added code snippet -KMNS (original lines commented)----------
    //height: 100%;
    //width: 100%;
        // ---------- End added code snippet -KMNS ----------
    background-color: black;
    z-index: 1100;
}

reno1979

The ColVis version is 1.1.2
Jquery 2.1.4
DataTables 1.10.7

With my own additional snippets I had an extra suprise, the buttons appended to the collection changed to the "[object HTMLLIElement]" string being displayed after a destroy and rebuild.

I solved it changing the following line:
In file dataTables.colVis.js function _fnAddButtons last row:

 // ---------- Begin added code snippet -KMNS (original lines commented)----------
 $(this.dom.buttons).appendTo(this.dom.collection,this.dom.wrapper);
//$(this.dom.collection,{}).append( this.dom.buttons );
 // ---------- End added code snippet -KMNS ----------

Thank you for updating the extension Allan :)