98839 - UMR in TextRun::AddSegment (4268 times)

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[John Morrison]

I was running purify (looking for an ABW for bug 98306), and I turned up this 
UMR (actually, I've seen it forever, just never reported it; my bad). I haven't
pondered whether it's valid, or invalid, but here it is. Decide what to do. 

CVS Blame points to troy, but in his absence, I'm assigning to some layout 
dudes,
and they can fight for the honor or owning this bug.

The test was simply to start, load a page and then shutdown.

Note that there are 4268 of these UMR's, so even if this is bogus, if you could
figure out a way to fool Purify with some casting fu, or something, it would 
clear out a lot of noise.



[W] UMR: Uninitialized memory read in TextRun::AddSegment(int,int,int) {351 
occurrences}
        Reading 4 bytes from 0x00137778 (4 bytes at 0x00137778 uninitialized)
        Address 0x00137778 points into a thread's stack 
        Thread ID: 0x50c
        Error location
            TextRun::AddSegment(int,int,int) [nsTextFrame.cpp:4304]
                #endif // IBMBIDI
                    mTotalNumChars += aNumChars;
                    mBreaks[mNumSegments] = mTotalNumChars;
             =>     mSegments[mNumSegments].mIsWhitespace = aIsWhitespace;
                    mTotalContentLen += aContentLen;
                    mSegments[mNumSegments].mContentLen = 
PRUint32(mTotalContentLen);
                    mNumSegments++;
            ???            [ip=0x00137ab4]
            nsTextFrame::MeasureText(nsIPresContext *,nsHTMLReflowState 
const&,nsTextTransformer&,nsILineBreaker 
*,TextStyle::nsTextFrame&,TextReflowData::nsTextFrame&) [nsTextFrame.cpp:4495]
                      }
                      else if (textRun.IsBuffering()) {
                        // Add a whitespace segment
             =>         textRun.AddSegment(wordLen, contentLen, PR_TRUE);
                        continue;
                      }
                      else {
            nsTextFrame::Reflow(nsIPresContext 
*,nsHTMLReflowMetrics&,nsHTMLReflowState const&,UINT&) [nsTextFrame.cpp:5054]
            nsLineLayout::ReflowFrame(nsIFrame *,nsIFrame * 
*,UINT&,nsHTMLReflowMetrics *,int&) [nsLineLayout.cpp:993]
            
nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&,nsLineLayout&,nsLineBox 
*,nsIFrame *,BYTE *) [nsBlockFrame.cpp:3460]
            
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&,nsLineLayout&,nsLineBox 
*,int *,BYTE *,int,int) [nsBlockFrame.cpp:3344]
            
nsBlockFrame::DoReflowInlineFramesMalloc(nsBlockReflowState&,nsLineBox *,int 
*,BYTE *,int,int) [nsBlockFrame.cpp:3248]
            nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&,nsLineBox 
*,int *,int,int) [nsBlockFrame.cpp:3208]
            nsBlockFrame::ReflowLine(nsBlockReflowState&,nsLineBox *,int *,int) 
[nsBlockFrame.cpp:2358]

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm going to have to guess it's doing this because of the bit fields.  We have:

struct SegmentData {
  PRUint32  mIsWhitespace : 1;
  PRUint32  mContentLen : 31;  // content length
  <...>
};

struct TextRun {
  <...>
  SegmentData   mSegments[TEXT_MAX_NUM_SEGMENTS];
  <...>
  void AddSegment(PRInt32 aNumChars, PRInt32 aContentLen, PRBool aIsWhitespace)
  {
    <...>
    mSegments[mNumSegments].mIsWhitespace = aIsWhitespace;
    mTotalContentLen += aContentLen;
    mSegments[mNumSegments].mContentLen = PRUint32(mTotalContentLen);
    <...>
  }
};

I wonder if purify is detecting that setting the low/high bit using some
operation that reads and then writes.  Perhaps reordering some of these
statements would fix that?

I don't see how this could be a real UMR considering that aIsWhitespace is
passed as PR_TRUE.

Comment 2

[John Morrison]

After looking at it, and doing a purify+debug run through that code path, I 
agree that this is a bogus warning from Purify.

As for what I said earlier about "if you could figure out a way ...", it's 
just not worth the effort; Purify can easily filter these out. So, feel free
to mark this bug invalid, and this bug will be available for future reference
should anyone else wonder about this message.

Comment 3

[Chris Waterson]

This shouldn't make Purify data any harder to read (since pfy groups together
similar errors), so I'm tempted to WONTFIX. If it _is_ unbearable, we could add
some #ifdef DEBUG code that initializes the bit fields.

Comment 4

[John Morrison]

No, wontfix is fine. Purify does let you filter this out; I just wanted to know 
that I wasn't filtering out something relevant instead of noise.

Comment 5

[John Morrison]

.

Comment 6

[timeless]

The way I read the statement is:
mSegments[mNumSegments]=(aIsWhiteSpace<<31)|(mSegments[mNumSegments]&~(1<<31));
mTotalContentLen += aContentLen;
mSegments[mNumSegments]=(mSegments[mNumSegments]&(1<<31)|(mTotalContentLen&~
(1<<31));

Which is why i sympathize w/ purify.

would people object to an inline setter which takes the two fields and does the 
math for the compiler? it seems like the warning (and this is coming from a 
vc7.0 opt build) indicates the compiler wasn't able to optimize the code two 
assignments.

fwiw from pre 1.7a
[W] UMR: Uninitialized memory read in TextRun::AddSegment(int,int,int) {101 
occurrences}
        Reading 4 bytes from 0x0013dbc8 (4 bytes at 0x0013dbc8 uninitialized)
        Address 0x0013dbc8 points into a thread's stack 
        Address 0x0013dbc8 is 316 bytes above the frame pointer in 
TextRun::AddSegment(int,int,int)
        Thread ID: 0xbcc
        Error location
            TextRun::AddSegment(int,int,int)+0x66 
[r:\cenzic\mozilla\layout\html\base\src\nstextframe.cpp:4503 ip=0x049b1005]
                      return;
                    }
                #endif // IBMBIDI
                    mTotalNumChars += aNumChars;
                    mBreaks[mNumSegments] = mTotalNumChars;
             =>     mSegments[mNumSegments].mIsWhitespace = aIsWhitespace;