#1606 closed defect (fixed)
[patch] Dialog.js has severe memory leaks
Reported by: | guest | Owned by: | bill |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | Widgets | Version: | 0.3 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
I've found some memory leaks in Dialog.js that are related to events. These cause IE to rapidly run out of memory and go to 100%CPU with garbage collection; Firefox is affected as well. I found them in 0.3.1, but they're still in the head version.
In show(), an event is connected to the background. It is never disconnected in the hide() method. For that matter, shouldn't it just be added once in initialize?
Also, show doesn't check to see if the timer is already active before creating it. It should check, just in case successive calls are made to show().
The worst one is in onTick. In that function an event is added every time the timer goes off. The connect should probably use "once: true", or use a flag like _scrollConnected.
show: function() { if(this.lifetime){ this.timeRemaining = this.lifetime; if(!this.blockDuration){ //<<<< This is never disconnected in hide dojo.event.connect(this.shared.bg, "onclick", this, "hide"); }else{ dojo.event.disconnect(this.shared.bg, "onclick", this, "hide"); } if(this.timerNode){ this.timerNode.innerHTML = Math.ceil(this.timeRemaining/1000); } if(this.blockDuration && this.closeNode){ if(this.lifetime > this.blockDuration){ this.closeNode.style.visibility = "hidden"; }else{ this.closeNode.style.display = "none"; } } //<<<< Should check first this.timer = setInterval(dojo.lang.hitch(this, "_onTick"), 100); } this.showModalDialog(); dojo.widget.Dialog.superclass.show.call(this); } _onTick: function(){ // summary // callback every second that the timer clicks if(this.timer){ this.timeRemaining -= 100; if(this.lifetime - this.timeRemaining >= this.blockDuration){ //<<<< should use "once: true" dojo.event.connect(this.shared.bg, "onclick", this, "hide"); if(this.closeNode){ this.closeNode.style.visibility = "visible"; } } if(!this.timeRemaining){ clearInterval(this.timer); this.hide(); }else if(this.timerNode){ this.timerNode.innerHTML = Math.ceil(this.timeRemaining/1000); } } }
Attachments (2)
Change History (10)
Changed 13 years ago by
comment:1 Changed 13 years ago by
I'm with ATG (Art Technology Group), you should have a CLA on file from us that was sent in a few months ago.
Changed 13 years ago by
Attachment: | dojo_1606.diff added |
---|
renamed to .diff so trac can display it with its formatting.
comment:3 Changed 13 years ago by
Milestone: | → 0.4 |
---|---|
Owner: | changed from bill to dylan |
Status: | new → assigned |
Summary: | Dialog.js has severe memory leaks → [patch] Dialog.js has severe memory leaks |
Version: | 0.4 → 0.3 |
I need to verify the CLA.
comment:4 Changed 13 years ago by
Milestone: | 0.4 → 0.4.1 |
---|
comment:6 Changed 13 years ago by
Owner: | changed from dylan to bill |
---|---|
Status: | assigned → new |
comment:7 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Unified diff of Dialog.js with the leaks all fixed