Skip to content

Commit

Permalink
Fix memory leaks in data table / jqplot (#11354)
Browse files Browse the repository at this point in the history
* Fixes memory leaks in dataTable and jqplot via uncleared resize handlers

Previously, these plugins registered a handler on the `resize` event on window,
but used ad-hoc heuristics to determine when to remove the handler:

* dataTable would only remove the handler the next time the `resize` event fires,
  which is not guaranteed to happen regularly (especially if the user is not
  regularly resizing their browser window, e.g., it is full screened)
  * Fix: I define `_destroy` on `dataTable`, which appropriately cleans up the
    handler (and calls the `_destroy` method of its parent class, `UIControl`)
* jqplot contained code to remove the `resize` listener when `destroyPlot()`
  is called, but a) this function is not called when the plot is removed from
  the DOM (the code called `destroy()` on the `plot` object directly), and
  b) it incorrectly uses `this` instead of `self`, preventing it from working
  in the first place.
  * Fix: I fixed the `this`/`self` confusion, and changed the cleanup code
    such that `$.jqplot.visiblePlots` contains `JqplotGraphDataTable` objects
    rather than `jqplot` objects.

These event handlers prevented previous jqplot and datatable objects from being
garbage collected each time the dashboard is refreshed.

* Prevent leaking data tables via body mouseup handlers.

Adds code to clean up mouseup handlers on the body HTML element when data tables are destroyed.

Previously, these handlers caused data tables to remain in memory forever.
  • Loading branch information
John Vilk authored and sgiehl committed Mar 27, 2017
1 parent 0f887dc commit dc37bf7
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 8 deletions.
17 changes: 15 additions & 2 deletions plugins/CoreHome/javascripts/dataTable.js
Expand Up @@ -89,6 +89,17 @@ $.extend(DataTable.prototype, UIControl.prototype, {
// initialize your dataTable in your plugin
},

_destroy: function() {
UIControl.prototype._destroy.call(this);
// remove handlers to avoid memory leaks
if (this.windowResizeTableAttached) {
$(window).off('resize', this._resizeDataTable);
}
if (this._bodyMouseUp) {
$('body').off('mouseup', this._bodyMouseUp);
}
},

//initialisation function
init: function () {
var domElem = this.$element;
Expand Down Expand Up @@ -577,6 +588,7 @@ $.extend(DataTable.prototype, UIControl.prototype, {
}

$(window).on('resize', resizeDataTable);
self._resizeDataTable = resizeDataTable;
}
},

Expand Down Expand Up @@ -1179,11 +1191,12 @@ $.extend(DataTable.prototype, UIControl.prototype, {
});

//close exportToFormat onClickOutside
$('body').on('mouseup', function (e) {
self._bodyMouseUp = function (e) {
if (self.exportToFormat) {
self.exportToFormatHide(domElem);
}
});
};
$('body').on('mouseup', self._bodyMouseUp);

$('.exportToFormatItems a', domElem)
// prevent click jacking attacks by dynamically adding the token auth when the link is clicked
Expand Down
12 changes: 6 additions & 6 deletions plugins/CoreVisualizations/javascripts/jqplot.js
Expand Up @@ -178,12 +178,12 @@

// manage resources
target.on('piwikDestroyPlot', function () {
if (this._resizeListener) {
$(window).off('resize', this._resizeListener);
if (self._resizeListener) {
$(window).off('resize', self._resizeListener);
}
self._plot.destroy();
for (var i = 0; i < $.jqplot.visiblePlots.length; i++) {
if ($.jqplot.visiblePlots[i] == self._plot) {
if ($.jqplot.visiblePlots[i] === self) {
$.jqplot.visiblePlots[i] = null;
}
}
Expand Down Expand Up @@ -394,14 +394,14 @@
if ($.jqplot.visiblePlots[i] == null) {
continue;
}
$.jqplot.visiblePlots[i].destroy();
$.jqplot.visiblePlots[i].destroyPlot();
}
$.jqplot.visiblePlots = [];
});
}

if (typeof plot != 'undefined') {
$.jqplot.visiblePlots.push(plot);
$.jqplot.visiblePlots.push(self);
}
},

Expand Down Expand Up @@ -1196,4 +1196,4 @@ RowEvolutionSeriesToggle.prototype.beforeReplot = function () {
$.jqplot.preInitHooks.push($.jqplot.PieLegend.init);
$.jqplot.postDrawHooks.push($.jqplot.PieLegend.postDraw);

})(jQuery, require);
})(jQuery, require);

0 comments on commit dc37bf7

Please sign in to comment.