Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9752 closed defect (fixed)

[patch] [cla] Dojo Memory Leak

Reported by: Mike Wilcox Owned by: James Burke
Priority: high Milestone: 1.4
Component: Core Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description

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.

Attachments (2)

hostenv_browser.patch (633 bytes) - added by Mike Wilcox 10 years ago.
hostenv_browser.2.patch (750 bytes) - added by Mike Wilcox 10 years ago.

Download all attachments as: .zip

Change History (11)

Changed 10 years ago by Mike Wilcox

Attachment: hostenv_browser.patch added

comment:1 Changed 10 years ago by bill

Summary: Dojo Memory Leak[patch] [cla] Dojo Memory Leak

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()?

comment:2 Changed 10 years ago by James Burke

Milestone: tbd1.4

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).

comment:3 Changed 10 years ago by bill

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

comment:4 Changed 10 years ago by 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.

Changed 10 years ago by Mike Wilcox

Attachment: hostenv_browser.2.patch added

comment:5 Changed 10 years ago by 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.

comment:6 Changed 10 years ago by Mike Wilcox

Resolution: fixed
Status: newclosed

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

comment:7 Changed 10 years ago by 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.

comment:8 Changed 10 years ago by liucougar

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

comment:9 Changed 10 years ago by 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

Note: See TracTickets for help on using tickets.