memory leak - Githubissues

johan-ohrn commented 5 years ago

I've stumbled upon what I think is a memory leak issue in jsviews.

(Sorry for not providing a working fiddle but I couldn't get the chrome memory snapshot to display correctly for some reason.)

Save this into a html file:

<html>
<head>
    <script src="https://code.jquery.com/jquery-1.12.4.min.js"></script>
    <script src="https://www.jsviews.com/download/jsviews.js"></script>
    <script>
        $(function () {
            window.MemTest = function () {
                this.name = "memtest";
            }

            window.vm = {
                memTest: null,
                step1: function () {
                    $.observable(vm).setProperty('memTest', new MemTest());
                },
                step2: function () {
                    $.observable(vm).setProperty('memTest', null);
                }
            };

            $.templates("#template").link("#container", vm);
        });
    </script>
</head>
<body>
    <div id="container">aa</div>

    <script id="template" type="text/x-jsrender">
        {^{if memTest}}
            {{:memTest.name}}
        {{/if}}
        <br />
        {^{on step1}}step 1{{/on}}
        {^{on step2}}step 2{{/on}}
    </script>
</body>
</html>

The buttons step 1 and step 2 binds and unbinds a test object to the view model and displays it in the view.

After the test object has been unlinked and is no longer referenced by the viewmodel I expect it to be garbage collected by the browser. It's not however.

After clicking the step 2 button, take a heap snapshot from the memory tab in chrome dev tools. In the class filter type MemTest. Expand the class, click the instance and in the Retainers panel you should see something like this. the bindingStore instance is indirectly holding a reference to the MemTest instance that was unbound in the form of bindingStore[0].linkCtx.eventArgs.oldValue.

If I set a breakpoint in jsviews.js on line 7982 and manually store a reference to the bindingStore such as window.bindingStore = bindingStore then I can manually remove the reference from the oldValue property and a new heap snapshot shows that it's collected or about to be collected (I'm not entierly sure what the tools are telling me).

BorisMoore commented 5 years ago

That's very helpful - thanks for your detective work :) and clear repro...

