Bug 51367 - Deallocate GregorianDateTime.timeZone (if allocated) when copying so that we don't leak memory.
Summary: Deallocate GregorianDateTime.timeZone (if allocated) when copying so that we ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-20 17:40 PST by Daniel Bates
Modified: 2010-12-21 13:22 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.14 KB, patch)
2010-12-20 17:43 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2010-12-20 20:06 PST, Daniel Bates
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2010-12-20 17:40:41 PST
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.
Comment 1 Daniel Bates 2010-12-20 17:43:58 PST
Created attachment 77060 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-12-20 18:25:28 PST
Comment on attachment 77060 [details]
Patch

Can't we use an OwnArrayPtr here?  That would be much cleaner.
Comment 3 Antonio Gomes 2010-12-20 18:26:31 PST
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?
Comment 4 Daniel Bates 2010-12-20 20:06:15 PST
Created attachment 77077 [details]
Patch

Updated patch based on Eric Seidel's suggestion.
Comment 5 Daniel Bates 2010-12-20 20:08:02 PST
(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>.
Comment 6 Eric Seidel (no email) 2010-12-20 20:15:40 PST
Comment on attachment 77077 [details]
Patch

OK... so now I wonder why we wouldn't just use Vector<char> here.
Comment 7 Eric Seidel (no email) 2010-12-20 20:16:17 PST
Comment on attachment 77077 [details]
Patch

This is strictly better than what we have, so OK.
Comment 8 Build Bot 2010-12-20 20:47:13 PST
Attachment 77077 [details] did not build on win:
Build output: http://queues.webkit.org/results/7292081
Comment 9 Darin Adler 2010-12-21 11:18:42 PST
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.
Comment 10 Daniel Bates 2010-12-21 11:28:02 PST
(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.
Comment 11 Daniel Bates 2010-12-21 13:22:42 PST
Committed r74424: <http://trac.webkit.org/changeset/74424>