New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leaks in data table / jqplot #11354

Merged
merged 2 commits into from Mar 27, 2017

Conversation

Projects
None yet
3 participants
@jvilk
Contributor

jvilk commented Feb 19, 2017

Overview:

Prior to these changes, every data table created would remain in memory due to leftover resize and mouseup handlers on window/document.body (see individual commit messages for further details).

This PR adds a _destroy method to the data table class to appropriately clean up these handlers, and changes jqplot so that it appropriately destroys plot objects when the page changes.

Prerequisites to reproducing bug: Have a dashboard configured to show one of these data tables (e.g., the demo works fine for reproducing this bug).

Reproduce:

  1. Visit the Dashboard.
  2. Take a heap snapshot using Chrome's development tools.
  3. Click on the dashboard link in the left menu.
  4. Take another heap snapshot.
  5. Compare the number of live instances of exports.JqplotEvolutionGraphDataTable between snapshot 1 and 2. 2 will have double the instances of 1.

John Vilk added some commits Feb 18, 2017

John Vilk
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.
John Vilk
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.

@mattab mattab modified the milestone: 3.0.3 Feb 21, 2017

@sgiehl sgiehl merged commit dc37bf7 into matomo-org:3.x-dev Mar 27, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment