Closed Bug 234908 Opened 21 years ago Closed 21 years ago

getter_Copies scoping problem [was: regular crashes in nsCharTraits::length()]

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.7beta

People

(Reporter: calum.mackay, Assigned: darin.moz)

References

Details

Attachments

(3 files)

User-Agent: Build Identifier: Starting with today's build, 20040219, I'm reliably able to crash THunderbird - but not seamonkey/mailnews, by browsing through a few particular folders. The SEGV occurs at: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 1084348864 (LWP 17424)] 0x419d756f in nsCharTraits<char>::length(char const*) (s=0x0) at nsCharTraits.h:339 in nsCharTraits.h which is: 245 NS_SPECIALIZE_TEMPLATE 246 struct nsCharTraits<char> 247 { ... 335 static 336 size_t 337 length( const char_type* s ) 338 { 339 return strlen(s); 340 } stack trace will be attached in a moment. Reproducible: Always Steps to Reproduce: 1.I just trawl through certain folders; some emails seem to fire it off, but I don't see anything special about them. 2. 3. I see that nsCharTraits.h was changed yesterday as part of the fix for bug 231995; and the caller, nsTDependentString.h is all-new, I'm thinking it might be related?
Attached file stack trace (deleted) —
I don't seem to be able to reproduce with Thunderbird 20040218 (bug logged against today's 20040219). I've just checked, and my mozilla build is from the 18th, which might explain why I couldn't reproduce it there. So this prob isn't a Thunderbird bug at all...
*** Bug 234911 has been marked as a duplicate of this bug. ***
-> me
Assignee: mscott → string
Component: Mail Window Front End → String
Product: Thunderbird → Browser
Target Milestone: --- → mozilla1.7beta
Version: unspecified → Trunk
-> me (for real)
Assignee: string → darin
Blocks: 231995
Status: NEW → ASSIGNED
here's what nsDependentString::Rebind looked like in the old code: void Rebind( const char_type* aPtr ) { NS_ASSERTION(aPtr, "nsDependentCString must wrap a non-NULL buffer"); mHandle.DataStart(aPtr); // XXX This should not be NULL-safe, but we should flip the switch // early in a milestone. //mHandle.DataEnd(aPtr+nsCharTraits<char_type>::length(aPtr)); mHandle.DataEnd(aPtr ? (aPtr+nsCharTraits<char_type>::length(aPtr)) : 0); } what i have done is make it non-NULL safe b/c it is actually not valid to call nsDependentString(0). however, that might be too much of a change to be making during beta since we might not be able to flush out all the bad callsites. i'll put together a patch that resurrects this test.
of course... one does have to wonder why encoding is null in this case. maybe this indicates a bigger problem...
Right. That's what I wrote in bug 234911. Resurrecting the old behavior in Rebind wouldn't help MathML work correctly although it would fix the crash.
i think i know what's wrong! the new code for getter_Copies doesn't call Adopt on the internal string until after getter_Copies's destructor runs. in this case, that dtor doesn't run in time because we try to use encoding.get() in the same expression. hmm... /me wonders how best to deal with this problem since we don't have a way to do lazy length computation. for now, i think the code needs to be rewritten like this: if (NS_SUCCEEDED(GetEncoding(family, getter_Copies(encoding), fontType, ftEncoding)) { if (NS_SUCCEEDED(GetConverter(encoding.get(), getter_AddRefs(converter)))) { nsCOMPtr<nsICharRepresentable> mapper(do_QueryInterface(converter)); if (PR_LOG_TEST(gXftFontLoad, PR_LOG_DEBUG)) { printf("\t\tc> got the converter and CMap :%s !!\n", encoding.get()); } if (mapper) { ccmap = MapperToCCMap(mapper); } } } jshin, i'm still waiting for my XFT build to complete. can you test this change?
Thanks, that fixed the problem. Should we land the patch for the now?
yes, let's do that since it will at least fix the crash. i need to think about how i want to deal with this situation in general though. please add a XXX comment in that code pointing to this bug. thx!
Fix landed with an 'XXX comment'.
it looks like the 1.7a branch was cut this morning. I'm guessing the fix went into the trunk and not the branch. We might want to get this in on the 1.7a branch too.
The branch was cut before the string landing.
er, the tag. There is no branch. Yeah, this is the nsXPIDLString lazy length calculation issue...
work remaining for this bug (assuming we do not try to re-enable lazy length calculation for nsXPIDLString): (1) write up document explaining how NOT to use getter_Copies (2) search for other places where getter_Copies is being used in this way, and fix them.
I seem to get a crash on viewing any pgp-signed messages since the string checkin... May there be a connection to this issue or is that a seperate bug? Seeing this on a current trunk cvs build on Linux here...
Attached patch mailnews patch (deleted) — Splinter Review
grepping for getter_Copies in mailnews revealed a few places where this problem still exists. this patch fixes those.
Attachment #143015 - Flags: superreview?(bienvenu)
Attachment #143015 - Flags: review?(bienvenu)
Attachment #143015 - Flags: superreview?(bienvenu)
Attachment #143015 - Flags: superreview+
Attachment #143015 - Flags: review?(bienvenu)
Attachment #143015 - Flags: review+
Summary: regular crashes in nsCharTraits::length() → getter_Copies scoping problem [was: regular crashes in nsCharTraits::length()]
Attachment #143018 - Flags: superreview?(bienvenu)
Attachment #143018 - Flags: review?(bienvenu)
Attachment #143018 - Flags: superreview?(dbaron)
Attachment #143018 - Flags: superreview?(bienvenu)
Attachment #143018 - Flags: review?(dbaron)
Attachment #143018 - Flags: review?(bienvenu)
Comment on attachment 143018 [details] [diff] [review] same thing for the rest of the tree You might not want to drop the NS_SUCCEEDED check in the first file. Also, in the 2d, 3d, and 4th files, you could use .get() instead of casting. But it's fine either way.
Attachment #143018 - Flags: superreview?(dbaron)
Attachment #143018 - Flags: superreview+
Attachment #143018 - Flags: review?(dbaron)
Attachment #143018 - Flags: review+
last patch checked in. marking bug FIXED. i'll post a document about this problem to the newsgroups shortly...
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: