Opened on 08/16/2016 at 02:24:16 PM

Last modified on 09/06/2016 at 05:30:32 PM

#4338 reviewing defect

The window class for the status bar leaks a system resource

Reported by: eric@adblockplus.org 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

Attachments (0)

Change History (2)

comment:1 Changed on 08/17/2016 at 08:25:45 PM by eric@adblockplus.org

  • Review URL(s) modified (diff)

comment:2 Changed on 09/06/2016 at 05:30:32 PM by oleksandr

  • Cc oleksandr added
  • Priority changed from Unknown to P4
  • Status changed from new to reviewing

Add Comment

Modify Ticket

Change Properties
Action
as reviewing .
as The resolution will be set. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
 
Note: See TracTickets for help on using tickets.