Opened 3 years ago

Last modified 2 years ago

#4338 reviewing defect

The window class for the status bar leaks a system resource

Reported by: eric@… Assignee:
Priority: P4 Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: oleksandr Blocked By:
Blocking: Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29349925/

Description

The implementation of the status bar registers a windows class, which is a system resource (a class atom), but never removes the registration. In an ordinary application, the system automatically does this. In a DLL, however, these registrations persist unless explicitly removed. Since we never remove the registration, the resource leaks when the DLL is unloaded.

How to reproduce

Use IE with AdblockPlus enabled. The resource will leak when IE terminates.

Correct behavior

(1) The easiest way to fix a resource leak is to never allocate it in the first place. The only thing the window class is used for is to initialize the window procedure. This can be also be accomplished by calling SetWindowLongPtr when the window is created.

(2) Another way would be to unregister the class when the DLL is unloaded. This would require action in DllMain message DLL_PROCESS_DETACH. This would retain detached initialization, which means that the initialization would have to be checked for completion each time it's used. There's no substantive initialization issue, though, since the static window procedure is always available. In other words, it's needless effort to do all this instead of just calling SetWindowLongPtr.

This issue is related to #3391 to make detached initialization correct. The window class is (at present) allocated in a detached initialization thread. Eliminating the window class simplifies any detached initialization solution.

Reference

MSDN:RegisterClassEx function

https://msdn.microsoft.com/en-us/library/windows/desktop/ms633587.aspx

No window classes registered by a DLL are unregistered when the DLL is unloaded. A DLL must explicitly unregister its classes when it is unloaded.

What’s the atom returned by RegisterClass useful for?

Raymond Chen, in 2004: https://blogs.msdn.microsoft.com/oldnewthing/20041011-00/?p=37603

In other words, window class atoms are an anachronism.

MSDN: SetWindowLongPtr function

https://msdn.microsoft.com/en-us/library/windows/desktop/ms644898.aspx

Change History (2)

comment:1 Changed 3 years ago by eric@…

  • Review URL(s) modified (diff)

comment:2 Changed 2 years ago by oleksandr

  • Cc oleksandr added
  • Priority changed from Unknown to P4
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.