Use EnumFontFamiliesEx rather then EnumFontFamilies (gdi leak)

VERIFIED FIXED in Future

Status

Core Graveyard
GFX: Win32
P3
normal
VERIFIED FIXED
16 years ago
4 years ago

People

(Reporter: David G King, Assigned: smontagu)

Tracking

Trunk
Future
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030203
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030203

EnumFontFamilies is a 16-bit function that can cause GDI leaks.

From MSDN :-

"Windows GDI
EnumFontFamilies
The EnumFontFamilies function enumerates the fonts in a specified font family
that are available on a specified device.
Note  This function is provided only for compatibility with 16-bit versions of
Windows. Applications should use the EnumFontFamiliesEx function. "


Reproducible: Always

Steps to Reproduce:





This GDI call seems to only exist in /gfx/src/windows/nsDeviceContextWin.cpp,
line 610
(Reporter)

Updated

16 years ago
Blocks: 159298

Updated

16 years ago
Priority: -- → P3
Target Milestone: --- → Future
Summary: Use EnumFontFamiliesEx rather then EnumFontFamilies → Use EnumFontFamiliesEx rather then EnumFontFamilies (gdi leak)

Updated

15 years ago
Blocks: 204374

Comment 1

15 years ago
As far as I know, EnumFontFamilies() is a 32-bit call
wrapper to more "powerful" EnumFontFamiliesEx(), so it
should not cause any leak. Anyway, it's worth replacing
with direct call to EnumFontFamiliesEx() just to get
performance boost.
(Reporter)

Comment 2

15 years ago
I'm not sure how MS implement EnumFontFamilies() in 32-bit systems, however the
following statements from Microsoft are illuminating :

- From MSDN
The EnumFontFamilies function enumerates the fonts in a specified font family
that are available on a specified device.
Note  This function is provided only for compatibility with 16-bit versions of
Windows. Applications should use the EnumFontFamiliesEx function.

-From MSDN
Obsolete Functions

These functions are provided only for compatibility with 16-bit versions of Windows.

EnumFontFamilies
EnumFontFamProc
EnumFonts
EnumFontsProc
GetCharWidth
GetTextExtentPoint

so, keeping in mind that Microsoft seem to only issue warnings when things are
really bad, Mozilla needs to change over.

Comment 3

15 years ago
The facts are:

1) on Win9x/WinME, many GDI functions are in fact 16-bit (inter-
   nally), so Mozilla will anyway land in 16-bit system code
   (EnumFontFamilies() itself will be 32-bit function declared
   in gdi32.dll anyway, it may only lead to gdi.exe syscall)

2) on WinNT/2k/XP none of the GDI calls may land in 16-bit area,
   so EnumFontFamilies() is surely a 32-bit function

3) Microsoft marks these (and much more, like ScrollWindow()
   for example) functions as obsolete, because they changed
   the API; but marking these functions as obsolete does not
   mean that they are faulty or implemented as 16-bit: it just
   means that they should be not used in new software, as they
   only wrap to the new, enhanced API

MS left these obsolete calls to minimise source code changes
needed to port application from Win16 to Win32. So, as a Win32
API programmer with several years of experience, I am pretty
sure that using EnumFontFamilies() is not much more dangerous
than using EnumFontFamiliesEx() -- but I agree that it should
be changed (just because it will save one jump and internal
parameter translation in EnumFontFamilies() wrapper).

Updated

15 years ago
Flags: blocking1.4?

Updated

15 years ago
Keywords: nsbeta1

Comment 4

15 years ago
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Comment 5

15 years ago
Note: The origin of the '(gdi leak)' comment is bug 159298 comment 15, which 
states that 'a 16-bit GDI call will not release memory (GDI resources) even 
after Mozilla is closed'.  I'd guess that it only applies to Win 9x/Me, though.
(Reporter)

Comment 6

15 years ago
This bug is a strange one. It applies to all 32-bit versions of MS Windows,
however Win 9x and Win ME are more prone to this problem due to how MS
implemented GDI on those OS's. Win XP and Win 2K do also have this problem, but
hardly ever is it visible to the user.

In fact, testing any GDI leak, or fix for a leak is best done on a Win 9x system
due to the speed at which the problem appears.
-> smontagu
Assignee: kmcclusk → smontagu

Comment 8

15 years ago
Created attachment 126293 [details] [diff] [review]
suggested patch

I didn't do extensive testing with this, not sure how
much (if any) it helps.  I basically came across this
bug (trying to track down GDI leaks) and wanted to
try the "fix" to see if it helped my problems - it doesn't.

Anyhow here is the patch
(Reporter)

Comment 9

15 years ago
That fix looks good to me (based on research at MSDN, rather than extensive
Mozilla knowmedge). I'm not sure if it saves much either, but it was annoying to
have it there when Mozilla has GDI issues.

Do you think the patch is ready for an "r" request?
(Assignee)

Comment 10

15 years ago
Comment on attachment 126293 [details] [diff] [review]
suggested patch

After discussing this with jdunn on IRC and testing it myself, I am giving this
r=me. I would like to see a source for
http://bugzilla.mozilla.org/show_bug.cgi?id=159298#c15 to assess whether we
should aim to get this into 1.4 as well.
Attachment #126293 - Flags: superreview?(kin)
Attachment #126293 - Flags: review+
(Reporter)

Comment 11

15 years ago
Re comment15 and comment #10, I'm now going back to MSDN to find my source for
that info that I reported in Bug #159298.
(Reporter)

Comment 12

15 years ago
I can't find my referance, so don't worry about 1.4final unless I do find it
later (not that I have much time before 1.4 is released). However, putting this
patch in the Trunk is still a good idea.

Comment 13

15 years ago
Free System Resources Do Not Return to Previous Value
http://support.microsoft.com/search/preview.aspx?scid=kb;en-us;146418

"Windows does not free system resources abandoned by Windows 3.1-based programs
until all Windows 3.1-based programs have been closed. Only when there are no
Windows 3.1-based programs running can Windows safely release abandoned system
resources."

i think this is *the source*
(Assignee)

Comment 14

15 years ago
I don't think that is the same thing at all. That reference seems to refer to a
complete Win16 executable running on a Win32 system, not a Win32 executable that
makes calls to deprecated Win16 APIs, as in the case here.

I still think that we should get this in to the trunk, but I won't pursue it on
the branch since (a) there doesn't seem to be any firm evidence that a leak is
involved and (b) AFAICT this code is not much called anyway, except by MathML.
(Reporter)

Comment 15

15 years ago
Re Comment #13, that sounds familiar. I think it is worded better somewhere
else, however that referance gives the basics of the issue.

Your quote from the URL could be misleading. So here is another quote - "If a
program requests a service that uses deferred initialization, the service
remains initialized after the program has exited. The system resources
associated with that service are not freed. The system keeps the service
initialized so that the next program that requests the service does not have to
wait for the service to be initialized." One must keep in mind that loading
fonts is defined as "deferred initialization".

Updated

15 years ago
Flags: blocking1.4?

Comment 16

15 years ago
Comment on attachment 126293 [details] [diff] [review]
suggested patch

sr=kin@netscape.com
Attachment #126293 - Flags: superreview?(kin) → superreview+
(Assignee)

Comment 17

15 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

15 years ago
Luxor hasn't caught up, but I've just done a CVS pull which has the patch.
Verifying, and thanks to all.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.