bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Mismatched delete / delete [] in nsCategoryManager.cpp

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

15 years ago
Caught by valgrind the changes for bug 193031 do delete mArray where they should
do delete []mArray
==9397== Mismatched free() / delete / delete []
==9397==    at 0x40047092: __builtin_delete (vg_clientfuncs.c:194)
==9397==    by 0x403EBC32: ???
(/mnt/ibm/mozhack/mozilla/xpcom/components/nsCategoryManager.cpp:101)
==9397==    by 0x403EC682: ???
(/mnt/ibm/mozhack/mozilla/xpcom/components/nsCategoryManager.cpp:188)
==9397==    by 0x403E8CC7: BaseStringEnumerator::Release(void)
(/mnt/ibm/mozhack/mozilla/xpcom/components/nsCate
goryManager.cpp:110)
==9397==    by 0x40394D3B: ??? (../../dist/include/xpcom/nsCOMPtr.h:486)
==9397==    by 0x403EAD50: NS_CreateServicesFromCategory(char const *,
nsISupports *, char const *) (/mnt/ibm/mo
zhack/mozilla/xpcom/components/nsCategoryManager.cpp:794)
==9397==    by 0x403F55BD: nsComponentManagerImpl::AutoRegisterImpl(int, nsIFile
*, int) (/mnt/ibm/mozhack/mozil
la/xpcom/components/nsComponentManager.cpp:3190)
==9397==    by 0x403F6420: nsComponentManagerImpl::AutoRegister(nsIFile *)
(/mnt/ibm/mozhack/mozilla/xpcom/compo
nents/nsComponentManager.cpp:3405)
==9397==    by 0x804C496: main
(/mnt/ibm/mozhack/mozilla/js/src/xpconnect/shell/xpcshell.cpp:902)
==9397==    by 0x4059917D: __libc_start_main (in /lib/libc-2.2.5.so)
==9397==    by 0x804AAB1: (within
/mnt/ibm/mozhack/obj-i686-pc-linux-gnu-qt/js/src/xpconnect/shell/xpcshell)
==9397==    Address 0x41807A80 is 0 bytes inside a block of size 4 alloc'd
==9397==    at 0x40046F28: __builtin_vec_new (vg_clientfuncs.c:156)
==9397==    by 0x403E9026: EntryEnumerator::Create(nsTHashtable<CategoryLeaf> &)
(/mnt/ibm/mozhack/mozilla/xpcom
/components/nsCategoryManager.cpp:186)
==9397==    by 0x403E94EA: CategoryNode::Enumerate(nsISimpleEnumerator **)
(/mnt/ibm/mozhack/mozilla/xpcom/compo
nents/nsCategoryManager.cpp:335)
==9397==    by 0x403EA070: nsCategoryManager::EnumerateCategory(char const *,
nsISimpleEnumerator **) (/mnt/ibm/
mozhack/mozilla/xpcom/components/nsCategoryManager.cpp:611)
==9397==    by 0x403EA7B2: NS_CreateServicesFromCategory(char const *,
nsISupports *, char const *) (/mnt/ibm/mo
zhack/mozilla/xpcom/components/nsCategoryManager.cpp:757)
==9397==    by 0x403F55BD: nsComponentManagerImpl::AutoRegisterImpl(int, nsIFile
*, int) (/mnt/ibm/mozhack/mozil
la/xpcom/components/nsComponentManager.cpp:3190)
==9397==    by 0x403F6420: nsComponentManagerImpl::AutoRegister(nsIFile *)
(/mnt/ibm/mozhack/mozilla/xpcom/compo
nents/nsComponentManager.cpp:3405)
==9397==    by 0x804C496: main
(/mnt/ibm/mozhack/mozilla/js/src/xpconnect/shell/xpcshell.cpp:902)
==9397==    by 0x4059917D: __libc_start_main (in /lib/libc-2.2.5.so)
==9397==    by 0x804AAB1: (within
/mnt/ibm/mozhack/obj-i686-pc-linux-gnu-qt/js/src/xpconnect/shell/xpcshell)

note that delete([]) is null safe so I'm going to remove the if condition too.
(Assignee)

Comment 1

15 years ago
Created attachment 125916 [details] [diff] [review]
use delete[]
(Assignee)

Updated

15 years ago
Attachment #125916 - Flags: review?(dougt)

Comment 2

15 years ago
Comment on attachment 125916 [details] [diff] [review]
use delete[]

delete[] needs to be protected against null.  (delete without the braces
doesn't need to be.  on of the inconsistencies we all love about c++ memory
management)

if that and r=me.

Comment 3

15 years ago
Comment on attachment 125916 [details] [diff] [review]
use delete[]

r/sr=alecf - this is obvious stuff. (and for those reading, yes its safe to
delete[] a null value.
Attachment #125916 - Flags: superreview+
Attachment #125916 - Flags: review?(dougt)
Attachment #125916 - Flags: review+

Comment 4

15 years ago
oops, sorry I didn't see dougt's comment. that's pretty silly of C++. Bad language!
(Assignee)

Comment 5

15 years ago
err I try to keep track of these things and I was fairly certain that the C++
standard and even all of the compilers we support agree that delete[] 0 is safe.

Anyway, wrt C++ Standard, 5.3.5 #2 says delete[] 0 is safe.
bsmedberg checked an official copy, i checked the only online thing i could
find: http://std.dkuug.dk/jtc1/sc22/open/n2356/expr.html#expr.delete
1 The  delete-expression  operator  destroys  a  most   derived   object
  (_intro.object_) or array created by a new-expression.
          delete-expression:
                  ::opt delete cast-expression
                  ::opt delete [ ] cast-expression
  The ... second is for
  arrays....

2 ...  In either alternative, if the value of the
  operand of delete is the null pointer the operation has no effect....

The c++ portability guidelines don't say anything on this. do we actually know
of any compilers that are so c++98 broken that they do something silly for
delete[] 0?

If we do, then I'll add it to the portability guideline, otherwise I stand by my
memory of the standard and want to remove the if :)

Either way, it's time for me to add a note to the portability guidelines at least on
not mixing
	new[]	w/ delete	(or free) or
	new	w/ delete[]	(or free) or
	malloc	w/ delete or delete[] 
and that
	delete 0 is safe.

That said, i'm going to check this in when the tree opens w/o removing the if
per dougt.
Status: NEW → ASSIGNED

Comment 6

15 years ago
delete [] NULL is definitely not safe for all compilers.  maybe my info is old
and/or the compilers we care about do the right thing in this case.
yeah, I hvae this vague recolleciton that something didn't like delete[] NULL,
but I also think that that was vacpp, which we no longer support. Not sure on
that, though.
(Assignee)

Comment 8

15 years ago
oh well, checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.