valgrind - Conditional jump or move depends on uninitialised value(s) with Font Inflation

RESOLVED FIXED in mozilla29

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bc, Assigned: mats)

Tracking

({valgrind})

Trunk
mozilla29
x86_64
Linux
valgrind
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

Created attachment 8347678 [details]
warnings for mozilla/layout/reftests/font-inflation/maxRatio-1.html

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)
Created attachment 8347680 [details]
warnings for mozilla/layout/reftests/font-inflation/decoration-1.html
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
(Assignee)

Comment 3

5 years ago
Created attachment 8347825 [details] [diff] [review]
wip1

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.
Created attachment 8349111 [details]
reftest-font-inflation-before.log.gz

reftest/valgrind log before patch.
Created attachment 8349113 [details]
reftest-font-inflation-after.log.gz

reftest/valgrind log after patch.

The warnings still exist.
Flags: needinfo?(bclary)
(Assignee)

Comment 7

5 years ago
Created attachment 8349567 [details] [diff] [review]
wip2

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!
(Assignee)

Updated

4 years ago
Attachment #8347825 - Attachment description: fix? → wip1
(Assignee)

Updated

4 years ago
Attachment #8349567 - Attachment description: fix? → wip2
(Assignee)

Updated

4 years ago
Attachment #8347825 - Attachment is obsolete: false
(Assignee)

Comment 9

4 years ago
Created attachment 8355096 [details] [diff] [review]
fix

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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.