494756 - Wrong usage of delete on allocated m_msgReferences crashes switching to newsgroup

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Zdenek Kabelac]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; cs-CZ; rv:1.9.1b4) Gecko/20090427 Fedora/3.5-0.20.beta4.fc11 Firefox/3.5b4
Build Identifier: 

While looking for the reason why my TB often crashes I've noticed with valgrind this problem:

==10871== Thread 1:
==10871== Invalid free() / delete / delete[]
==10871==    at 0x4C23E3F: operator delete(void*) (vg_replace_malloc.c:342)
==10871==    by 0x185B317A: nsMsgDatabase::~nsMsgDatabase() (nsMsgDatabase.cpp:887)
==10871==    by 0x185B882C: nsNewsDatabase::~nsNewsDatabase() (nsNewsDatabase.cpp:57)
==10871==    by 0x185A9C7E: nsMsgDatabase::Release() (nsMsgDatabase.cpp:926)



The reason for this error is obvious - it calls C++ 'delete' on a pointer that was created  m_msgReferences = PL_NewDHashTable() (malloc)- thus instead should be destroyed with this call PL_DHashTableDestroy() (free).
(as PL is C library not doesn't generate C++ objects)


Reproducible: Always

Steps to Reproduce:
1.run TB within valgrind
2.
3.

Comment 1

[Ludovic Hirlimann [:Usul]]

I supose you are using Thunderbird 3.0beta ?

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> I supose you are using Thunderbird 3.0beta ?

If he isn't the bug is still there: http://mxr.mozilla.org/comm-central/search?string=m_msgReferences

Zdenek, fancy doing a patch for us? See https://developer.mozilla.org/En/Getting_your_patch_in_the_tree for more info, or just ask.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

I'll handle the patch.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

Calls PL_DHashTableDestroy() instead of delete.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 379550 [details] [diff] [review]
patch

David, can you review this one please?

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch + space after if

the one with better formatting.

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; cs-CZ; rv:1.9.1b4)
> Gecko/20090427 Fedora/3.5-0.20.beta4.fc11 Firefox/3.5b4
> Build Identifier: 
> 
> While looking for the reason why my TB often crashes 

Is there a bug# for that?  Or is this bug for that crash?

Comment 8

[Ludovic Hirlimann [:Usul]]

(In reply to comment #7)
> (In reply to comment #0)
> > User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; cs-CZ; rv:1.9.1b4)
> > Gecko/20090427 Fedora/3.5-0.20.beta4.fc11 Firefox/3.5b4
> > Build Identifier: 
> > 
> > While looking for the reason why my TB often crashes 
> 
> Is there a bug# for that?  Or is this bug for that crash?
this is the bug for the crash with the proposed fix.

Comment 9

[Zdenek Kabelac]

If you ask for my original problem - I've created RH BZ:

https://bugzilla.redhat.com/show_bug.cgi?id=494824

I'm not really sure if there is similar bug in the mozilla BZ.

Now I'm testing if it will segfault with Martin's new build with fixed
destructor.

It always somehow depends on the message order in the news group or my mailbox
- but it's not so easy to create minimal configuration. Unless I would upload
somewhere my complete mailbox from crash time, which is something I'm not going
to do.

Comment 10

[David :Bienvenu]

Comment on attachment 379551 [details] [diff] [review]
patch + space after if

this patch definitely looks right,thx, and I'll try it out - for future reference, we want diffs that hg can qimport, which means -p1 diffs..

Comment 11

[David :Bienvenu]

Comment on attachment 379551 [details] [diff] [review]
patch + space after if

thx for the patch, I've pushed it. We've never seen any crashes because of this problem, but the patch definitely seems like the right thing to do.

Comment 12

[David :Bienvenu]

fix checked in.

Comment 13

[Wayne Mery (:wsmwk)]

I didn't look at these hard - might bug 493186 be related?

Comment 14

[David :Bienvenu]

No, very unlikely that bug 493186 is related.

Comment 15

[Zdenek Kabelac]

I could imagine that my crash could be a result of some reuse of already wrongly destroyed elements (as it looked like usage of corrupted memory) - thought so far I'm in the 'wait' mode whether I'll see those crashes again when I'm now using fixed binary.

Also it looks like TB now takes less memory than it used to - but still there is huge amount of leaked memory chunks being shown by valgrind.

Comment 16

[Ludovic Hirlimann [:Usul]]

(In reply to comment #10)
> (From update of attachment 379551 [details] [diff] [review])
> this patch definitely looks right,thx, and I'll try it out - for future
> reference, we want diffs that hg can qimport, which means -p1 diffs..

documentation is at https://developer.mozilla.org/en/Installing_Mercurial#configuration

(In reply to comment #15)
> Also it looks like TB now takes less memory than it used to - but still there
> is huge amount of leaked memory chunks being shown by valgrind.

Please file bugs for these adn make sure to add the mlk keyword to those.

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #16)
> (In reply to comment #15)
> > Also it looks like TB now takes less memory than it used to - but still there
> > is huge amount of leaked memory chunks being shown by valgrind.
> 
> Please file bugs for these adn make sure to add the mlk keyword to those.

Need to be careful here, there are already a lot of leak bugs filed:

https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&product=MailNews+Core&product=Thunderbird&long_desc_type=substring&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=mlk&resolution=---&emailassigned_to1=1&emailtype1=exact&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=

and if they are shutdown leaks they will likely be covered by bug 426963.

I'd certainly be interested in seeing the list though.

Comment 18

[Zdenek Kabelac]

I've opened new bug 495038 with attachment from valgrind - as I'm not TB developer - I hope some developer will overlook the trace and check for problems.