#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)
Change History (11)
Changed 12 years ago by
Attachment: | hostenv_browser.patch added |
---|
comment:1 Changed 12 years ago by
Summary: | Dojo Memory Leak → [patch] [cla] Dojo Memory Leak |
---|
comment:2 Changed 12 years ago by
Milestone: | tbd → 1.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 12 years ago by
OK, but won't this change call the old handler twice? After all, the method is called "addEventListener" not "replaceEventListener".
comment:4 Changed 12 years ago by
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 12 years ago by
Attachment: | hostenv_browser.2.patch added |
---|
comment:5 Changed 12 years ago by
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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:7 Changed 12 years ago by
comment:9 Changed 12 years ago by
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:
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()?