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)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: bc, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: valgrind)

Attachments

(7 files)

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)
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
Attached patch wip1Splinter Review
Does this fix it?
Flags: needinfo?(bclary)
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.
reftest/valgrind log before patch.
reftest/valgrind log after patch.

The warnings still exist.
Flags: needinfo?(bclary)
Attached patch wip2Splinter Review
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
(In reply to Mats Palmgren (:mats) from comment #7)

> Does this fix it?

yep. No warnings related to font inflation. thanks!
Attachment #8347825 - Attachment description: fix? → wip1
Attachment #8349567 - Attachment description: fix? → wip2
Attachment #8347825 - Attachment is obsolete: false
Attached patch fixSplinter Review
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)
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.

Attachment

General

Created:
Updated:
Size: