Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #11371 (closed defect: fixed)

Opened 4 months ago

Last modified 3 months ago

[PATCH] Element#update should purge event handlers from the child elements

Reported by: ddegioanni Assigned to: kangax
Priority: normal Milestone: 2.x
Component: Prototype Version: edge
Severity: normal Keywords: event ajax memory leak ie6
Cc:

Description

Hello,

When an HTML element observed with the class Event, is destroyed by the Ajax.Updater, it causes a memory leak on this element at page unload (only with IE6).

Example:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
  <head>
    <meta http-equiv="content-type" content="text/html; charset=windows-1250">
    <title>Memory leak test</title>
    <script type="text/javascript" src="prototype-1.6.0.2.js"></script>
  </head>
  <body>
    <div id='ajaxdiv'>
      <div id='observedDiv'>
        I am observed!
      </div>
    </div>
    <div id='launchAjax'>
      Click here to Ajax Update!
    </div>
  </body>
  <script type="text/javascript">
    $('observedDiv').observe('click', function(){
      alert('Clicked');
    });
    $('launchAjax').observe('click', function(){
      new Ajax.Updater($('ajaxdiv'), 'file.html');
    });
  </script>
</html>

I fixed this issue by my side by overriding the function Event.observe(), which now builds a list of observed elements/events/handlers, then at each Ajax.Updater() call, I call Event.stopObserving() on each observed element that are children of the ajax container.

I am not a javascript expert but here is my solution if it can help (this solution is not so good because released elements are not removed from the list, I am sure that it could be done easily)

var EventCollector = {
  observedEvents: [],
  install: function(){
    Event.oldObserve = Event.observe;
    Event.observe = function(iElement, iEventName, iHandler) {
      EventCollector.observedEvents.push(
        {element:iElement, name:iEventName, handler:iHandler}
      );
      Event.oldObserve(iElement, iEventName, iHandler, arguments[3]);
    };
  },

  cleanEvents: function(parent){
    EventCollector.observedEvents.each(function(event){
      if(Element.descendantOf(event.element, parent)){
        Event.stopObserving(event.element, event.name, event.handler);
      }
    });
  }
};

EventCollector.install();

EventCollector.cleanEvents() is called before each new Ajax.Updater(...).

Attachments

index.html (0.7 kB) - added by ddegioanni on 03/18/08 16:23:00.
file.html (7 bytes) - added by ddegioanni on 03/18/08 16:23:42.
0005-Fix-memory-leaks-in-update-replace-in-IE.-Closes-11.patch (0.9 kB) - added by kangax on 03/31/08 19:37:43.
update_remove_replace.patch (1.8 kB) - added by jdalton on 04/01/08 02:17:40.
Automatically remove observers from Element#update. Gives option to remove observers for Element#replace, Element#remove

Change History

03/18/08 16:23:00 changed by ddegioanni

  • attachment index.html added.

03/18/08 16:23:42 changed by ddegioanni

  • attachment file.html added.

03/31/08 17:52:34 changed by jdalton

Thank you for the tests and the very clear example.
The patches provided here may help you: http://dev.rubyonrails.org/ticket/9466

Please report back if they worked or not :)

03/31/08 19:37:00 changed by kangax

  • owner changed from sam to kangax.
  • summary changed from Event in ajax leaks with IE6 to [PATCH] Element#update should purge event handlers from the child elements.

03/31/08 19:37:43 changed by kangax

  • attachment 0005-Fix-memory-leaks-in-update-replace-in-IE.-Closes-11.patch added.

(follow-up: ↓ 4 ) 03/31/08 19:50:55 changed by jdalton

Fixing the Event system to work correctly would avoid these extraneous patches.
This is why I placed a reference to ticket #9466.

My patch:
http://dev.rubyonrails.org/attachment/ticket/9466/event_ie_leak_fix.diff

Corrects the Event systems issue of not removing element observers on window unload.
Assuming my patch works correctly, there is no need for additional patches.

(in reply to: ↑ 3 ) 03/31/08 20:00:24 changed by kangax

Replying to jdalton:

Fixing the Event system to work correctly would avoid these extraneous patches.
This is why I placed a reference to ticket #9466. My patch:
http://dev.rubyonrails.org/attachment/ticket/9466/event_ie_leak_fix.diff Corrects the Event systems issue of not removing element observers on window unload.
Assuming my patch works correctly, there is no need for additional patches.

: )

Cleaning things up on unload is good but might not be enough in this case. Ajax.PeriodicalUpdater might make IE leak, due to a frequently updated content (and not detaching handlers of that content). I'll try to make a test, to see if that's issue.

03/31/08 20:03:49 changed by jdalton

Any page will build up memeory usage until a refresh, that's when a leak will present itself because the memory will not be returned.

Tests are always welcome :P
Remeber it primarily effects IE6 and below because IE7 had some progress in removing memory leaks.

03/31/08 20:06:34 changed by jdalton

I can see where waiting till the page unload might delay things where as smart removal (when it's needed) can help lighten the onunload processing.

(follow-up: ↓ 8 ) 03/31/08 20:40:41 changed by jdalton

Maybe a second param for Element#remove method is needed that will remove the element as well as stop its observers().

Element.Methods.remove = Element.Methods.remove.wrap(function(proceed, element, stopObserving){
  element = proceed(element);
  return stopObserving? element.stopObserving() : element;
});

(in reply to: ↑ 7 ) 03/31/08 20:53:12 changed by kangax

Replying to jdalton:

Maybe a second param for Element#remove method is needed that will remove the element as well as stop its observers(). {{{ Element.Methods.remove = Element.Methods.remove.wrap(function(proceed, element, stopObserving){ element = proceed(element); return stopObserving? element.stopObserving() : element; }); }}}

Sounds good to me - more flexible, finer control. update/remove/replace should still purge in IE by default.

03/31/08 20:55:33 changed by jdalton

not remove() because you can still retain a reference to the Element.

04/01/08 02:17:40 changed by jdalton

  • attachment update_remove_replace.patch added.

Automatically remove observers from Element#update. Gives option to remove observers for Element#replace, Element#remove

04/27/08 19:35:57 changed by jdalton

  • status changed from new to closed.
  • resolution set to fixed.