Here is a possible fix: Add a new line after this one: https://github.com/BorisMoore/jsviews/blob/master/jsviews.js#L3883

    $(target).triggerHandler(propertyChangeStr + ...
    eventArgs.oldValue = null; // Avoid holding on to stale objects

Does that work well for you?

Boris

johan-ohrn commented 5 years ago

As far as I can tell it's working.

I ran into an other related issue. I kept noticing in the chrome memory profiler that there are leaks. After a lot of trial and error I have managed to reproduce it. Again run this html:

<html>
<head>
    <script src="https://code.jquery.com/jquery-1.12.4.min.js"></script>
    <script src="https://www.jsviews.com/download/jsviews.js"></script>
    <script>
        $(function () {
            $.views.tags({
                myTag: {
                    template: "<div>{^{on ~tag.step2}}step 2{{/on}}</div>",
                    init: function () {
                        var that = this;
                        setTimeout(function () {
                            var container = $('<div class="blah"></div>').appendTo('body');

                            //var data = {};
                            var data = that.tagCtx;

                            $.templates("{^{if whatever}} true {{else}} else {{/if}}").link(container, data);
                        });
                    },
                    step2: function () {
                        $('.blah').remove();
                        vm.doToggle();
                    }
                }
            });

            window.vm = {
                toggle: false,
                doToggle: function () {
                    $.observable(vm).setProperty('toggle', !vm.toggle);
                }
            };

            $.templates("#template").link("#container", vm);
        });
    </script>
</head>
<body>
    <div id="container">aa</div>

    <script id="template" type="text/x-jsrender">
        {^{on doToggle}}step 1{{/on}}
        <br />
        {^{if toggle}}
            {{myTag /}}
        {{/if}}
    </script>
</body>
</html>

Try the following with var data = {}; and var data = that.tagCtx;.

take a heap snapshot
click the step 1 button
take a new heap snapshot
click the step 2 button
take a new heap snapshot
In the drop down that says "All objects", change to "objects allocated between snapshot 1 and 2"
In the class filter textbox type "Tag".

Running the code with var data = that.tagCtx; reveals that a Tag instance is kept in memory.

johan-ohrn commented 5 years ago

I just discovered that passing in the current view in the .link() call prevents the memory leak:

var data = that.tagCtx;
$.templates("{^{if whatever}} true {{else}} else {{/if}}").link(container, data, undefined, undefined, that.tagCtx.view);

Hope this info is helpful.

BorisMoore commented 5 years ago

Thanks Johan. Yes, at the moment, when you call tmpl.link(container, data) then JsViews holds a reference to the data, on the top-level view object returned by $.view(). So it is technically a memory leak, in that it prevents disposal of the data object. But if you call tmpl.link(container2, data2) again (even on a different container) without passing a parentView, then it releases the reference to data, and replaces it with a reference to data2. So it will not generally lead to a significant memory leak, in the sense of constantly adding new undisposed objects. But it is still non-ideal.

So I have looked at fixing that design. Here is a proposed update which should no longer hold on to objects inappropriately. Let me know if it works well for you...:

jsviews1.0.4a.zip

BorisMoore commented 5 years ago

I just added new fix for #424. This update contains both fixes:

jsviews1.0.4b.zip

BorisMoore commented 5 years ago

@johan-ohrn: There is a new update for #424 which contains the memory leak fixes.

jsviews1.0.4c.zip

Did you manage to check on the memory leaks issues you were seeing? Let me know how it is. Thanks!

johan-ohrn commented 5 years ago

Sorry I didn't find the time yet. Probably wont be able to test again until next week :(

I'll let you know!

BorisMoore commented 5 years ago

Sure - thanks for letting me know...

johan-ohrn commented 5 years ago

I'm still seeing a lot of references to objects that should have been deleted. I'm having some problems producing a small demo because the code on the page in question is quite complex at this point.

So I'm just throwing out these retaining graphs (bindingStore, cbBindingStore, viewStore) taken from a memory dump in chrome. Does this give you any ideas? Or do you need a demo to see it live in action?

johan-ohrn commented 5 years ago

Here is an observation I've made.

Consider the following code where an event handler is subscribed but never unsubscribed:

class MemTest {};
window.mt = new MemTest();
var handler = function(){debugger};
$.observe(mt, '*', handler);
//$.unobserve(mt, '*', handler);
window.mt = null

This will lead to a leak in the form of cbBindingStore holding a reference to the object similar to the leak I'm experiensing on the page. I haven't been able to find a place in our code where we forget to unsubscribe our handlers but I can't say with 100% certainty that we're not doing just that.

Anyway consider the same using plain jQuery:

class MemTest {};
window.mt = new MemTest();
var handler = function(){debugger};
$(mt).bind('myevent', handler);
// ignore unbind
window.mt = null

This does not lead to a memory leak because jQuery attaches itself to the object and does not store any internal references.

I know it's against good practice to not unsubscribe your handlers but would it be possible for jsviews to not store references internally similar to how jQuery does it?

BorisMoore commented 5 years ago

Does the last proposed update resolve the scenario you described here?

$.bind() is of course deprecated, superseded by on(). Does using on() also avoid internal references? (They have internal stores...) The doc here says For earlier versions, the .bind() method is used for attaching an event handler directly to elements.

At any rate, I think the way JsViews maintains a store is necessary, so that APIs work such as: $.unobserve(): Removing all ‘observe’ handlers from all objects, and $.unlink(): Unregister all views and data-binding. What do you think?

To figure out your current issue without a test case is tricky. Did you try using $.unobserve() and/or $.unlink() to see if either of those calls then frees up your retained objects?

johan-ohrn commented 5 years ago

I will verify this tomorrow at work when I have access to a web server.

Sorry bind was a poor example. However .on() behaves the same way as .bind() does. This code for example does not lead to a memory leak:

class MemTest {};
window.mt = new MemTest();
var handler = function(){debugger};
$(mt).on('myevent', handler);
// skip off
window.mt = null

Anyway now that I think about what you're saying $.unlink(): Unregister all views and data-binding it's clear that this could only be possible if jsviews remembers which those objects are - hence the store. It's only to bad that javascript has no concept of WeakReference.

So I will also try to $.unlink() the root template from the main placeholder element and examine a memory dump after doing so. If I understand correctly it's expected that jsviews should free all it's internal references to any data objects that was previously linked, including nested templates and templates that were dynamically added to the DOM by conditional rendering such as {^{if something}} ... {{/if}}?

I'll let you know what I find.

BorisMoore commented 5 years ago

OK - and yes, removing HTML elements from the DOM using jQuery dom manipulation calls such as empty() or html(...) will trigger disposal and should not retain objects. However direct use of $.observe() to add handlers (which may not be associated with any specific HTML elements) will not be detected during empty() calls etc, and needs to be released using unobserve() or equivalent...

BorisMoore commented 5 years ago

BTW it can be convenient when testing to set:

$.views.settings.advanced({_jsv: true});

Then you can access _jsv.bindings, _jsv.cbBindings, _jsv.views and examine what views or bindings have not been disposed...

Maybe you are already doing that...

johan-ohrn commented 5 years ago

I can confirm that the latest version does indeed fix this.

The tidbit about $.views.settings.advanced({_jsv: true}); was immensely helpful!

So far I've found a couple of instances where we indeed have missed to call $.unobserve().

I've found an other issue in our code that's totally non-obvious and is illustrated by this code:

var person = {name:'someone', address:{street:'some road'}};
var handler = function() {debugger;};
$.observe(person, 'address.street', handler);
person.address = null;
$.unobserve(person, 'address.street', handler);
console.log( _jsv.cbBindings);

This is a dumbed down version of some cleanup code where we null a property that was used in an observe path before actually calling unobserve. This will lead to a memory leak where the address object is kept in _jsv.cbBindings.

Obviously our code is faulty in this regard but considering how hard these leaks are to track down I'm really hoping something can be done on a jsviews level so that consumer code don't have to deal with these details.

Which makes me think about $.unobserve(): Removing all ‘observe’ handlers from all objects: What is all objects really? I assume you mean you pass a root object to $.observe such as $.observe(person, 'address^street', handler). I'm just guessing here but calling $.unobserve(person, 'address^street', handler) look in the cbBinding store for the objects to unobserve. If this is correct could not the affected objects (address in this example) be stored on the person object similar to how jQuery does it with it's special jQuery property?

Sorry I keep returning to this line of thought.

I'll let you know if I find anything else.

BorisMoore commented 5 years ago

You still want $.unobserve() to be able to find that handler binding on the person object (even if from there you can find the address object). So there will be a reference to person held by the binding store, until you call unobserve() orunobserve(person).

johan-ohrn commented 5 years ago

Let me see if I can explain my thinking in some pseudo code. All my reasoning is based on observations so it may of course be flawed, so thanks for your patience.

Given my example:

var handler = function() {
}
var person = {
  name: 'someone',
  address: {
    street: 'some road'
  }
};
$.observe(person, 'address^street', handler)

This results in the following bindings:

_jsv.cbBindings: {
  .obs29: {
    16: {name: "someone", address: {…}, jQuery112405235813798479716: {…}}
    17: {street: "some road", jQuery112405235813798479716: {…}}
  }
}

Calling $.unobserve(person, 'address^street', handler) will remove both 16 and 17. Calling $.unobserve(person.address, 'street', handler) will remove only 17.

My proposal is to move the the internal store to the bound objects like jQuery does.

In pseudo code the observe()/unobserve() method should look something like this:

function observe(person, path, handler) {
  _jsv.cbBindings['.obs29'] = {
    '16': person,
    '17': person.address
  }
  attach event handler to person
  attach event handler to person.address
}

function unobserve(person, path, handler) {
  var binding = locate binding for person from _jsv.cbBindings <-- this gives _jsv.cbBindings['.obs29']
  for each obj in binding {
    detatch event handler from obj
    remove obj from binding
  }
  remove binding from _jsv.cbBindings
}

I'm suggesting the following approach:

function observe(person, path, handler) {
  // create a binding store on person
  person.cbBindings = {
    '16': person,
    '17': person.address
  }
  // create a binding store on person.address
  person.address.cbBindings = {
    '17': person.address
  }
  attach event handler to person
  attach event handler to person.address
}

function unobserve(person, path, handler) {
  var binding = person.cbBindings
  detach event handler from person

  // recursively call unobserve for children, in this case address
  unobserve(person.cbBindings['17'], ...)
}

Basically instead of keeping the cbBindings store internal inside jsviews move it to the bound objects so that each object has knowledge of it's own bindings and the bindings of any of it's decendants.

The fact that person would hold a reference to address in it's binding store could potentially lead to leaks if you would do something like person.address = null without also calling unobserve() to remove person.address from the person objects binding store. But this still seem a lot better because unless person is a singleton object in your application you would eventually throw away the person object at which point the address object would also be free for garbage collection. As it is now person will never be collected because _jsv.cbBindings is a singleton that is not thrown away until you reload the whole page.

I hope I manage to convey the idea.

johan-ohrn commented 5 years ago

I did some more testing to document what was happening in our code:

var handler = function() {
}
var address = {
  street: 'some road'
}
var person = {
  name: 'someone',
  address: address
};
$.observe(person, 'address^street', handler)

person.address = null;

$.unobserve(person, 'address^street', handler)

Notice how I null the address property before calling unobserve(). Looking at _jsv.cbBindings after this shows that it keeps a reference to the address object:

_jsv.cbBindings: {
  .obs29: {
    17: {street: "some road", jQuery112405235813798479716: {…}}
  }
}

I'm guessing that the unobserve() method is traversing the person object hierarchy given the path and since address is now null it doesn't unsubscribe the handler from address. It's however possible to manually unsubscribe it by passing the address object like this $.unobserve(address, 'street', handler) to remove the reference from _jsv.cbBindings.

If observe() would be able to store in _jsv.cbBindings on a per-call-basis all the objects it subscribed (think a dictionary) then unobserve() could just look up the relevant objects from that dictionary and determine which of those matches the given path and unsubscribe them, as opposed to traversing the object hierarchy at the time of calling unobserve() - which may have changed from the time observe() was called.

This might be an easier fix and could perhaps be a solution to the problem?

I still prefer the "store on bound object" approach if it's compatible with the current use cases. I'll see if I can come up with a small demo instead of this rambling.

BorisMoore commented 5 years ago

For the "store on bound object" approach how does it deal with $.unobserve() with no arguments - which should unbind all objects...?

johan-ohrn commented 5 years ago

It would not be possible. You'd always have to pass in an object. But what would be the use case of unbinding all objects? I mean its seem like such a disruptive operation that would break bindings all over a single page app such that reloading the page would most likely be necessary for the page to recover.

BorisMoore commented 5 years ago

It can be useful in a few scenarios. For example it is used on the jsviews.com site, here https://github.com/BorisMoore/jsviews.com/blob/gh-pages/index.js#L614 - whenever navigating from a 'page' which has a sample, to unobserve() the sample code. Basically whenever you want to remove an iframe, replace linked content by other linked content, relink top-level content, etc. (and indeed in these scenarios, it is a good way of ensuring against memory leaks...). See also https://www.jsviews.com/#jsvunlink, scenarios for calling unlink().

Note also you can call unobserve(someHandler), without passing any objects, or you can use namespaces, to selectively unobserve for a subset of the bindings, but without passing an object.

At any rate, I obviously prefer not to make significant breaking changes at this point, so would want to preserve those APIs.

I take your point that nulling an object in a property chain (as opposed to removing it observably) before calling unobserve can lead to memory leaks. That said, the current implementation took some time to stabilize and become robust, so I would tend to avoid significant changes in the underlying implementation at this time. They would need to be very simple, and with a strong motivating scenario, to want to consider before some future v2...

johan-ohrn commented 5 years ago

I see what you mean and it make sense to be strict with breaking changes. Keep it in the back of your head for now. It might happen that you think of something that could improve the situation.

BorisMoore commented 5 years ago

I have an update ready for publishing (1.0.4) that include the changes discussed previously in this thread. However I will be travelling for the next three weeks, and unfortunately did not quite have enough time to complete testing and committing the new version. I expect to do so in the first week of July.

BorisMoore commented 5 years ago

Fixed in v1.0.4

BorisMoore / jsviews

memory leak #423