Last Comment Bug 555091 - gfxWindowsFonts.h: FontEntry::FontEntry fails to initialise mWindowsFamily and mWindowsPitch
: gfxWindowsFonts.h: FontEntry::FontEntry fails to initialise mWindowsFamily an...
Status: RESOLVED FIXED
: valgrind
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 1.9.2 Branch
: x86 Windows XP
: -- normal (vote)
: mozilla1.9.3a5
Assigned To: John Daggett (:jtd)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-25 16:11 PDT by Julian Seward [:jseward]
Modified: 2010-04-12 22:44 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.4-fixed
.10-fixed


Attachments
patch, v.0.1, initialize gfxWindowsFamily/Pitch (887 bytes, patch)
2010-03-26 01:02 PDT, John Daggett (:jtd)
jfkthame: review+
mbeltzner: approval1.9.2.4+
mbeltzner: approval1.9.1.10+
Details | Diff | Review
equivalent patch for trunk (945 bytes, patch)
2010-03-27 04:46 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Review

Description Julian Seward [:jseward] 2010-03-25 16:11:14 PDT
1.9.2, win32 non-debug build, running through pages in John Daggett's
simple but very-excellent test

http://people.mozilla.org/~jdaggett/memtesting/iterateblogs.html#60000

running on a hacked version of Valgrind on a hacked version of Wine
running on x86_64 Linux.

After leafing through some number of pages, the errors shown below are
reported.  Because iterateblogs.html works through its collection of
200ish pages in random order, and does not appear to print out the
URLs, I don't know which particular page causes this.  However, two
separate runs showed the problem, one after about 8 mins, the other
after about 70 mins.

The reported errors are shown at the bottom.


ANALYSIS

The uninitialised value uses are reported in
  gfx/thebes/src/gfxWindowsPlatform.cpp 
function
  gfxWindowsPlatform::FindFontForCharProc
lines 644 and 646:

644   if (fe->mWindowsFamily == mfe->mWindowsFamily)
645       rank += 3;
646   if (fe->mWindowsPitch == mfe->mWindowsPitch)
647       rank += 3;

from this plus the undefined-origin data in the errors below, it
follows that mWindowsFamily or mWindowsPitch are uninitialised.

The report implies that the undefined values are created at
gfx/thebes/public/gfxWindowsFonts.h:114:

    FontEntry(const nsAString& aFaceName, gfxWindowsFontType aFontType,
              PRBool aItalic, PRUint16 aWeight, gfxUserFontData *aUserFontData) :
        gfxFontEntry(aFaceName), mFontType(aFontType),
        mForceGDI(PR_FALSE), mUnknownCMAP(PR_FALSE),
        mUnicodeFont(PR_FALSE), mSymbolFont(PR_FALSE),
        mCharset(0), mUnicodeRanges(0)
    {
        mUserFontData = aUserFontData;
        mItalic = aItalic;
        mWeight = aWeight;
        if (IsType1())
            mForceGDI = PR_TRUE;
        mIsUserFont = aUserFontData != nsnull;
    }

Comparing this with the member variables declared at lines 288/289
of the same file, I cannot see where mWindowsFamily and mWindowsPitch
are initialised by the above constructor:

    PRUint8 mWindowsFamily;
    PRUint8 mWindowsPitch;


REPORTED ERRORS

There are two almost identical errors, differing only in that one is
for the comparison at gfxWindowsPlatform.cpp:644 and the other
for the comparison at line 646:

