Closed
Bug 950411
Opened 10 years ago
Closed 10 years ago
valgrind - Conditional jump or move depends on uninitialised value(s) with Font Inflation
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: bc, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: valgrind)
Attachments
(7 files)
9.09 KB,
text/plain
|
Details | |
3.37 KB,
text/plain
|
Details | |
808 bytes,
patch
|
Details | Diff | Splinter Review | |
15.92 KB,
application/x-gzip
|
Details | |
16.04 KB,
application/x-gzip
|
Details | |
786 bytes,
patch
|
Details | Diff | Splinter Review | |
752 bytes,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
May be a dupe of bug 797002 rm -f ./reftest.log && /work/mozilla/builds/nightly/mozilla/firefox-debug/_virtualenv/bin/python _tests/reftest/runreftest.py --extra-profile-file=./dist/plugins --symbols-path=./dist/crashreporter-symbols --debugger='valgrind' --debugger-args='--tool=memcheck --vex-iropt-register-updates=allregs-at-mem-access' --timeout=86400 --total-chunks=20 --this-chunk=10 '/work/mozilla/builds/nightly/mozilla/layout/reftests/reftest.list' | tee ./reftest.log REFTEST TEST-START | file:///work/mozilla/builds/nightly/mozilla/layout/reftests/font-inflation/maxRatio-1.html SET PREFERENCE pref(font.size.inflation.emPerLine,15) SET PREFERENCE pref(font.size.inflation.forceEnabled,true) SET PREFERENCE pref(font.size.inflation.lineThreshold,0) SET PREFERENCE pref(font.size.inflation.maxRatio,200) REFTEST TEST-LOAD | file:///work/mozilla/builds/nightly/mozilla/layout/reftests/font-inflation/maxRatio-1.html | 451 / 509 (88%) ==7769== Conditional jump or move depends on uninitialised value(s) ==7769== at 0x4101AF: int const& std::min<int>(int const&, int const&) (stl_algobase.h:198) ==7769== by 0x90705FA: MinimumFontSizeFor(nsPresContext*, int) (nsLayoutUtils.cpp:5431) ... ==7769== Uninitialised value was created by a heap allocation ==7769== at 0x4A0536D: malloc (vg_replace_malloc.c:291) ==7769== by 0x63F31D6: moz_xmalloc (mozalloc.cpp:52) ==7769== by 0x90D373D: nsFontInflationData::UpdateFontInflationDataWidthFor(nsHTMLReflowState const&) (mozalloc.h:201) } ==7769== Conditional jump or move depends on uninitialised value(s) ==7769== at 0x6FE4C64: int const& std::max<int>(int const&, int const&) (stl_algobase.h:221) ==7769== by 0x90706A6: MinimumFontSizeFor(nsPresContext*, int) (nsLayoutUtils.cpp:5446) ... ==7769== Uninitialised value was created by a heap allocation ==7769== at 0x4A0536D: malloc (vg_replace_malloc.c:291) ==7769== by 0x63F31D6: moz_xmalloc (mozalloc.cpp:52) ==7769== by 0x90D373D: nsFontInflationData::UpdateFontInflationDataWidthFor(nsHTMLReflowState const&) (mozalloc.h:201) ==7769== Conditional jump or move depends on uninitialised value(s) ==7769== at 0x90706E2: nsLayoutUtils::FontSizeInflationInner(nsIFrame const*, int) (nsLayoutUtils.cpp:5462) ... ==7769== by 0x910B1F6: nsHTMLReflowState::InitResizeFlags(nsPresContext*, nsIAtom*) (nsHTMLReflowState.cpp:444) ==7769== Uninitialised value was created by a heap allocation ==7769== at 0x4A0536D: malloc (vg_replace_malloc.c:291) ==7769== by 0x63F31D6: moz_xmalloc (mozalloc.cpp:52) ==7769== by 0x90D373D: nsFontInflationData::UpdateFontInflationDataWidthFor(nsHTMLReflowState const&) (mozalloc.h:201)
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
fyi, there are other tests which also show problems. The tests are still running so this is not a complete list. int const& std::min(int const&, int const&) (stl_algobase.h:198) * layout/reftests/font-inflation/threshold-1a.html * layout/generic/crashtests/762902.html nsLayoutUtils::FontSizeInflationInner(nsIFrame const*, int) (nsLayoutUtils.cpp:5462) * layout/reftests/font-inflation/threshold-scope-cell-1.html * layout/reftests/font-inflation/threshold-scope-cell-1.html
Reporter | ||
Comment 4•10 years ago
|
||
I did a base line run without the patch for the specific chunk on my local workstation but they weren't in the chunk any more. I'll redo this using the font-inflation/reftest.list to make sure I get it. More news as I get it.
Reporter | ||
Comment 5•10 years ago
|
||
reftest/valgrind log before patch.
Reporter | ||
Comment 6•10 years ago
|
||
reftest/valgrind log after patch. The warnings still exist.
Flags: needinfo?(bclary)
Assignee | ||
Comment 7•10 years ago
|
||
Does this fix it? (valgrind doesn't work properly on my machine for some reason, so I can't test myself, sorry)
Attachment #8347825 -
Attachment is obsolete: true
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #7) > Does this fix it? yep. No warnings related to font inflation. thanks!
Assignee | ||
Updated•10 years ago
|
Attachment #8347825 -
Attachment description: fix? → wip1
Assignee | ||
Updated•10 years ago
|
Attachment #8349567 -
Attachment description: fix? → wip2
Assignee | ||
Updated•10 years ago
|
Attachment #8347825 -
Attachment is obsolete: false
Assignee | ||
Comment 9•10 years ago
|
||
OK, thanks for testing. There are two separate paths that could lead to UMR in this code. In UpdateFontInflationDataWidthFor we lookup an existing nsFontInflationData in a frame property, or if non-existant, create a new instance (and set the property): http://hg.mozilla.org/mozilla-central/annotate/c8d5a871ae32/layout/generic/nsFontInflationData.cpp#l46 then we call UpdateWidth() for that instance. In case it's a new instance, mNCAWidth is not intialized, UpdateWidth() is supposed to do that, but fails if we take the early return on line 192. (the mNCAWidth assignment is on line 223). http://hg.mozilla.org/mozilla-central/annotate/c8d5a871ae32/layout/generic/nsFontInflationData.cpp#l179 (that's why I guessed "wip1" would fix it) The other path is what Valgrind warns about in the first attachment: ==7769== Conditional jump or move depends on uninitialised value(s) int const& std::min<int>(int const&, int const&) MinimumFontSizeFor(...) (nsLayoutUtils.cpp:5431) nsLayoutUtils::InflationMinFontSizeFor(...) (nsLayoutUtils.cpp:5578) nsLayoutUtils::FontSizeInflationFor(...) (nsLayoutUtils.cpp:5604) FontSizeInflationListMarginAdjustment(...) (nsHTMLReflowState.cpp:104) nsCSSOffsetState::ComputeMargin(...) (nsHTMLReflowState.cpp:2499) nsCSSOffsetState::InitOffsets(...) (nsHTMLReflowState.cpp:2178) nsHTMLReflowState::InitConstraints(...) (nsHTMLReflowState.cpp:1940) nsHTMLReflowState::Init(...) (nsHTMLReflowState.cpp:322) nsHTMLReflowState::nsHTMLReflowState(...) (nsHTMLReflowState.cpp:193) ComputeDescendantWidth(...) (nsFontInflationData.cpp:163) nsFontInflationData::UpdateWidth(...) (nsFontInflationData.cpp:209) which means we're using the current mNCAWidth value while calculating it's new value. I wonder if that's a healthy thing to do... Anyway, initializing mNCAWidth in the constructor seems like the right thing to do for both problems.
Assignee: nobody → matspal
Attachment #8355096 -
Flags: review?(roc)
Attachment #8355096 -
Flags: review?(roc) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3919ce6f5b0c
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3919ce6f5b0c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•