#789 bug
jozef

observe/stopObserving cause memory leaks in IE

Reported by jozef | September 8th, 2009 @ 09:46 PM

When calling observe method the element is stored in CACHE closure local variable. This CACHE seems to be used to clean event handlers in IE on unload. References from CACHE variable prevent already removed DOM Elements to be cleaned by GC. This is especially problem when having page that does not reload but manipulate DOM extensively. This can be easily seen by using sIEve or similar tools. Biggest problem is that there is no way how to remove the element from CACHE local variable.

As a good place to fix I see the stopObserving method.

Current implementation 1.6.1 is


  function stopObserving(element, eventName, handler) {
    element = $(element);

    var registry = Element.retrieve(element, 'prototype_event_registry');

    .....

    registry.set(eventName, responders.without(responder));

    return element;
  }

and I would change it to


  function stopObserving(element, eventName, handler) {
    element = $(element);

    var registry = Element.retrieve(element, 'prototype_event_registry');

    .....

    if (responders.length == 1)
    {
      if (registry.size() == 1)
      {
        Element.getStorage(element).unset('prototype_event_registry');
        CACHE = CACHE.without(element);
      }
    }
    else
      registry.set(eventName, responders.without(responder));

    return element;
  }

Comments and changes to this ticket

  • Tobie Langel

    Tobie Langel September 8th, 2009 @ 11:13 PM

    I haven't looked at this issue in particular, but for the record, Sieve, like Drip are very untrustable tools and have a long history of finding false positives.

    Have you noticed memory leaks using process explorer?

  • jozef

    jozef September 8th, 2009 @ 11:35 PM

    I did not do any exact measurements but IE usually grows from 70MB to few 100s of MB. On sIEve I was not watching memory leaks but number of DOM Elements and it kept growing. I did few experiments:

    1. record number of dom nodes
    2. update empty element by content and add event handlers (Element.observe) to it
    3. clean event handlers (Element.stopObserving) and set content to ''
    4. record number of dom nodes

    If everything was cleaned the number of nodes would be the same in point 1. and 4. However the difference was exactly the number of observed elements. I know that those will be cleaned at page unload but as I mentioned in this case there are almost no reloads of the page.

  • jozef

    jozef September 8th, 2009 @ 11:46 PM

    @@@javascript CACHE = CACHE.without(element);

    
    should be replaced by 


    @@@javascript CACHE.splice(CACHE.indexOf(element),1);

    for performance reasons

  • Samuel Lebeau
  • jozef

    jozef September 17th, 2009 @ 03:48 PM

    Found errors after myself so here are the updated changes. Added closure local variable destroyingCACHE to closure set to false by default and set to true in _destroyCache function. Rethought/fixed bugs in the stopObserving function created by my changes.

    
      var destroyingCACHE = false;
      
      function _destroyCache() {
        destroyingCACHE = true;
        
        ....
    
      }
    
      ......
    
      function stopObserving(element, eventName, handler) {
        element = $(element);
    
        var registry = Element.retrieve(element, 'prototype_event_registry');
    
        .......
    
        if (!destroyingCACHE && responders.length == 1 && registry.size() == 1)
        {
          Element.getStorage(element).unset('prototype_event_registry');
          CACHE.splice(CACHE.indexOf(element),1);
        }
        else
        {
          if (responders.length == 1) 
            registry.unset(eventName);
          else 
            registry.set(eventName, responders.without(responder));
        }  
    
        return element;
      }
    
  • T.J. Crowder

    T.J. Crowder November 16th, 2009 @ 04:50 PM

    [responsible:none bulk edit command]

  • Tobie Langel

    Tobie Langel March 1st, 2010 @ 12:49 AM

    • State changed from “new” to “bug”
    • Tag set to section:dom
  • ronin-93814 (at lighthouseapp)

    ronin-93814 (at lighthouseapp) April 13th, 2011 @ 11:35 AM

    • Importance changed from “” to “”

    this relates to #466 and #1025

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

The Prototype JavaScript library.

Shared Ticket Bins

Pages