983019 - Conditional jump or move depends on uninitialised value(s) allocated via gfxFcFont::GetOrMakeFont() during reflow

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: assertion/valgrind messages

Full log at <http://sisyphus.bughunter.ateam.phx1.mozilla.com/bughunter/media/logs/2014/03/09/17/06/2014-03-09-17-06-57,firefox,nightly,debug,linux,valgrind2.bughunter.ateam.phx1.mozilla.com,crashtest.log>, requires MozillaVPN, proper acls, etc. See me if you have questions about access.

This build was created on 2014/03/09 and probably won't match the line numbers in a current trunk build. I'm sorry but I can't run valgrind on this machine while another test framework is running. Let me know if this is a problem and I'll try to get current values in a bit.

chunk 39 of 40.

rm -f ./crashtest.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-each-insn --smc-check=all-non-file --soname-synonyms=somalloc=NONE  --trace-children=yes --track-origins=yes --read-var-info=yes' --timeout=86400  --total-chunks=40 --this-chunk=39 '/work/mozilla/builds/nightly/mozilla/testing/crashtest/crashtests.list' | tee ./crashtest.log

gfx/tests/crashtests/798853.html produces several Conditional jump or move depends on uninitialised value(s) all with the same allocation site:

==3130==  Uninitialised value was created by a heap allocation
==3130==    at 0x4A05AF3: malloc (vg_replace_malloc.c:293)
==3130==    by 0x63B27DB: moz_xmalloc (mozalloc.cpp:52)
==3130==    by 0x76B8B60: gfxFcFont::GetOrMakeFont(_FcPattern*, _FcPattern*, gfxFontStyle const*) (mozalloc.h:201)
==3130==    by 0x76B96E2: gfxPangoFontGroup::GetBaseFont() (gfxPangoFonts.cpp:718)
==3130==    by 0x76B9AED: gfxPangoFontGroup::GetFontAt(int) (gfxPangoFonts.cpp:1407)
==3130==    by 0x76799FA: nsFontMetrics::GetMetrics() const (nsFontMetrics.cpp:142)
==3130==    by 0x7679C08: nsFontMetrics::ExternalLeading() (nsFontMetrics.cpp:205)
==3130==    by 0x88DBD04: nsHTMLReflowState::CalcLineHeight(nsStyleContext*, int, float) (nsHTMLReflowState.cpp:2403)
==3130==    by 0x88DBEAB: nsHTMLReflowState::CalcLineHeight() const (nsHTMLReflowState.cpp:2472)
==3130==    by 0x8895E16: nsBlockReflowState::nsBlockReflowState(nsHTMLReflowState const&, nsPresContext*, nsBlockFrame*, bool, bool, bool, int) (nsBlockReflowState.cpp:109)
==3130==    by 0x88BB52E: nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsBlockFrame.cpp:1020)
==3130==    by 0x889D3C3: nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) (nsBlockReflowContext.cpp:260)

It also asserts:
ASSERTION: Failed to create scaled font: 'cairo_scaled_font_status(scaledFont) == CAIRO_STATUS_SUCCESS', file /work/mozilla/builds/nightly/mozilla/gfx/thebes/gfxPangoFonts.cpp, line 2123

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

There are two typos, the first is the cause of the Valgrind warning:
http://hg.mozilla.org/mozilla-central/annotate/b01286b4ed37/gfx/thebes/gfxFT2Utils.cpp#l60
where aMetrics->maxDescent is assigned to itself (it's uninitialized
at this point).  I'm pretty sure aMetrics->emDescent was intended.

The second typo is on line 64: "aSpaceGlyph = 0". I'm pretty sure
*aSpaceGlyph was intended.

It's hard to read this code to verify that all members were indeed
assigned from members that were already initialized.  So I rewrote
it using a few local variables so that's easy to see that all members
derive their value from 'emHeight' at the top.

Comment 2

[Mats Palmgren (inactive)]

I'm guessing it's a regression from bug 505284.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 8390843 [details] [diff] [review]
fix

Review of attachment 8390843 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, both fixes look correct to me (and the rewrite for clarity, thanks!)

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4832153571bc

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4832153571bc