memory leak with adding/removing dalliances

[Traksewt]

This is similar to #149 memory leak adding/removing tracks, however now it is for adding and removing dalliance browsers. This is the case when Dalliances need to be dynamically created and destroyed.

It appears cbrowser.availableSources is registering as a listener in track-adder.js makeStab, and there isn't currently a neat way to remove this.

http://jsfiddle.net/yeb68220/

Small amount of memory in this case, but larger when custom listeners are referencing to large objects (or referring back to the browser itself). We could add remove methods for all custom listeners (e.g. featureListeners), though removing by passing in the function source is a little cumbersome (though possible). In this case, it would be ideal (for me) for the browser to have a destroy method that will remove all listeners.

[dasmoth]

The primary cause of this was a window.addEventListener('resize', ....). Since the window object (obviously) doesn't go away when you remove Biodalliance from the DOM, this was causing the whole lot to get retained.

I've added a "destroy" method on Browser which currently just removes this listener, and seems to solve the problem.

Would it make sense if "destroy" also handled the removal from the DOM?

[Traksewt]

I'm also worried about the custom listeners as there is currently no way to remove them.

Checkout this contrived memory leak for a feature listener. http://jsfiddle.net/yeb68220/1/

Also, how about window load event? that also has a ref to thisB. I'll put some additions to your destroy and you see if it is OK.

Since you create the browser based on the div id provided, it could be neat if the code handled deleting itself as well. I'm toying with the idea of leaving unused dalliances (up to 5) hidden instead of deleted and lazily create them when needed, which I implemented as a workaround to the mem leaks. However, if I go down this route, I may need a helper method to clean the cache.

[Traksewt]

By removing all the tiers, the memory could be cleaned up. I submitted a helper method to clear all tiers. This way I didn't have to manage removing the other custom listeners.

[dasmoth]

Yes, the window load event should be cleaned up (probably immediately after it fires, rather than at destroy-time). Although I doubt this will be a major problem when dynamically creating/destroying Biodalliance instances, since the listener only gets added in the first place if you create a Browser before the onload event has been fired.

The featureListener case has me puzzled, actually -- I can't immediately see why this is causing a leak (JS implementations aren't supposed to be fooled by cycles of references). As you say, a remove method makes sense anyway (and I think it's probably worth systematically adding a removeListener method for every addListener -- I'm hoping to do this before 0.13.x).

Is there a particular reason you prefer "clear all listeners" methods to the more JS-standard removeListener pattern?

[Traksewt]

Yes removelistener is good. I was just being lazy cause there were a few listeners to handle and its a pain keeping the function around, though that is the standard in js. Possibly both? As if you call destroy with listeners it can be mem leak, though you can leave it upto the dev to cleanup. I am fine if you just add removelistener. Thanks. On 16/04/2015 5:12 PM, "Thomas Down" notifications@github.com wrote:

Yes, the window load event should be cleaned up (probably immediately after it fires, rather than at destroy-time). Although I doubt this will be a major problem when dynamically creating/destroying Biodalliance instances, since the listener only gets added in the first place if you create a Browser before the onload event has been fired.

The featureListener case has me puzzled, actually -- I can't immediately see why this is causing a leak (JS implementations aren't supposed to be fooled by cycles of references). As you say, a remove method makes sense anyway (and I think it's probably worth systematically adding a removeListener method for every addListener -- I'm hoping to do this before 0.13.x).

Is there a particular reason you prefer "clear all listeners" methods to the more JS-standard removeListener pattern?

— Reply to this email directly or view it on GitHub https://github.com/dasmoth/dalliance/issues/150#issuecomment-93662233.

[dasmoth]

Okay, the reason your latest Fiddle is leaking is that you don't actually call Browser.destroy(). Add browser.destroy() immediately after $('#browser1').remove(); and it seems to work fine for me.

I'm pretty confident that leaving attached listeners is fine -- you'll end up with circular references from your event-listener closures, but JS does specify "real" GC, not reference counting -- so this should be fine.

It would be kind-of nice to be able to clean up without requiring an explicit call to destroy, but the only way I can see to do that is via mutationobservers to detect when our container gets removed -- and that seems like overkill for handling a relatively rare event.