Last Comment Bug 653179 - nsPresShell.cpp:8829:52: warning: 'width' may be used uninitialized in this function
: nsPresShell.cpp:8829:52: warning: 'width' may be used uninitialized in this f...
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Zack Weinberg (:zwol)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 266236
  Show dependency treegraph
 
Reported: 2011-04-27 10:41 PDT by Daniel Holbert [:dholbert]
Modified: 2011-05-02 15:28 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (986 bytes, patch)
2011-05-02 11:40 PDT, Zack Weinberg (:zwol)
dholbert: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-04-27 10:41:46 PDT
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...
Comment 1 Daniel Holbert [:dholbert] 2011-04-27 10:49:06 PDT
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?)
Comment 2 Zack Weinberg (:zwol) 2011-04-27 10:58:06 PDT
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.
Comment 3 Zack Weinberg (:zwol) 2011-04-27 10:58:55 PDT
(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.)
Comment 4 Daniel Holbert [:dholbert] 2011-04-27 11:06:08 PDT
(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 )
Comment 5 Zack Weinberg (:zwol) 2011-05-02 11:40:43 PDT
Created attachment 529531 [details] [diff] [review]
patch

Ok, after extensive grepping, I'm confident this is the only place I got this wrong.
Comment 6 Daniel Holbert [:dholbert] 2011-05-02 11:46:36 PDT
Comment on attachment 529531 [details] [diff] [review]
patch

Glad to hear it -- thanks for looking into this!
Comment 7 Zack Weinberg (:zwol) 2011-05-02 15:28:14 PDT
http://hg.mozilla.org/mozilla-central/rev/551d758ef4ec

Note You need to log in before you can comment on or make changes to this bug.