Bug 51367 – Deallocate GregorianDateTime.timeZone (if allocated) when copying so that we don't leak memory.

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Daniel Bates]

When copying a GregorianDateTime object B to A using GregorianDateTime.copyFrom() we don't deallocate the timeZone of A before allocating a new timeZone buffer and copying the timeZone of B. Hence, we are leaking memory.

Instead, we should deallocate the timeZone buffer of A then allocate a new timeZone buffer for A and copy the timeZone from B into it.

[Daniel Bates]

Created attachment 77060 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77060 [details]
Patch

Can't we use an OwnArrayPtr here?  That would be much cleaner.

[Antonio Gomes]

Comment on attachment 77060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77060&action=review

> JavaScriptCore/wtf/DateMath.h:204
> +        delete [] timeZone;

should we also set it to '0', in case if (rhs.timeZone) fails?

[Daniel Bates]

Created attachment 77077 [details]
Patch

Updated patch based on Eric Seidel's suggestion.

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 77060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77060&action=review
> 
> > JavaScriptCore/wtf/DateMath.h:204
> > +        delete [] timeZone;
> 
> should we also set it to '0', in case if (rhs.timeZone) fails?

Notice, we do this by line 209 in <http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/DateMath.h?rev=72884#L209>.

[Eric Seidel (no email)]

Comment on attachment 77077 [details]
Patch

OK... so now I wonder why we wouldn't just use Vector<char> here.

[Eric Seidel (no email)]

Comment on attachment 77077 [details]
Patch

This is strictly better than what we have, so OK.

[Build Bot]

Attachment 77077 [details] did not build on win:
Build output: http://queues.webkit.org/results/7292081

[Darin Adler]

Comment on attachment 77077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77077&action=review

Should find out why the Windows build broke.

> JavaScriptCore/wtf/DateMath.cpp:1132
> -    tm.timeZone = NULL;
> +    tm.timeZone.clear();

Side note: The most modern idiom for this is to assign to nullptr. We are going to deprecate clear().

> JavaScriptCore/wtf/DateMath.h:202
> +            timeZone.set(new char[inZoneSize]);

The most modern idiom for this is:

    timeZone = adoptArrayPtr(new char[inZoneSize]);

We are going to deprecate set soon.

[Daniel Bates]

(In reply to comment #9)
> (From update of attachment 77077 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77077&action=review
> 
> Should find out why the Windows build broke.

Will look into and fix before landing.

> 
> > JavaScriptCore/wtf/DateMath.cpp:1132
> > -    tm.timeZone = NULL;
> > +    tm.timeZone.clear();
> 
> Side note: The most modern idiom for this is to assign to nullptr. We are going to deprecate clear().

Will change.

> 
> > JavaScriptCore/wtf/DateMath.h:202
> > +            timeZone.set(new char[inZoneSize]);
> 
> The most modern idiom for this is:
> 
>     timeZone = adoptArrayPtr(new char[inZoneSize]);

Will change.

> 
> We are going to deprecate set soon.

[Daniel Bates]

Committed r74424: <http://trac.webkit.org/changeset/74424>