74773 - UMR in AccumulateCRC()

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[kinmoz]

When running Purify on my 04/04/01 Mozilla Win32 Debug build, with classic skin, 
sfaser and I were seeing a UMR in AccumulateCRC(), nsStyleContext.cpp line 4392:

    i = ( (int) ( crc_accum >> 24) ^ *data_blk_ptr++ ) & 0xff;

I believe the UMR is hit several times just starting up the browser, if not, try 
going to www.mozilla.org.

Note that Purify reported a total of 16 occurrences of this UMR, but the stack 
traces all looked the same, so I'm only including one of them:


[W] UMR: Uninitialized memory read in AccumulateCRC {4 occurrences}
        Reading 1 byte from 0x0bd68484 (1 byte at 0x0bd68484 uninitialized)
        Address 0x0bd68484 is 540 bytes into a 764 byte block at 0x0bd68268
        Address 0x0bd68484 points to a C++ new block in heap 0x03620000
        Thread ID: 0x89
        Error location
            AccumulateCRC  [nsStyleContext.cpp:4392]
            StyleMarginCRC [nsStyleContext.cpp:4426]
            StyleMarginImpl::ComputeCRC32(UINT)const [nsStyleContext.cpp:451]
            nsStyleContextData::ComputeCRC32(UINT)const 
[nsStyleContext.cpp:2456]
            nsStyleContextData::SetCRC32(void) [nsStyleContext.cpp:2084]
            StyleContextImpl::ShareStyleData(void) [nsStyleContext.cpp:3852]
            StyleContextImpl::RemapStyle(nsIPresContext *,int) 
[nsStyleContext.cpp:3617]
            NS_NewStyleContext(nsIStyleContext * *,nsIStyleContext *,nsIAtom 
*,nsISupportsArray *,nsIPresContext *) [nsStyleContext.cpp:4345]
            StyleSetImpl::GetContext(nsIPresContext *,nsIStyleContext *,nsIAtom 
*,nsISupportsArray *,int,int&) [nsStyleSet.cpp:838]
            StyleSetImpl::ResolveStyleFor(nsIPresContext *,nsIContent 
*,nsIStyleContext *,int) [nsStyleSet.cpp:922]
        Allocation location
            new(UINT)      [new.cpp:23]
            nsStyleContextData::Create(nsIPresContext *) 
[nsStyleContext.cpp:2176]
            StyleContextImpl::EnsureStyleData(nsIPresContext *) 
[nsStyleContext.cpp:3787]
            StyleContextImpl::RemapStyle(nsIPresContext *,int) 
[nsStyleContext.cpp:3442]
            NS_NewStyleContext(nsIStyleContext * *,nsIStyleContext *,nsIAtom 
*,nsISupportsArray *,nsIPresContext *) [nsStyleContext.cpp:4345]
            StyleSetImpl::GetContext(nsIPresContext *,nsIStyleContext *,nsIAtom 
*,nsISupportsArray *,int,int&) [nsStyleSet.cpp:838]
            StyleSetImpl::ResolveStyleFor(nsIPresContext *,nsIContent 
*,nsIStyleContext *,int) [nsStyleSet.cpp:922]
            nsPresContext::ResolveStyleContextFor(nsIContent *,nsIStyleContext 
*,int,nsIStyleContext * *) [nsPresContext.cpp:670]
            nsCSSFrameConstructor::ResolveStyleContext(nsIPresContext *,nsIFrame 
*,nsIContent *,nsIAtom *,nsIStyleContext * *) [nsCSSFrameConstructor.cpp:6706]
            nsCSSFrameConstructor::ConstructFrame(nsIPresShell *,nsIPresContext 
*,nsFrameConstructorState&,nsIContent *,nsIFrame *,nsFrameItems&) 
[nsCSSFrameConstructor.cpp:7061]

Comment 1

[Pierre Saslawsky]

I'm going to attach a simple fix, if Marc or Simon wants to review.

Comment 2

[Pierre Saslawsky]

Attachment: fix

Comment 3

[Pierre Saslawsky]

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

Comment 4

[Simon Fraser [no longer active]]

Looks good. But were mCachedMargin, mCachedPadding, mCachedBorder and 
mCachedOutlineWidth never initialized in the ctor?

Comment 5

[Marc Attinasi]

Pierre, is your patch against your local changes, or am I missing something? I
cannot see how it will remedy the UMR complaints since the association between
the HasCached flag and the data member is semantic, not syntactic. Also, what
Simon asked...

Comment 6

[Pierre Saslawsky]

Marc, I don't understand your question.

Comment 7

[Marc Attinasi]

Never mind Pierre, I think I was drinking too much Starbucks - I understand it
now and It looks fine. Echoing what Simon mentioned: should we consider
initializing the cache-data (in ResetFrom rather than the ctor, I think?).

Comment 8

[Pierre Saslawsky]

I prefered to patch it that way (ie. don't use the cached data if it's not valid, 
like everywhere else) rather than initializing the cached data because it was 
more efficient.  A nsMargin contains 4 PRUnt32 and we use approximately 6000 of 
them for margins, 7000 for paddings and 8200 for borders to construct the 
Netscape home page.  That's almost 85000 initializations.

Comment 9

[Marc Attinasi]

Wow, but that is without your data sharing, right? ;)

[s]r=attinasi for the patch as it is, your reasoning is sound Pierre.

Comment 10

[Pierre Saslawsky]

Ooops, let me rectify these numbers...  They correspond to the number of calls to 
GetStyleData for each of the structs.  My mistake: GetStyleData returns a 
pointer, that's it.  In fact, I should have looked at GetMutableStyleData that 
will result in the new APIs into the creation of a temporary structure on the 
stack to return the data.  The numbers would be much lower: (520 + 880 + 960) * 4 
= 9440 initializations.  Not much of a gain, but why not?

As you have seen, we call GetStyleData a lot but there isn't much we can do about 
it.  I found places, though, where the style context could provide utility 
functions instead of requiring the clients to iterate through the contexts while 
looking for a particular style.

Comment 11

[karnaze (gone)]

Moving to m0.9.2

Comment 12

[Pierre Saslawsky]

fixed

Comment 13

[Hixie (not reading bugmail)]

code level bug, -> reporter for QA