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...
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 )
Created attachment 529531 [details] [diff] [review] patch Ok, after extensive grepping, I'm confident this is the only place I got this wrong.
Comment on attachment 529531 [details] [diff] [review] patch Glad to hear it -- thanks for looking into this!