jQuery-QueryBuilder icon indicating copy to clipboard operation
jQuery-QueryBuilder copied to clipboard

Fix memory leak when calling `.destroy`

Open mreishus opened this issue 7 years ago • 3 comments

Creating a query builder, loading it with data, then destroying it was causing some memory to be retained in the browser.

This should fix.

This fix was created by my colleague w/ @claudior2184 .

Merge request checklist

  • [x] I read the guidelines for contributing
  • [x] I created my branch from dev and I am issuing the PR to dev
  • [x] I didn't pushed the dist directory
  • [x] Unit tests are OK
  • [x] If it's a new feature, I added the necessary unit tests
  • [x] If it's a new language, I filled the __locale and __author fields

mreishus avatar Nov 12 '18 15:11 mreishus

I am not against this change but are you sure it fix anything ? I don't see any reason for these properties not be GC.

mistic100 avatar Nov 12 '18 16:11 mistic100

Use this test case, save as HTML and drag to browser.

<!-- Imports -->
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css" integrity="sha384-1q8mTJOASx8j1Au+a5WDVnPi2lkFfwwEAa8hDDdjZlpLegxhjVME1fgjWPGmkzs7" crossorigin="anonymous">

<script src="https://code.jquery.com/jquery-2.2.3.min.js"></script>

<script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/js/bootstrap.min.js" integrity="sha384-0mSbJDEHialfmuBBQP6A4Qrprq5OVfW37PRR3j5ELqxss1yVqOtnepnHVP9aJ7xS" crossorigin="anonymous"></script>

<script src="http://cdn.jsdelivr.net/npm/jQuery-QueryBuilder/dist/js/query-builder.standalone.min.js">
</script>

<!-- Application -->
<div>
  <button id="1kbtn">Make 1k QB</button>
  
  <div id="builder-basic" />
  
</div>

<script>
function createQB() {
  var rules_basic = {
    condition: 'AND',
    rules: [{
      id: 'price',
      operator: 'less',
      value: 10.25
    }, {
      condition: 'OR',
      rules: [{
        id: 'category',
        operator: 'equal',
        value: 2
      }, {
        id: 'category',
        operator: 'equal',
        value: 1
      }]
    }]
  };
  var filters_basic = [
  {
      id: 'name',
      label: 'Name',
      type: 'string'
    }, {
      id: 'category',
      label: 'Category',
      type: 'integer',
      input: 'select',
      values: {
        1: 'Books',
        2: 'Movies',
        3: 'Music',
        4: 'Tools',
        5: 'Goodies',
        6: 'Clothes'
      },
      operators: ['equal', 'not_equal', 'in', 'not_in', 'is_null', 'is_not_null']
    }, {
      id: 'in_stock',
      label: 'In stock',
      type: 'integer',
      input: 'radio',
      values: {
        1: 'Yes',
        0: 'No'
      },
      operators: ['equal']
    }, {
      id: 'price',
      label: 'Price',
      type: 'double',
      validation: {
        min: 0,
        step: 0.01
      }
    }, {
      id: 'id',
      label: 'Identifier',
      type: 'string',
      placeholder: '____-____-____',
      operators: ['equal', 'not_equal'],
      validation: {
        format: /^.{4}-.{4}-.{4}$/
      }
    }
  ];

  $('#builder-basic').queryBuilder({
    plugins: ['bt-tooltip-errors'],
    filters: filters_basic,
    rules: rules_basic
  });

  $('#btn-reset').on('click', function() {
    $('#builder-basic').queryBuilder('reset');
  });

  $('#btn-set').on('click', function() {
    $('#builder-basic').queryBuilder('setRules', rules_basic);
  });

  $('#btn-get').on('click', function() {
    var result = $('#builder-basic').queryBuilder('getRules');

    if (!$.isEmptyObject(result)) {
      alert(JSON.stringify(result, null, 2));
    }
  });
  
}

function destroyQB() {
  $("#builder-basic").queryBuilder("destroy");
}

function make1kQB() {
  console.log('make1kqb begin');
  for (var i = 0; i <= 1000; i++) {
    createQB();
    //console.log(i);
    destroyQB();
  }
  console.log('make1kqb end');
}
// on document load
$(function(){
    $("#1kbtn").click(function(){
        make1kQB();
    }); 


});

</script>

How to check

chrome_2018-11-12_11-25-54

mreishus avatar Nov 12 '18 17:11 mreishus

I made a rough test by overriding the method https://jsfiddle.net/zk9qt5fj/2/
It stills goes up, but more slowly (~5MB each run instead of ~10MB) there must some others references

mistic100 avatar Nov 23 '18 16:11 mistic100