[patch] Dialog.js has severe memory leaks

[guest]

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);
    }
  }
}

[guest]

Unified diff of Dialog.js with the leaks all fixed

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

[sdrye at atg.com]

Adding email.

[tk]

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

[dylan]

I need to verify the CLA.

[alex]

CLA verified

[bill]

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

[(none)]

Milestone 0.4.1 deleted