Closed Bug 983019 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics: Text, defect)

x86_64
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bc, Assigned: MatsPalmgren_bugz)

References

()

Details

(Keywords: regression, valgrind)

Attachments

(2 files)

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
Attached patch fixSplinter Review
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.
Assignee: nobody → matspal
Attachment #8390843 - Flags: review?(jfkthame)
I'm guessing it's a regression from bug 505284.
Blocks: 505284
Severity: normal → major
Keywords: regression
Severity: major → minor
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!)
Attachment #8390843 - Flags: review?(jfkthame) → review+
Component: Layout: Text → Graphics: Text
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4832153571bc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.