Skip to content

Commit

Permalink
Merge pull request #5381 from epixa/backport/5185-memory-leaks-binder
Browse files Browse the repository at this point in the history
Backport PR #5185: Fix listener leaks
  • Loading branch information
w33ble committed Nov 12, 2015
2 parents 15ea3a2 + 91bb5ee commit 2be9a91
Show file tree
Hide file tree
Showing 9 changed files with 165 additions and 27 deletions.
24 changes: 9 additions & 15 deletions src/kibana/components/vislib/components/tooltip/tooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ define(function (require) {
return function TooltipFactory(d3, Private) {
var _ = require('lodash');
var $ = require('jquery');
var Binder = require('utils/Binder');
var positionTooltip = require('components/vislib/components/tooltip/_position_tooltip');

var allContents = [];
Expand Down Expand Up @@ -31,6 +32,8 @@ define(function (require) {
this.tooltipClass = 'vis-tooltip';
this.tooltipSizerClass = 'vis-tooltip-sizing-clone';
this.showCondition = _.constant(true);

this.binder = new Binder();
}

/**
Expand Down Expand Up @@ -131,7 +134,7 @@ define(function (require) {

var $chart = self.$getChart();
if ($chart) {
$chart.on('mouseleave', function (event) {
self.binder.jqOn($chart, 'mouseleave', function (event) {
// only clear when we leave the chart, so that
// moving between points doesn't make it reposition
$chart.removeData('previousPlacement');
Expand Down Expand Up @@ -162,7 +165,7 @@ define(function (require) {
}
}

fakeD3Bind(this, 'mousemove', function () {
self.binder.fakeD3Bind(this, 'mousemove', function () {
if (!self.showCondition.call(element, d, i)) {
return render();
}
Expand All @@ -171,26 +174,17 @@ define(function (require) {
return render(self.formatter(events));
});

fakeD3Bind(this, 'mouseleave', function () {
self.binder.fakeD3Bind(this, 'mouseleave', function () {
render();
});

});
};
};

function fakeD3Bind(el, event, handler) {
$(el).on(event, function (e) {
// mimicing https://github.com/mbostock/d3/blob/3abb00113662463e5c19eb87cd33f6d0ddc23bc0/src/selection/on.js#L87-L94
var o = d3.event; // Events can be reentrant (e.g., focus).
d3.event = e;
try {
handler.apply(this, [this.__data__]);
} finally {
d3.event = o;
}
});
}
Tooltip.prototype.destroy = function () {
this.binder.destroy();
};

return Tooltip;
};
Expand Down
12 changes: 11 additions & 1 deletion src/kibana/components/vislib/lib/alerts.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ define(function (require) {
return function AlertsFactory(d3, Private) {
var $ = require('jquery');
var _ = require('lodash');
var Binder = require('utils/Binder');

var ErrorHandler = Private(require('components/vislib/lib/_error_handler'));
/**
Expand All @@ -19,9 +20,10 @@ define(function (require) {

this.vis = vis;
this.data = data;
this.binder = new Binder();
this.alertDefs = alertDefs || [];

$(vis.el).on('mouseenter', '.vis-alerts-tray', function () {
this.binder.jqOn(vis.el, 'mouseenter', '.vis-alerts-tray', function () {
var $tray = $(this);
hide();
$(vis.el).on('mousemove', checkForExit);
Expand Down Expand Up @@ -88,6 +90,14 @@ define(function (require) {
);
};

/**
* Tear down the Alerts
* @return {undefined}
*/
Alerts.prototype.destroy = function () {
this.binder.destroy();
};

return Alerts;
};
});
15 changes: 11 additions & 4 deletions src/kibana/components/vislib/lib/handler/handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ define(function (require) {
return function HandlerBaseClass(d3, Private) {
var _ = require('lodash');
var errors = require('errors');
var Binder = require('utils/Binder');

var Data = Private(require('components/vislib/lib/data'));
var Layout = Private(require('components/vislib/lib/layout/layout'));
Expand Down Expand Up @@ -41,7 +42,7 @@ define(function (require) {
}

this.layout = new Layout(vis.el, vis.data, vis._attr.type, opts);

this.binder = new Binder();
this.renderArray = _.filter([
this.layout,
this.legend,
Expand Down Expand Up @@ -195,13 +196,19 @@ define(function (require) {
* @method destroy
*/
Handler.prototype.destroy = function () {
this.charts.forEach(function (chart) {
this.binder.destroy();

this.renderArray.forEach(function (renderable) {
if (_.isFunction(renderable.destroy)) {
renderable.destroy();
}
});

this.charts.splice(0).forEach(function (chart) {
if (_.isFunction(chart.destroy)) {
chart.destroy();
}
});

this.charts.length = 0;
};

return Handler;
Expand Down
7 changes: 5 additions & 2 deletions src/kibana/components/vislib/vis.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ define(function (require) {
return function VisFactory(d3, Private) {
var _ = require('lodash');

var Binder = require('utils/Binder');

var ResizeChecker = Private(require('components/vislib/lib/resize_checker'));
var Events = Private(require('factories/events'));
var handlerTypes = Private(require('components/vislib/lib/handler/handler_types'));
Expand All @@ -24,13 +26,14 @@ define(function (require) {
}
Vis.Super.apply(this, arguments);
this.el = $el.get ? $el.get(0) : $el;
this.binder = new Binder();
this.ChartClass = chartTypes[config.type];
this._attr = _.defaults({}, config || {}, {});

// bind the resize function so it can be used as an event handler
this.resize = _.bind(this.resize, this);
this.resizeChecker = new ResizeChecker(this.el);
this.resizeChecker.on('resize', this.resize);
this.binder.on(this.resizeChecker, 'resize', this.resize);
}

/**
Expand Down Expand Up @@ -105,7 +108,7 @@ define(function (require) {
Vis.prototype.destroy = function () {
var selection = d3.select(this.el).select('.vis-wrapper');

this.resizeChecker.off('resize', this.resize);
this.binder.destroy();
this.resizeChecker.destroy();
if (this.handler) this._runOnHandler('destroy');

Expand Down
6 changes: 5 additions & 1 deletion src/kibana/components/vislib/visualizations/_chart.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ define(function (require) {
this.handler = handler;
this.chartEl = el;
this.chartData = chartData;
this.tooltips = [];

var events = this.events = new Dispatch(handler);

Expand All @@ -33,6 +34,7 @@ define(function (require) {

// Add tooltip
this.tooltip = new Tooltip('chart', $el, formatter, events);
this.tooltips.push(this.tooltip);
}

this._attr = _.defaults(handler._attr || {}, {});
Expand Down Expand Up @@ -82,7 +84,9 @@ define(function (require) {
Chart.prototype.destroy = function () {
var selection = d3.select(this.chartEl);
this.events.removeAllListeners();
if (this.tooltip) this.tooltip.hide();
this.tooltips.forEach(function (tooltip) {
tooltip.destroy();
});
selection.remove();
selection = null;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ define(function (require) {
return touchdownTmpl(callPlay(d3.event));
}

var endzoneTT = this.endzoneTT = new Tooltip('endzones', this.handler.el, textFormatter, null);
var endzoneTT = new Tooltip('endzones', this.handler.el, textFormatter, null);
this.tooltips.push(endzoneTT);
endzoneTT.order = 0;
endzoneTT.showCondition = function inEndzone() {
return callPlay(d3.event).touchdown;
Expand Down
8 changes: 5 additions & 3 deletions src/kibana/plugins/dashboard/directives/grid.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
define(function (require) {
var _ = require('lodash');
var $ = require('jquery');

var Binder = require('utils/Binder');
require('gridster');

var app = require('modules').get('app/dashboard');
Expand All @@ -17,6 +17,7 @@ define(function (require) {

var $window = $(window);
var $body = $(document.body);
var binder = new Binder($scope);

// appState from controller
var $state = $scope.state;
Expand Down Expand Up @@ -49,9 +50,10 @@ define(function (require) {

// This is necessary to enable text selection within gridster elements
// http://stackoverflow.com/questions/21561027/text-not-selectable-from-editable-div-which-is-draggable
$el.on('mousedown', function () {
binder.jqOn($el, 'mousedown', function () {
gridster.disable().disable_resize();
}).on('mouseup', function () {
});
binder.jqOn($el, 'mouseup', function enableResize() {
gridster.enable().enable_resize();
});

Expand Down
51 changes: 51 additions & 0 deletions src/kibana/utils/Binder.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
define(function (require) {
var $ = require('jquery');
var d3 = require('d3');
var callEach = require('lodash').callEach;
var bindKey = require('lodash').bindKey;
var rest = require('lodash').rest;

function Binder($scope) {
this.disposal = [];
if ($scope) {
$scope.$on('$destroy', bindKey(this, 'destroy'));
}
}

Binder.prototype._bind = function (on, off, emitter, args) {
on.apply(emitter, args);
this.disposal.push(function () {
off.apply(emitter, args);
});
};

Binder.prototype.on = function (emitter/*, ...args */) {
this._bind(emitter.on, emitter.off || emitter.removeListener, emitter, rest(arguments));
};

Binder.prototype.jqOn = function (el/*, ...args */) {
var $el = $(el);
this._bind($el.on, $el.off, $el, rest(arguments));
};

Binder.prototype.fakeD3Bind = function (el, event, handler) {
this.jqOn(el, event, function (e) {
// mimick https://github.com/mbostock/d3/blob/3abb00113662463e5c19eb87cd33f6d0ddc23bc0/src/selection/on.js#L87-L94
var o = d3.event; // Events can be reentrant (e.g., focus).
d3.event = e;
try {
handler.apply(this, [this.__data__]);
} finally {
d3.event = o;
}
});
};

Binder.prototype.destroy = function () {
var destroyers = this.disposal;
this.disposal = [];
callEach(destroyers);
};

return Binder;
});
66 changes: 66 additions & 0 deletions test/unit/specs/utils/Binder.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
define(function (require) {
var $ = require('jquery');
var sinon = require('test_utils/auto_release_sinon');

var Binder = require('utils/Binder');

describe('Binder class', function () {
var $scope;

beforeEach(inject(function ($rootScope) {
$scope = $rootScope.$new();
}));

context('Constructing with a $scope', function () {
it('accepts a $scope and listens for $destroy', function () {
sinon.stub($scope, '$on');
var binder = new Binder($scope);
expect($scope.$on.callCount).to.be(1);
expect($scope.$on.args[0][0]).to.be('$destroy');
});

it('unbinds when the $scope is destroyed', function () {
var binder = new Binder($scope);
sinon.stub(binder, 'destroy');
$scope.$destroy();
expect(binder.destroy.callCount).to.be(1);
});
});

describe('Binder#on', function () {
it('binds to normal event emitters', function () {
var binder = new Binder();
var emitter = {
on: sinon.stub(),
removeListener: sinon.stub()
};
var handler = sinon.stub();

binder.on(emitter, 'click', handler);
expect(emitter.on.callCount).to.be(1);
expect(emitter.on.args[0][0]).to.be('click');
expect(emitter.on.args[0][1]).to.be(handler);

binder.destroy();
expect(emitter.removeListener.callCount).to.be(1);
expect(emitter.removeListener.args[0][0]).to.be('click');
expect(emitter.removeListener.args[0][1]).to.be(handler);
});
});

describe('Binder#jqOn', function () {
it('binds jquery event handlers', function () {
var binder = new Binder();
var el = document.createElement('div');
var handler = sinon.stub();

binder.jqOn(el, 'click', handler);
$(el).click();
expect(handler.callCount).to.be(1);
binder.destroy();
$(el).click();
expect(handler.callCount).to.be(1);
});
});
});
});

0 comments on commit 2be9a91

Please sign in to comment.