336737 - Crash when switching folders [@ NS_strlen]

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Andrew Schultz]

With seamonkey trunk build 2006050410, I've crashed a few times switching to a different mail folder (usually IMAP) but it doesn't happen consistently.  I don't remember crashing with the previous nightly, 2006050309.

Comment 1

[Andrew Schultz]

Attachment: stacktrace

Comment 2

[Wayne Mery (:wsmwk)]

I got same top of stack with windows on startup of thunderbird trunk 2006050409
talkback id TB18335210

Comment 3

[Wayne Mery (:wsmwk)]

There are a few others for windows also reported since yesterday 5/4. The oldest deployment id is MozillaOrgMozillaTrunkWin322006050310 for TB18287346

Comment 4

[Andrew Schultz]

I went back to the 2006050309 build and did still hit this.  2006050210 seems ok.

Comment 5

[timeless]

Attachment: null pointers are not legal for strlen and strdup and friends, they aren't healthy in general either

Comment 6

[David :Bienvenu]

Comment on attachment 221114 [details] [diff] [review]
null pointers are not legal for strlen and strdup and friends, they aren't healthy in general either

I've actually already got a different patch in my tree that uses the empty string instead of returning errors, so I'm going to decide which I prefer. Also, something changed underneath this code to cause this crash - so I'd like to investigate the underlying cause...

Comment 7

[timeless]

*** Bug 336893 has been marked as a duplicate of this bug. ***

Comment 8

[Benjamin Smedberg]

I changed nsCRT::strlen (which became NS_strlen) to not accept null input... mostly accidentally, but I think it's the right thing to do if the callers can be easily fixed.

Comment 9

[David :Bienvenu]

that's always been part of the contract of the nsCRT:: string routines, part of their raison d'etre, if you will, and part of the reason we used them instead of the vanilla c library or PL_ routines. A better thing to do might be to fix all the callers to null check and not use nsCRT:: routines, but leave the nsCRT:: routines the way there were...

Comment 10

[Benjamin Smedberg]

The reason for nsCRT:: functions was because we needed a strlen for prunichar*, not just char*. The nsCRT::strlen(char*) function has crashed-on-null for a long time.

Comment 11

[David :Bienvenu]

Attachment: tweak timeless's fix

Comment 12

[timeless]

Comment on attachment 221337 [details] [diff] [review]
tweak timeless's fix

thanks for addressing most of my XXXs, however you removed one w/o addressing it:

nsMsgGroupThread *nsMsgGroupView::AddHdrToThread(nsIMsgDBHdr *msgHdr, PRBool *pNewThread)
  nsHashKey *hashKey = AllocHashKeyForHdr(msgHdr);
  // XXX using this pointer without null checking will almost certainly

Why? the code ends up sticking a null pointer into a hash among other strange things....

Comment 13

[David :Bienvenu]

Attachment: patch for checkin

this is the -u patch for checkin, also addresses timeless's last comment.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

*** Bug 337580 has been marked as a duplicate of this bug. ***

Comment 15

[Adam Guthrie]

*** Bug 337672 has been marked as a duplicate of this bug. ***

Comment 16

[Scott MacGregor]

I checked this in for David now that the tree is open again.

Comment 17

[Scott MacGregor]

Even with the patch checked in, I'm still seeing talkback reports today with this crash such as:

TB18759487M

Comment 18

[Scott MacGregor]

Attachment: additional fix

This additional change might fix the crash seen in TB18759487M.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

+  *aRecipientsString = nsCRT::strdup(unparsedRecipients.IsEmpty() ? NS_LITERAL_STRING("").get() : unparsedRecipients.get());

Could this be:
  *aRecipientsString = ToNewUnicode(unparsedRecipients);
?

Comment 20

[David :Bienvenu]

should this be marked fixed now? 

Comment 21

[Wayne Mery (:wsmwk)]

Bienvenu in comment #20
> should this be marked fixed now? 

one would think. both patches checked in, and NS_strlen doesn't appear in TB trunk or TB2 on talkback

Comment 22

[David :Bienvenu]

thx, marking fixed.