Closed Bug 653179 Opened 13 years ago Closed 13 years ago

nsPresShell.cpp:8829:52: warning: 'width' may be used uninitialized in this function

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: zwol)

References

Details

(Whiteboard: [build_warning])

Attachments

(1 file)

Just ran across this build warning:
> nsPresShell.cpp:8829:52: warning: 'width' may be used uninitialized in this function

It's absolutely correct - the code is:
> 8801 void ReflowCountMgr::PaintCount(const char*     aName,
[...]
> 8827       nscoord width, height = fm->MaxHeight();
> 8828       aRenderingContext->SetTextRunRTL(PR_FALSE);
> 8829       aRenderingContext->GetWidth((char*)buf, width);
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8827
(and the above code has basically remained the same since it was checked into CVS in 2001) 

It looks like this *used* to fine, because it was calling a function that took |width| as a reference (to be set as an outparam):
http://hg.mozilla.org/mozilla-central/annotate/faeb9fecfc94/gfx/src/nsThebesRenderingContext.cpp#l874

... but the deCOMtamination in Bug 266236 that converted nsThebesRenderingContext into nsRenderingContext ended up making this invoke a different function that takes |width| as a PRUint32 (***NOT*** a reference), and run through a loop a certain number of times based on its value:
http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsRenderingContext.cpp#515

This is very broken.  It probably doesn't matter, since AFAICT the ReflowCountMgr class is largely unused, but makes me worry that Bug 266236 may have had some other unintended consequences...
Blocks: 266236
OS: Linux → All
Hardware: x86_64 → All
Hm, it looks like this is all inside a giant "#ifdef MOZ_REFLOW_PERF" block -- I'm not actually sure why that's getting evaluated in my build.

(Maybe that's part of why Bug 266236 didn't catch this chunk?)
So, the offending patch in the series is 6/9, which I did by changing a bunch of method signatures and then fixing all the compile errors.  This is a nice case study in why that's not good enough:  we used to be calling GetWidth(char*, nscoord&) where the latter is the function's out-parameter, now we're calling GetWidth(char*, PRUint32) where the latter is the length of the string in bytes.  I will audit all calls to GetWidth and its relatives.

I would like to make the editorial side observation that this sort of thing is why we should not leave uninitialized-variable warnings unfixed when they are  known to be false positives.  There are so many bogus warnings in layout that I can't practically check for new warnings when I do this sort of change, and that's bad.
(It's also an argument for not having lots of overloads of the same function, but that's the hand we're dealt in this case.)
(In reply to comment #1)
> Hm, it looks like this is all inside a giant "#ifdef MOZ_REFLOW_PERF" block --
> I'm not actually sure why that's getting evaluated in my build.

(Just to follow up on this -- MOZ_REFLOW_PERF is turned on in debug builds:
http://mxr.mozilla.org/mozilla-central/source/configure.in#9093 )
Attached patch patchSplinter Review
Ok, after extensive grepping, I'm confident this is the only place I got this wrong.
Assignee: nobody → zackw
Status: NEW → ASSIGNED
Attachment #529531 - Flags: review?(dholbert)
Comment on attachment 529531 [details] [diff] [review]
patch

Glad to hear it -- thanks for looking into this!
Attachment #529531 - Flags: review?(dholbert) → review+
http://hg.mozilla.org/mozilla-central/rev/551d758ef4ec
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: