Last Comment Bug 653705 - MacOSFontEntry::mIsCFF may be used without initialization
: MacOSFontEntry::mIsCFF may be used without initialization
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-29 06:00 PDT by Jonathan Kew (:jfkthame)
Modified: 2011-05-02 02:13 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, ensure IsCFF() initializes the field, whether TRUE or FALSE (1.58 KB, patch)
2011-04-29 06:00 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review-
Details | Diff | Splinter Review
patch, alternative version (900 bytes, patch)
2011-05-02 00:41 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2011-04-29 06:00:56 PDT
Created attachment 529081 [details] [diff] [review]
patch, ensure IsCFF() initializes the field, whether TRUE or FALSE

When we create a MacOSFontEntry for a downloadable font, the mIsCFF field is not initialized. The IsCFF() method correctly sets it to TRUE for CFF fonts, but fails to set it to FALSE otherwise (on the assumption that the field was initialized to FALSE in the first place).

The attached patch explicitly sets it to FALSE in IsCFF unless it's supposed to be TRUE, so that initialization in the constructor is no longer necessary.
Comment 1 John Daggett (:jtd) 2011-05-01 19:08:17 PDT
Comment on attachment 529081 [details] [diff] [review]
patch, ensure IsCFF() initializes the field, whether TRUE or FALSE

> MacOSFontEntry::MacOSFontEntry(const nsAString& aPostscriptName, ATSFontRef aFontRef,
>                                PRUint16 aWeight, PRUint16 aStretch, PRUint32 aItalicStyle,
>                                gfxUserFontData *aUserFontData)
>     : gfxFontEntry(aPostscriptName),
>       mATSFontRef(aFontRef),
>       mATSFontRefInitialized(PR_TRUE),
>       mRequiresAAT(PR_FALSE),
>       mIsCFFInitialized(PR_FALSE)

There are two versions of the constructor, one which initializes
mIsCFF, one which doesn't.  I think both should explicitly initialize
the value, rather than the fix you've written.
Comment 2 Jonathan Kew (:jfkthame) 2011-05-02 00:41:54 PDT
Created attachment 529426 [details] [diff] [review]
patch, alternative version

(In reply to comment #1)
> There are two versions of the constructor, one which initializes
> mIsCFF, one which doesn't.

I know - the patch _removed_ the initialization from the constructor that had it, since it's not needed if IsCFF() explicitly sets it to whichever value is appropriate. That way the field is only ever set in one place.

>  I think both should explicitly initialize
> the value, rather than the fix you've written.

Fine, if you prefer.
Comment 3 John Daggett (:jtd) 2011-05-02 01:29:18 PDT
Comment on attachment 529426 [details] [diff] [review]
patch, alternative version

Yeah, always better to have explicit initialization than rely on the "all codepaths initialize it", that's really easy to break in the future.
Comment 4 Jonathan Kew (:jfkthame) 2011-05-02 02:13:54 PDT
http://hg.mozilla.org/mozilla-central/rev/1b9584c937a7

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