555091 - gfxWindowsFonts.h: FontEntry::FontEntry fails to initialise mWindowsFamily and mWindowsPitch

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Julian Seward [:jseward]]

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)]

Attachment: patch, v.0.1, initialize gfxWindowsFamily/Pitch

Comment 2

[Julian Seward [:jseward]]

The particular page causing this report is: http://ascher.ca/blog

Comment 3

[Jonathan Kew [:jfkthame]]

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]]

Attachment: 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]]

(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]]

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]]

(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]]

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)]

Pushed to trunk
http://hg.mozilla.org/mozilla-central/rev/1751ac4a04a8

Comment 10

[John Daggett (:jtd)]

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]]

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]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ade6f1838d0a

Comment 13

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5fe90279ec03