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)
Tracking
()
RESOLVED
FIXED
mozilla1.7beta
People
(Reporter: calum.mackay, Assigned: darin.moz)
References
Details
Attachments
(3 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•21 years ago
|
||
Reporter | ||
Comment 2•21 years ago
|
||
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...
Assignee | ||
Comment 3•21 years ago
|
||
*** Bug 234911 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 4•21 years ago
|
||
-> me
Assignee: mscott → string
Component: Mail Window Front End → String
Product: Thunderbird → Browser
Target Milestone: --- → mozilla1.7beta
Version: unspecified → Trunk
Assignee | ||
Comment 6•21 years ago
|
||
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.
Assignee | ||
Comment 7•21 years ago
|
||
of course... one does have to wonder why encoding is null in this case. maybe
this indicates a bigger problem...
Comment 8•21 years ago
|
||
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.
Assignee | ||
Comment 9•21 years ago
|
||
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?
Comment 10•21 years ago
|
||
Thanks, that fixed the problem. Should we land the patch for the now?
Assignee | ||
Comment 11•21 years ago
|
||
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!
Comment 12•21 years ago
|
||
Fix landed with an 'XXX comment'.
Comment 13•21 years ago
|
||
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.
Comment 14•21 years ago
|
||
The branch was cut before the string landing.
Comment 15•21 years ago
|
||
er, the tag. There is no branch.
Yeah, this is the nsXPIDLString lazy length calculation issue...
Assignee | ||
Comment 16•21 years ago
|
||
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.
Comment 17•21 years ago
|
||
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...
Assignee | ||
Comment 18•21 years ago
|
||
grepping for getter_Copies in mailnews revealed a few places where this problem
still exists. this patch fixes those.
Assignee | ||
Updated•21 years ago
|
Attachment #143015 -
Flags: superreview?(bienvenu)
Attachment #143015 -
Flags: review?(bienvenu)
Updated•21 years ago
|
Attachment #143015 -
Flags: superreview?(bienvenu)
Attachment #143015 -
Flags: superreview+
Attachment #143015 -
Flags: review?(bienvenu)
Attachment #143015 -
Flags: review+
Assignee | ||
Updated•21 years ago
|
Summary: regular crashes in nsCharTraits::length() → getter_Copies scoping problem [was: regular crashes in nsCharTraits::length()]
Assignee | ||
Comment 19•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #143018 -
Flags: superreview?(bienvenu)
Attachment #143018 -
Flags: review?(bienvenu)
Assignee | ||
Updated•21 years ago
|
Attachment #143018 -
Flags: superreview?(dbaron)
Attachment #143018 -
Flags: superreview?(bienvenu)
Attachment #143018 -
Flags: review?(dbaron)
Attachment #143018 -
Flags: review?(bienvenu)
Comment 20•21 years ago
|
||
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+
Assignee | ||
Comment 21•21 years ago
|
||
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
Updated•4 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•