Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

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

diff.txt (1.3 KB) - added by guest 13 years ago.
Unified diff of Dialog.js with the leaks all fixed
dojo_1606.diff (1.3 KB) - added by tk 13 years ago.
renamed to .diff so trac can display it with its formatting.

Download all attachments as: .zip

Change History (10)

Changed 13 years ago by guest

Attachment: diff.txt added

Unified diff of Dialog.js with the leaks all fixed

comment:1 Changed 13 years ago by guest

I'm with ATG (Art Technology Group), you should have a CLA on file from us that was sent in a few months ago.

comment:2 Changed 13 years ago by sdrye at atg.com

Adding email.

Changed 13 years ago by tk

Attachment: dojo_1606.diff added

renamed to .diff so trac can display it with its formatting.

comment:3 Changed 13 years ago by dylan

Milestone: 0.4
Owner: changed from bill to dylan
Status: newassigned
Summary: Dialog.js has severe memory leaks[patch] Dialog.js has severe memory leaks
Version: 0.40.3

I need to verify the CLA.

comment:4 Changed 13 years ago by dylan

Milestone: 0.40.4.1

comment:5 Changed 13 years ago by alex

CLA verified

comment:6 Changed 13 years ago by bill

Owner: changed from dylan to bill
Status: assignednew

comment:7 Changed 13 years ago by bill

Resolution: fixed
Status: newclosed

(In [6432]) Fixes #1606. However, there's a (pre-existing) bug that clicking the background won't close the dialog on IE. Will file trac report.

comment:8 Changed 12 years ago by (none)

Milestone: 0.4.1

Milestone 0.4.1 deleted

Note: See TracTickets for help on using tickets.