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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jseward, Assigned: jtd)

Tracking

({valgrind})

1.9.2 Branch
mozilla1.9.3a5
x86
Windows XP
valgrind
Points:
---

Firefox Tracking Flags

(status1.9.2 .4-fixed, status1.9.1 .10-fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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)
(Reporter)

Updated

7 years ago
Keywords: valgrind
(Assignee)

Comment 1

7 years ago
Created attachment 435118 [details] [diff] [review]
patch, v.0.1, initialize gfxWindowsFamily/Pitch
Attachment #435118 - Flags: review?(jfkthame)
(Reporter)

Comment 2

7 years ago
The particular page causing this report is: http://ascher.ca/blog
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.)
Attachment #435118 - Flags: review?(jfkthame) → review+
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.
Attachment #435372 - Flags: review?(jdaggett)
(Reporter)

Comment 5

7 years ago
(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.
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.
Attachment #435372 - Attachment is obsolete: true
Attachment #435372 - Flags: review?(jdaggett)
(Reporter)

Comment 7

7 years ago
(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.
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.
(Assignee)

Comment 9

7 years ago
Pushed to trunk
http://hg.mozilla.org/mozilla-central/rev/1751ac4a04a8
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

7 years ago
Comment on attachment 435118 [details] [diff] [review]
patch, v.0.1, initialize gfxWindowsFamily/Pitch

Same two fields need to be initialized in older code.
Attachment #435118 - Flags: approval1.9.2.4?
Attachment #435118 - Flags: approval1.9.1.10?
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)
Attachment #435118 - Flags: approval1.9.2.4?
Attachment #435118 - Flags: approval1.9.2.4+
Attachment #435118 - Flags: approval1.9.1.10?
Attachment #435118 - Flags: approval1.9.1.10+
Assignee: nobody → jdaggett
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ade6f1838d0a
status1.9.2: --- → .4-fixed
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5fe90279ec03
status1.9.1: --- → .10-fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.