[patch] [cla] Dojo Memory Leak

[Mike Wilcox]

The new windows Leak Detector uncovered a leak in Dojo in hostenv_browser.js in the _handleNodeEvent function. Surprisingly, we are not using safe methods for attaching. I've included a patch that does. It unfortunately uses a few more bytes, but those could be made up for with shorter variable names.

[bill]

Hmm, I'm not sure that patch will handle canceling of events correctly. addEventListener()/attachEvent() are similar to dojo.connect() already so it doesn't seem like you need to call the old event handler from the new one (won't that call the old handler twice?), but I must be missing something.

I'm also assuming that the memory leak only occurs when there is no dojo.disconnect()?

[James Burke]

To Bill's questions, this function is used before there is a dojo.connect/dojo.disconnect for the dojo.addOnUnload and dojo.addOnWindowUnload functions, and both of those options do not allow for event cancelation. It is actually a weakness with those functions (well at least the addOnUnload that binds to onbeforeunload). For a Dojo 2.0 I would just remove them -- better for code to dojo.connect to onbeforeunload and onunload directly.

As for the patch, I thought it was hazardous to call attachEvent with three arguments. Or maybe I am thinking of addEventListener with just two arguments.

In any case, if this change works in IE6, 7 and 8 (as well as Safari and FF)? If so, feel free to apply the change (I have to rebuild my IE6 image so it could take a while before I could test directly).

[bill]

OK, but won't this change call the old handler twice? After all, the method is called "addEventListener" not "replaceEventListener".

[James Burke]

Oh right, if addEventListener/attachEvent are used, then we can get by without the oldHandler stuff. That stuff should be removed in the patch.

[Mike Wilcox]

Good call on the oldhandler code. That shrinks it back down to what it was. Also based on what you two said I added an addOnLoad and 2 addOnUnloads to test everything was being called. Tested on IE6,7,8, Safari 3,4 and FF 3.5. BTW, my bet was that the third arg in IE would be discarded as extra and that seems to be the case.

I added the updated patch and will now commit the changes.

[Mike Wilcox]

(In [19946]) Fixes #9752 applying memory leak patch with changes as per James

[Mike Wilcox]

(In [19947]) Refs #9752 applying memory leak patch with changes as per James - sending again, I think I forgot to save before the last commit.

[liucougar]

may I know what is "The new windows Leak Detector"?

[Mike Wilcox]

It was given to me by dmachi so I didn't have the url offhand. It's called IE JavaScript? Leaks Detector. I think this is the link:

​http://blogs.msdn.com/askie/archive/2008/12/31/javascript-memory-leak-detector-for-internet-explorer.aspx