192342 - Use EnumFontFamiliesEx rather then EnumFontFamilies (gdi leak)

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Win32, defect, P3)

Description

[David G King]

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

Comment 1

[Radoslaw Sokol]

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.

Comment 2

[David G King]

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

[Radoslaw Sokol]

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

Comment 4

[Samir Gehani]

adt: nsbeta1-

Comment 5

[Malcolm Rowe]

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.

Comment 6

[David G King]

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.

Comment 7

[Kevin McCluskey (gone)]

-> smontagu

Comment 8

[Jim Dunn]

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

Comment 9

[David G King]

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?

Comment 10

[Simon Montagu :smontagu]

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.

Comment 11

[David G King]

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.

Comment 12

[David G King]

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

[Marius Strumyla]

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*

Comment 14

[Simon Montagu :smontagu]

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.

Comment 15

[David G King]

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".

Comment 16

[kinmoz]

Comment on attachment 126293 [details] [diff] [review]
suggested patch

sr=[e-mail]

Comment 17

[Simon Montagu :smontagu]

Fix checked in.

Comment 18

[David G King]

Luxor hasn't caught up, but I've just done a CVS pull which has the patch.
Verifying, and thanks to all.