[PATCH] Event memory leaks on IE6

FYI Drip isn't a trustful tool, it gives a lot of false positives.

could you provide us a code sample which leaks memory so we can test it? That would be very much appreciated.

this example gives me 1.6.0_rc0: +1M memory usage after each request, 1.5.1.1: ~constant memory usage

Leak example

this patch works for me, ie6 performs much much better

as i understand, we cant get attached elements with the new Event API for Event.cache so in my patch i checks every DOM element for _eventID and stops observing them all.

the result is somethnig like in 1.5.1.1 when Event.unloadCache did Event.stopObserve on all observers before nullifying.

I have a lot of Event.observe in my web system, and this has been killing my IE6 users. I posted to the prototype group http://groups.google.com/group/prototype-core/browse_frm/thread/c2456d26913c7178 but got no answers. I think this is more than a normal priority bug, since it has been holding me from update to version 1.6... and probably causing a hazard in a lot of other web pages.

aggressive bug example

Replying to na43251

Your patch is a bit to brute force for me, it iterates over a whole document just to remove possibly only a few observers.

The patch I have attached only removes the elements that are observing. I have also optimized the wrapper lookup so it spends less time looking up a wrapper if it already has it.

It passes the unit tests as well :)

Also it reduces the complexity of the getEventID() method by removing a condition for the arguments.callee and has the counter start at 1 instead of 2.

I rolled in the 1 line patch from this ticket as well: http://dev.rubyonrails.org/ticket/11453

Yep, the first attached file demonstrates leak quite aggressively and shows ~2M/sec increase in IE6. IE7 seems to be unaffected. Simple removal of event handlers from observed elements solves the issue:

destroyCache: function() {

  $A(document.getElementsByTagName('*')).each(function(element) {
    if (!element._prototypeEventID) return;
    Event.stopObserving(element);
  })
  
  ...

}

I think "brute-force" approach is more than enough in this case. unloadCache is executed once on unload. It doesn't seem to be a critical part of the program. I'm not sure if creating an instance of Wrapper for each event handler is a better approach. Moreover, keeping reference to element in a wrapper could introduce circular references between this.handler and this.element

Replying to kangax:

Do removed elements that at one time where in the dom and had observers contribute to the memory leak?

If "yes", then the "brute-force" method will miss those.

I don't by the circular reference argument. Prototype 1.5+ stored references to the element in its implementation and it didn’t suffer from memory leaks.

I don't buy the Wrapper instance argument either. I did some benchmarking using console.time()/1000+ iterations and the speeds where equiv.

The initialize function of a Wrapper "class" defines another function - the one stored in a handler property. This handler function has this.element in it's closure - therefore circular reference is created. I'm not an expert in such things but it seems that proposed pattern could lead to even more problems : )

crap :D
updating patch to take care of that.
Event Prototype 1.5 set the element to null.

Replying to jdalton:

crap :D
updating patch to take care of that.
Event Prototype 1.5 set the element to null.

told ya : )

related (leaks when element, that had an event observer, is destroyed): http://dev.rubyonrails.org/ticket/11371

Only removes elements that are observing, optimize wrapper lookup.

Fixed:
http://github.com/jdalton/prototype/commit/6ff8485e40b8633928bcfda0adc08469b7ea3c16