#425 ✓ resolved
ykphuah

Memory Leak in IE's Event.observe

Reported by ykphuah | November 4th, 2008 @ 02:13 AM | in 1.6.1

Attached test html will show that prototype's Event.observe have memory leak in IE. The memory leak does not happen if the test html is used with prototype 1.5 instead.

The cause of it is that the stopObserving loop is removed during the window onload.

Comments and changes to this ticket

  • ykphuah
  • ykphuah

    ykphuah November 4th, 2008 @ 02:14 AM

    • Title changed from “Memory Leak in IE” to “Memory Leak in IE's Event.observe”
  • Juriy Zaytsev

    Juriy Zaytsev November 4th, 2008 @ 05:44 AM

    • Milestone set to 1.6.1
    • State changed from “new” to “bug”
    • Tag set to patched
    • Assigned user set to “Juriy Zaytsev”

    Thanks.

    Why is stopObserving wrapped in try/catch?

  • ykphuah

    ykphuah November 4th, 2008 @ 05:54 AM

    That's to prevent the it from coughing an error if I use Event.observe(window, ...), as shown in the attached test html. I have tested a couple of methods but none seem to prevent a javascript error from happening in IE, hence I added the try/catch block.

    I have no idea why the try/catch is not needed in prototype 1.5. Maybe that's because there's already a try/catch block in prototype-1.5's stopObserving, which is removed from 1.6's stopObserving as well.

  • John-David Dalton

    John-David Dalton November 4th, 2008 @ 06:11 AM

    Thanks ykphuah, you have been very helpful. Much appreciated :)

  • Juriy Zaytsev

    Juriy Zaytsev November 6th, 2008 @ 07:34 AM

    I believe try/catch is not needed if we call stopObserving from the Event:

    
    function destroyCache() {
      for (var id in cache) {
        for (var eventName in cache[id]) {
          cache[id][eventName].each(function(wrapper) {
            Event.stopObserving(wrapper.element);
          });
          cache[id][eventName] = null;
        }
      }
    }
    
  • Frank DENIS

    Frank DENIS November 17th, 2008 @ 02:20 PM

    Hello Jurly,

    Actually the try...catch seems mandatory.

  • Juriy Zaytsev
  • Vitaliy

    Vitaliy December 3rd, 2008 @ 04:00 PM

    Maybe this way, just to avoid recursion in Event.stopObserving

    
    Event.stopObserving(wrapper.element, eventName, wrapper.handler);
    
  • Juriy Zaytsev

    Juriy Zaytsev February 22nd, 2009 @ 10:48 PM

    • Tag changed from patched to patched, tested

    I would rather use the patch from this thread: http://groups.google.com/group/p...

    Thoughts?

  • Nick Stakenburg

    Nick Stakenburg February 24th, 2009 @ 11:03 PM

    +1 on patch based on the one justin dropped in that thread. I got a seperate ticket #528 running on this leak you can close as duplicate.

  • Juriy Zaytsev

    Juriy Zaytsev February 25th, 2009 @ 02:19 AM

    Nick, actually trunk version of prototype explicitly "collects" observed elements in CACHE variable.

    That CACHE is then enumerated over on "unload" and Event.stopObserving is being calle on each item. This means that part of the patch (from the thread) is already in the trunk. I suppose all that's left to do is to null all of the Event.cache properties.

    Is there anything else? Can someone check if leak still occurs with the current trunk version?

  • Nick Stakenburg

    Nick Stakenburg February 25th, 2009 @ 03:18 AM

    I'd have to get a mac first. Since submodules became required for a rake dist I haven't been able to do anything on windows. msysgit errors out when updating submodules.

    Last time I was able to check I remember there was the CACHE object that wasn't exposed, just one call to stopObserving without parameters and the leak was still there.

    Might be a good idea to check with Microsoft's Leak Detection tool. It showed me elements with .prototypeEventID as leaks and after Element Cache was introduced those became .prototypeUID. mocambo in the google thread also mentions ._prototypeEventID

  • GitHub Robot

    GitHub Robot February 27th, 2009 @ 11:46 PM

    • State changed from “bug” to “resolved”

    (from [d22a9988faefd918e9f2c406d2bada064b0d6b90]) Null out references to elements in cache on page unload. Need this in addition to the Event#stopObserving calls to clean up memory leaks. [#425 state:resolved] (ykphuah, mr_justin, Andrew Dupont) http://github.com/sstephenson/pr...

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

Attachments

Tags

Referenced by

Pages