Use appropriate version of delete in storage block...

Use appropriate version of delete in storage block
depending on how storage was allocated.

[Craig Schlenter, 2008-12-18 05:48:07]

I'm not sure who can review this but it should fix an error that valgrind
spotted - data was being allocated with new[] but it was freed with delete
instead of delete[].

I have only tested this using net_perftests under linux and I am unfamiliar with
the code so please review carefully.

Thank you!

Snippets of the valgrind output follow.

==12796== Mismatched free() / delete / delete []
==12796==    at 0x40054AA: operator delete(void*) (vg_replace_malloc.c:342)
==12796==    by 0x818DA93:
disk_cache::StorageBlock<disk_cache::EntryStore>::~StorageBlock()
(storage_block-inl.h:27)
==12796==    by 0x818BE34: disk_cache::EntryImpl::~EntryImpl()
(entry_impl.cc:142)
==12796==    by 0x81848D8: 
[snip, lots of other stuff]
==12796==  Address 0x5491e38 is 0 bytes inside a block of size 512 alloc'd
==12796==    at 0x4005E1E: operator new[](unsigned) (vg_replace_malloc.c:268)
==12796==    by 0x8184873:
disk_cache::StorageBlock<disk_cache::EntryStore>::AllocateData()
(storage_block-inl.h:119)

[Peter Kasting, 2008-12-18 06:17:03]

Added Ricardo to reviewers.

There's a couple of things in this file that look odd to me (why use
"sizeof(*data_)" instead of "sizeof T"?  Why allocate to a void* and then to
data instead of just doing "data_ = new T[...]"?  Is it safe that we never
change |extended_| in SetData()?), but they're outside the scope of this patch.

http://codereview.chromium.org/15032/diff/1/2
File net/disk_cache/storage_block-inl.h (right):

http://codereview.chromium.org/15032/diff/1/2#newcode26
Line 26: if (own_data_) {
Doing this here isn't enough.  We also delete in SetData().  You should write a
helper function that does this and then call it from both places.

[Craig Schlenter, 2008-12-18 06:53:58]

Thanks for the review Peter. I have added a function now to do the deleting and
it's called from the destructor and from SetData.

[Peter Kasting, 2008-12-18 07:13:23]

LGTM

http://codereview.chromium.org/15032/diff/5/6
File net/disk_cache/storage_block-inl.h (right):

http://codereview.chromium.org/15032/diff/5/6#newcode122
Line 122: if (own_data_) {
Nit: I'd use an early return here.

[rvargas (doing something else), 2008-12-18 18:53:17]

http://codereview.chromium.org/15032/diff/5/6
File net/disk_cache/storage_block-inl.h (right):

http://codereview.chromium.org/15032/diff/5/6#newcode126
Line 126: delete[] data_;
This is not correct. Although it may seem like it is an array of T, it is
actually a single T object with some extra space at after it. To maintain the
proper behavior you have to call the destructor explicitly  and use a cast to
free the memory. See http://code.google.com/p/chromium/issues/detail?id=5041

[Craig Schlenter, 2008-12-19 05:12:18]

Thanks!! I should have searched issue tracker first and brushed up on my
placement new skills - sorry. I can't seem to upload a fixed patch at the moment
"Error accessing url /15032/description", but I've changed the delete[] to:

data_->~T();
delete[] reinterpret_cast<char*>(data_);

Hopefully that's right now.

I will keep trying to upload a fresh copy ...

[Craig Schlenter, 2008-12-19 07:17:00]

Uploaded a fresh patch that calls the destructor and then does a delete[] with
cast. I'll add a comment in issue 5041 too pointing to here.

[rvargas (doing something else), 2008-12-19 20:50:18]

LGTM

On 2008/12/19 07:17:00, Craig Schlenter wrote:
> Uploaded a fresh patch that calls the destructor and then does a delete[] with
> cast. I'll add a comment in issue 5041 too pointing to here.