Conditional jump or move depends on uninitialised value(s)
   at 0x10B2A995: gfxWindowsPlatform::FindFontForCharProc (gfxwindowsplatform.cpp:644)
   by 0x1025996F: nsBaseHashtable<nsCStringHashKey,__int64,__int64>::s_EnumStub (nsbasehashtable.h:346)
   by 0x10AA5D5D: PL_DHashTableEnumerate (pldhash.c:754)
   by 0x109CAAFA: nsBaseHashtable<nsCStringHashKey,__int64,__int64>::Enumerate (nsbasehashtable.h:221)
   by 0x10B2ABD4: gfxWindowsPlatform::FindFontForChar (gfxwindowsplatform.cpp:683)
   by 0x10B21D96: gfxWindowsFontGroup::WhichSystemFontSupportsChar (gfxwindowsfonts.cpp:2623)
   by 0x10B178A2: gfxFontGroup::FindFontForChar (gfxfont.cpp:1600)
   by 0x10B17A73: gfxFontGroup::ComputeRanges (gfxfont.cpp:1640)
   by 0x10B21F0E: gfxWindowsFontGroup::InitTextRunUniscribe (gfxwindowsfonts.cpp:2662)
   by 0x10B20945: gfxWindowsFontGroup::MakeTextRun (gfxwindowsfonts.cpp:1499)
   by 0x10B26AFA: TextRunWordCache::MakeTextRun (gfxtextrunwordcache.cpp:683)
   by 0x10B27AAE: gfxTextRunWordCache::MakeTextRun (gfxtextrunwordcache.cpp:992)
   by 0x1044E303: MakeTextRun (nstextframethebes.cpp:436)
   by 0x1044DF86: BuildTextRunsScanner::BuildTextRunForFrames (nstextframethebes.cpp:1783)
   by 0x1044CB6B: BuildTextRunsScanner::FlushFrames (nstextframethebes.cpp:1214)
   by 0x1044F414: BuildTextRuns (nstextframethebes.cpp:1148)

 Uninitialised value was created by a client request
   at 0xC7F773B: RtlAllocateHeap (heap.c:208)
   by 0x78583A57: ???
   by 0x78583B57: ???
   by 0x10B1E2D0: FontEntry::CreateFontEntry (gfxwindowsfonts.cpp:659)
   by 0x10B1DF90: FontEntry::LoadFont (gfxwindowsfonts.cpp:610)
   by 0x10B2AD99: gfxWindowsPlatform::MakePlatformFont (gfxwindowsplatform.cpp:853)
   by 0x10B1BFC4: gfxUserFontSet::OnLoadComplete (gfxuserfontset.cpp:257)
   by 0x10718C62: nsFontFaceLoader::OnStreamComplete (nsfontfaceloader.cpp:139)
   by 0x100C0B35: nsStreamLoader::OnStopRequest (nsstreamloader.cpp:125)
   by 0x1043E37B: nsCrossSiteListenerProxy::OnStopRequest (nscrosssitelistenerproxy.cpp:334)
   by 0x100D2765: nsStreamListenerTee::OnStopRequest (nsstreamlistenertee.cpp:72)
   by 0x10132F76: nsHttpChannel::OnStopRequest (nshttpchannel.cpp:5309)
   by 0x100CA496: nsInputStreamPump::OnStateStop (nsinputstreampump.cpp:576)
   by 0x100C9FDB: nsInputStreamPump::OnInputStreamReady (nsinputstreampump.cpp:401)
   by 0x10ABBFA9: nsInputStreamReadyEvent::Run (nsstreamutils.cpp:112)
   by 0x10ACEA72: nsThread::ProcessNextEvent (nsthread.cpp:527)



and



Conditional jump or move depends on uninitialised value(s)
   at 0x10B2A9B5: gfxWindowsPlatform::FindFontForCharProc (gfxwindowsplatform.cpp:646)
   by 0x1025996F: nsBaseHashtable<nsCStringHashKey,__int64,__int64>::s_EnumStub (nsbasehashtable.h:346)
   by 0x10AA5D5D: PL_DHashTableEnumerate (pldhash.c:754)
   by 0x109CAAFA: nsBaseHashtable<nsCStringHashKey,__int64,__int64>::Enumerate (nsbasehashtable.h:221)
   by 0x10B2ABD4: gfxWindowsPlatform::FindFontForChar (gfxwindowsplatform.cpp:683)
   by 0x10B21D96: gfxWindowsFontGroup::WhichSystemFontSupportsChar (gfxwindowsfonts.cpp:2623)
   by 0x10B178A2: gfxFontGroup::FindFontForChar (gfxfont.cpp:1600)
   by 0x10B17A73: gfxFontGroup::ComputeRanges (gfxfont.cpp:1640)
   by 0x10B21F0E: gfxWindowsFontGroup::InitTextRunUniscribe (gfxwindowsfonts.cpp:2662)
   by 0x10B20945: gfxWindowsFontGroup::MakeTextRun (gfxwindowsfonts.cpp:1499)
   by 0x10B26AFA: TextRunWordCache::MakeTextRun (gfxtextrunwordcache.cpp:683)
   by 0x10B27AAE: gfxTextRunWordCache::MakeTextRun (gfxtextrunwordcache.cpp:992)
   by 0x1044E303: MakeTextRun (nstextframethebes.cpp:436)
   by 0x1044DF86: BuildTextRunsScanner::BuildTextRunForFrames (nstextframethebes.cpp:1783)
   by 0x1044CB6B: BuildTextRunsScanner::FlushFrames (nstextframethebes.cpp:1214)
   by 0x1044F414: BuildTextRuns (nstextframethebes.cpp:1148)

 Uninitialised value was created by a client request
   at 0xC7F773B: RtlAllocateHeap (heap.c:208)
   by 0x78583A57: ???
   by 0x78583B57: ???
   by 0x10B1E2D0: FontEntry::CreateFontEntry (gfxwindowsfonts.cpp:659)
   by 0x10B1DF90: FontEntry::LoadFont (gfxwindowsfonts.cpp:610)
   by 0x10B2AD99: gfxWindowsPlatform::MakePlatformFont (gfxwindowsplatform.cpp:853)
   by 0x10B1BFC4: gfxUserFontSet::OnLoadComplete (gfxuserfontset.cpp:257)
   by 0x10718C62: nsFontFaceLoader::OnStreamComplete (nsfontfaceloader.cpp:139)
   by 0x100C0B35: nsStreamLoader::OnStopRequest (nsstreamloader.cpp:125)
   by 0x1043E37B: nsCrossSiteListenerProxy::OnStopRequest (nscrosssitelistenerproxy.cpp:334)
   by 0x100D2765: nsStreamListenerTee::OnStopRequest (nsstreamlistenertee.cpp:72)
   by 0x10132F76: nsHttpChannel::OnStopRequest (nshttpchannel.cpp:5309)
   by 0x100CA496: nsInputStreamPump::OnStateStop (nsinputstreampump.cpp:576)
   by 0x100C9FDB: nsInputStreamPump::OnInputStreamReady (nsinputstreampump.cpp:401)
   by 0x10ABBFA9: nsInputStreamReadyEvent::Run (nsstreamutils.cpp:112)
   by 0x10ACEA72: nsThread::ProcessNextEvent (nsthread.cpp:527)
