222387 - Release SetIcon stored icons when we are done using them

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Daniel de Wildt]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007

Opening/closing a window consums GDI resources. (bug 198916 and bug 204374)
After experimenting with a tool "GDILeak" from Microsoft
http://msdn.microsoft.com/msdnmag/issues/01/03/leaks/default.aspx
http://msdn.microsoft.com/msdnmag/issues/03/01/GDILeaks/
it found that two GDI resource leaks contains the window or taskbar icon.
I think that these icons are never released after they are loaded

Reproducible: Didn't try

Steps to Reproduce:
I don't know if the following remarks are correct, because I'm not firm with the
Mozilla code. I think that in the source file
/seamonkey/source/widget/src/windows/nsWindow.cpp in nsWindow::SetIcon
different taskbar icons are loaded. The first two ::LoadImage didn't use
LR_SHARED. Icons loaded without LR_SHARED should be destroyed with ::DestroyIcon
which seems never be called in nsWindow.cpp
See also the following MSDN links
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/WinUI/WindowsUserInterface/Resources/Icons.asp
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/WinUI/WindowsUserInterface/Resources/IntroductiontoResources/ResourceReference/ResourceFunctions/LoadImage.asp

Actual Results:  
Icon resources are not released after a window is closed.

Expected Results:  
Icon resources should be released after a window is closed.

This icon feature seems to be implemented to solve bug 57576
In bug 180150 are some fixes done and the LR_SHARED was removed.

Comment 1

[timeless]

Note winapi says this is perfectly legal:

http://msdn.microsoft.com/library/en-us/winui/WinUI/WindowsUserInterface/Resources/IntroductiontoResources/ResourceReference/ResourceFunctions/LoadImage.asp


      The system automatically deletes these resources when the process that
created them terminates, however, calling the appropriate function saves memory
and decreases the size of the process's working set.

However, i'm not opposed to releasing in a more timely manner

Comment 2

[timeless]

.

Comment 4

[Daniel de Wildt]

Is in attachment 133424 [details] [diff] [review] WM_SETICON the correct message or should it be WM_GETICON?

btw
These icons should be released to avoid a GDI resource leak. (The resources are
NEVER released at runtime, but at each window creation will need new resources
because the icons are not shared.)
As described in other GDI bugs (like bug 204374), a huge GDI resource consum can
hang a system. It's not possible to get the icon handle after the window handle
is destroyed. The ONLY change to release the resource is by terminating the
process (Mozilla).

Comment 5

[Ere Maijala (slow)]

Comment on attachment 133428 [details] [diff] [review]
 

It won't compile without casting the LRESULTs to HICONs.

Comment 6

[timeless]

reporter: i'm aware of the issues. i've been crashing due to low gdi for longer
than most people have had bugzilla accounts. i don't care. please limit
comments to technical issues, not politics nor whining nor complaints.

as for the code, i'm using seticon. i don't like the idea of destroying an icon
while it's in use. if you do then you can write a patch and explanation as to
why it's ok to destroy an icon while it's in use.

ere: are you sure you looked at the right patch?

Comment 7

[John Morrison]

timeless: I read what Daniel wrote as making a technical statement, not a complaint.

Comment 8

[Ere Maijala (slow)]

Timeless: Yes. You can't just assign the result of SendMessage() to HICON
without casting it.

Comment 9

[timeless]

Attachment: release resources when they're replaced and at window destruction

Comment 10

[Ere Maijala (slow)]

Comment on attachment 133523 [details] [diff] [review]
release resources when they're replaced and at window destruction

r=ere

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133523 [details] [diff] [review]
release resources when they're replaced and at window destruction

sr=bzbarsky

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 133523 [details] [diff] [review]
release resources when they're replaced and at window destruction

We've wrapped up 1.6a. This will have to land when the tree opens for 1.6b.

Comment 13

[Brendan Eich [:brendan]]

Why is this bug UNCONFIRMED?

/be

Comment 14

[timeless]

brendan: no reason, although as gdi bugs go this didn't get any of the spam that
most of them got, for which i'm thankful.

checked in