43939 - new gtk timer code leakes several TimeVal objects per second

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

There's code in the code that pav checked in that leaks TimeVal objects:

 61 TimeVal TimeVal::operator+(PRUint32 msec) const
 62 {
 63   TimeVal *t = new TimeVal(*this);
 64 
 65   t->mSeconds += (PRUint32)(msec / 1000);
 66   t->mUSeconds += (msec % 1000) * 1000;
 67 
 68   if (t->mUSeconds >= 1000000) {
 69     t->mUSeconds -= 1000000;
 70     t->mSeconds++;
 71   }
 72  
 73   return *t;
 74 }

I'm pretty sure this invokes the implicit copy constructor.  Adding an explicit
constructor for the copy and adding a counter reveals that we are leaking
objects all over the place.  Leave Mozilla up for about 20 seconds without
about:blank loaded and we leak about 10,000 objects.

The worst thing about this is in my code review for this code I said to pavlov:

[snip]
> +TimeVal TimeVal::operator+(PRUint32 msec) const
> +{
> +  TimeVal *t = new TimeVal(*this);
> +
> +  PRUint32 tmp = msec;
> +
> +  while (tmp >= 1000)
> +  {
> +    t->mSeconds++;
> +    tmp -= 1000;
> +  }
> +
> +  t->mUSeconds += (tmp*1000);
> +
> +  if (t->mUSeconds >= 1000000) {
> +    t->mSeconds++;
> +    t->mUSeconds -= 1000000;
> +  }
> +
> +  return *t;
> +}

I think there is an extra object created here.
[snip]

Pav, didn't you check with scc before checking this in like you said you would?

Comment 1

[Scott Collins]

Arrgghhh

Normally, one would write this like this

    // copy-constructor
  TimeVal::TimeVal( const TimeVal& tv )
    {
      // ...
    }

    // member |operator+=|
  TimeVal&
  TimeVal::operator+=( PRInt32 msec )
    {
      // ...
      return *this;
    }


    // (two) non-member |operator+| combines the two
  TimeVal
  operator+( const TimeVal& lhs, PRInt32 rhs )
    {
      return TimeVal(lhs)+=rhs;
    }

  TimeVal
  operator+( PRInt32 lhs, const TimeVal& rhs )
    {
      return TimeVal(rhs)+=lhs;
    }

None of these routines does allocation.  This technique is described in detail in 
the "Effective C++" books (which I recommended you check first) and is 
exemplified by watersons |nsInt64| class (which I also pointed you at)

  <http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsInt64.h>

Of course you leak.  No one is around to call |delete| on the anonymous pointer 
value you return from |operator+|.  If |new| doesn't appear in your copy-
constructor or assignment operator, then it (probably) should not appear in your 
|operator+| or |operator+=|.

Comment 2

[Jay Patel [:jay]]

Putting on [dogfood+] radar. 

Comment 3

[Jay Patel [:jay]]

Putting on [nsbeta2+] radar for beta2 fix. 

Comment 4

[Brian Ryner (not reading)]

Attachment: Proposed fix

Comment 5

[Brian Ryner (not reading)]

I think the patch I just attached should stop the leaking TimeVals.  scc, can
you take a look and see if it's ok?

Comment 6

[Scott Collins]

Now that's what I like to see.  Patch looks good.  That should kill the leak.  
Did you test it?  If it runs, r=scc.

Comment 7

[Tomi Leppikangas]

Compiles ok on Sun/WS5.0, leaks are gone too.

Comment 8

[Brian Ryner (not reading)]

Checked in.

Comment 9

[Stuart Parmenter]

i suck.  I (ovbiously) didn't quite understand what scc had said.  thanks for
fixing this :)

Comment 10

[John Morrison]

code bug. verified fixed.