Comment 1 John Daggett (:jtd) 2010-03-26 01:02:12 PDT
Created attachment 435118 [details] [diff] [review]
patch, v.0.1, initialize gfxWindowsFamily/Pitch
Comment 2 Julian Seward [:jseward] 2010-03-26 04:58:38 PDT
The particular page causing this report is: http://ascher.ca/blog
Comment 3 Jonathan Kew (:jfkthame) 2010-03-27 04:10:21 PDT
Comment on attachment 435118 [details] [diff] [review]
patch, v.0.1, initialize gfxWindowsFamily/Pitch

Yes, we shouldn't leave them uninitialized.

(Ideally, we should be setting them based on the characteristics of the user font when it's loaded, so that they can appropriately influence any system fallback that occurs, but IMO that's a pretty low priority.)
Comment 4 Jonathan Kew (:jfkthame) 2010-03-27 04:46:00 PDT
Created attachment 435372 [details] [diff] [review]
equivalent patch for trunk

We should do the equivalent initialization on trunk as well, although I don't think we are currently using these fields in such a way that we'd actually read them while they're uninitialized.
Comment 5 Julian Seward [:jseward] 2010-03-27 05:40:59 PDT
(In reply to comment #4)
> Created an attachment (id=435372) [details]
> equivalent patch for trunk

Confused.  I thought the patch in comment #1 _was_ for trunk,
since it applies to gfx/thebes/src/gfxGDIFontList.cpp, and
I can't find any such file in my 1.9.2 tree.
Comment 6 Jonathan Kew (:jfkthame) 2010-03-27 05:47:26 PDT
So it is - sorry, I'm the one who was confused! The bug was reported for the 1.9.2 branch, where the code is different.

Ok, then should we ask to fix this on 1.9.2 as well - which is where you got the valgrind error? I don't think that will actually occur with the current trunk code, AFAICT.
Comment 7 Julian Seward [:jseward] 2010-03-27 06:35:27 PDT
(In reply to comment #6)
> Ok, then should we ask to fix this on 1.9.2 as well - which is where you got
> the valgrind error? I don't think that will actually occur with the current
> trunk code, AFAICT.

The problem was observed on 1.9.2, but John's fix is for the trunk.
So I'd guess that implies the problem could occur both on 192 or trunk.
Comment 8 Jonathan Kew (:jfkthame) 2010-03-27 06:44:36 PDT
The "problem" of those fields being left uninitialized in the fontentry (for downloaded fonts) does occur on trunk, yes. However, I don't think our trunk code ever looks at those uninitialized fields for downloaded fonts, in the way 1.9.2 does, because of changes in how font fallback matching is done.

So as of now, trunk will not actually show the same problem, but we should initialize the fields anyway for future robustness.
Comment 9 John Daggett (:jtd) 2010-04-06 00:59:04 PDT
Pushed to trunk
http://hg.mozilla.org/mozilla-central/rev/1751ac4a04a8
Comment 10 John Daggett (:jtd) 2010-04-06 01:04:33 PDT
Comment on attachment 435118 [details] [diff] [review]
patch, v.0.1, initialize gfxWindowsFamily/Pitch

Same two fields need to be initialized in older code.
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-07 13:07:34 PDT
Comment on attachment 435118 [details] [diff] [review]
patch, v.0.1, initialize gfxWindowsFamily/Pitch

a=beltzner for 1.9.2.4 and 1.9.1.10 - no verification needed (valgrind issue)
Comment 12 Reed Loden [:reed] (use needinfo?) 2010-04-12 22:02:06 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ade6f1838d0a
Comment 13 Reed Loden [:reed] (use needinfo?) 2010-04-12 22:44:37 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5fe90279ec03

Note You need to log in before you can comment on or make changes to this bug.