If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla1.7beta

Status

()

Core
String
--
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Calum Mackay, Assigned: Darin Fisher)

Tracking

Trunk
mozilla1.7beta
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

14 years ago
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

14 years ago
Created attachment 141754 [details]
stack trace
(Reporter)

Comment 2

14 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

14 years ago
*** Bug 234911 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 4

14 years ago
-> me
Assignee: mscott → string
Component: Mail Window Front End → String
Product: Thunderbird → Browser
Target Milestone: --- → mozilla1.7beta
Version: unspecified → Trunk
(Assignee)

Comment 5

14 years ago
-> me (for real)
Assignee: string → darin
(Assignee)

Updated

14 years ago
Blocks: 231995
Status: NEW → ASSIGNED
(Assignee)

Comment 6

14 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

14 years ago
of course... one does have to wonder why encoding is null in this case.  maybe
this indicates a bigger problem...

Comment 8

14 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

14 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

14 years ago
Thanks, that fixed the problem. Should we land the patch for the now?
(Assignee)

Comment 11

14 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

14 years ago
Fix landed with an 'XXX comment'. 

Comment 13

14 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.
The branch was cut before the string landing.
er, the tag.  There is no branch.


Yeah, this is the nsXPIDLString lazy length calculation issue...
(Assignee)

Comment 16

14 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

14 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

14 years ago
Created attachment 143015 [details] [diff] [review]
mailnews patch

grepping for getter_Copies in mailnews revealed a few places where this problem
still exists.  this patch fixes those.
(Assignee)

Updated

14 years ago
Attachment #143015 - Flags: superreview?(bienvenu)
Attachment #143015 - Flags: review?(bienvenu)

Updated

14 years ago
Attachment #143015 - Flags: superreview?(bienvenu)
Attachment #143015 - Flags: superreview+
Attachment #143015 - Flags: review?(bienvenu)
Attachment #143015 - Flags: review+
(Assignee)

Updated

14 years ago
Summary: regular crashes in nsCharTraits::length() → getter_Copies scoping problem [was: regular crashes in nsCharTraits::length()]
(Assignee)

Comment 19

14 years ago
Created attachment 143018 [details] [diff] [review]
same thing for the rest of the tree
(Assignee)

Updated

14 years ago
Attachment #143018 - Flags: superreview?(bienvenu)
Attachment #143018 - Flags: review?(bienvenu)
(Assignee)

Updated

14 years ago
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+
(Assignee)

Comment 21

14 years ago
last patch checked in.  marking bug FIXED.

i'll post a document about this problem to the newsgroups shortly...
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.