Platform SDK Shell, Resource leak if DestroyMenu is not called for a dynamically added submenu

I'm puzzled by the ownership of submenus. According to tools like
BoundsChecker and Memory Validator, my simple code excerpt below has a
resource leak. On the other hand, the MFC documentation clearly says that
the main menu will take care of destroying the added submenu. A shell
namespace extension needs to do lots of things with menus and I don't want
to leave any resource leaks there.

Code sample:

// Create the main menu.
CMenu menuMain;
menuMain.CreateMenu();

// Create a submenu.
CMenu menuSub;
menuSub.CreatePopupMenu();
menuSub.AppendMenu( MF_STRING, ID_APP_EXIT, ( LPCTSTR )"Exit" );

// Add the submenu to the main menu.
menuMain.AppendMenu( MF_POPUP, ( UINT )menuSub.m_hMenu, "File" );
menuSub.Detach(); // Comment out this line to avoid the leak reports.

Quote from MSDN / CMenu::AppendMenu:
"When nIDNewItem specifies a pop-up menu, it becomes part of the menu to
which it is appended. If that menu is destroyed, the appended menu will also
be destroyed. An appended menu should be detached from a CMenu object to
avoid conflict."

But BoundsChecker and Memory Validator both say that the menu resource
created by the menuSub.CreatePopupMenu() call is not getting destroyed.

The destructor of the CMenu class calls DestroyMenu for the HMENU handle of
menuMain. But is the submenu destroyed as well? According to MSDN it is, but
BoundsChecker and Memory Validator claim otherwise. What is the truth?

(The issue is not MFC-specific. It can easily be reproduced with pure Win32
API calls.)

Antti Nivala

Hello Antti,
I am currently looking into this issue. and will update you later after
doing some research.

Thanks,
Rhett Gong [MSFT]
Microsoft Online Partner Support

This posting is provided "AS IS" with no warranties, and confers no rights.
Please reply to newsgroups only. Thanks.

Hi Antti,
I just add your code in a MFC sdi project, Code added is similar with below:
//-----------------------------------
void CMenuLeakView::OnRButtonDown(UINT nFlags, CPoint point)
{
// TODO: Add your message handler code here and/or call default
CMenu menuMain;
menuMain.CreateMenu();
CMenu menuSub;
menuSub.CreatePopupMenu();
menuSub.AppendMenu( MF_STRING, ID_APP_EXIT, ( LPCTSTR )"Exit" );
menuMain.AppendMenu( MF_POPUP, ( UINT )menuSub.m_hMenu, "File" );
menuSub.Detach(); // Comment out this line to avoid the leak reports.
menuMain.TrackPopupMenu(TPM_LEFTALIGN, point.x,point.y, this);

CView::OnRButtonDown(nFlags, point);
}
//--------------------------------------
I also add a bp in "return ::DestroyMenu(Detach());" this function is
called twice and returns 1 in eax. Then I close the windows, and check
boundschecker, but I can't get any resource leak report from boundschecker.


Best regards,
Rhett Gong [MSFT]
Microsoft Online Partner Support

This posting is provided "AS IS" with no warranties, and confers no rights.
Please reply to newsgroups only. Thanks.

Now that you've been here a while you must surely have become the world's most eclectic PSDK programmer.

--
----------------------------------------------------------
http://www.uscricket.com

I'm inclined to agree with you that it is not a real resource leak, but
since you did not get the same leak reports from BoundsChecker as I did, I
am concerned that perhaps you did not do the same steps as I did. I get the
same leak reports from another tool (Memory Validator) as well, which
suggests that this is not just BoundsChecker giving false reports.

I'm sending you my test project (an SDI project with your code below added
to it, plus ON_WM_RBUTTONDOWN in the message map). I'm also including a
screenshot so that you can see the resource leak report from BoundsChecker
on my computer. Please try with my test project to see if you could get the
same leak report from BoundsChecker and then comment more on this issue.

Thanks,
Antti



below:
boundschecker.
rights.

I reproduced the same test myself. For me, everything is happening like
Antti said. According to BoundsChecker, there was a resource leak. For a
single right-hand click, a breakpoint in "return ::DestroyMenu(Detach());"
was reached only once and BoundsChecker reported a resource leak indicating
the line:

"menuSub.CreatePopupMenu();"

I used the same code as Rhett so probably he just missed something...

Timo



below:
boundschecker.
rights.

Hi Antti,
After comparing your code and mine, I did not find any difference except
the code I used to test before. I remove the code, but still can't get the
leak report in my side, very curious.

Fortunately, I get the leak report in your project. According to the code,
I saw your comment on the "menuSub.Detach();", if we comment out this line,
we can find that "return ::DestroyMenu(Detach());" will be called twice for
the main & sub menu. And if we leave it there, it just returns FALSE in
CMenu::DestroyMenu() without destroying the submenu. And from my first
program, "return ::DestroyMenu(Detach());" is called twice, so I did not
see the leak report from bc.
// mfc source
BOOL CMenu::DestroyMenu()
{
if (m_hMenu == NULL)
return FALSE;
return ::DestroyMenu(Detach());
}
// -----
So I think we should not call the detach for submenu by ourselves, it will
be called inside CMenu::DestroyMenu() and the menu is destroyed before the
main menu is destroyed.

Best regards,
Rhett Gong [MSFT]
Microsoft Online Partner Support

This posting is provided "AS IS" with no warranties, and confers no rights.
Please reply to newsgroups only. Thanks.

Good to hear that you were able to reproduce the resource leak report with
my code.



This is exactly the point of this issue: Should we or should we not call
menuSub.Detach() after adding menuSub to menuMain with AppendMenu. Your
suggestion is that we do not call Detach, leaving the ownership of the
submenu to the CMenu object, in which case it gets destroyed when menuSub
goes out of scope. This sounds reasonable since this way we can avoid the
resource leak report from BoundsChecker and other similar tools.

BUT, your suggestion is exactly the OPPOSITE to what the MSDN documentation
of CMenu::AppendMenu says. Quote from MSDN: "When nIDNewItem specifies a
pop-up menu, it becomes part of the menu to which it is appended. If that
menu is destroyed, the appended menu will also be destroyed. An appended
menu should be detached from a CMenu object to avoid conflict."

This is why I need clarification to this issue from Microsoft. If I follow
the MSDN documentation and violate your suggestion, I get resource leak
reports from BoundsChecker and other tools. If I follow your advise and
violate the MSDN documentation, I do not get the resource leak reports but I
might have the risk of encountering the "conflict" mentioned by the MSDN
documentation.

In other words: Does calling AppendMenu transfer the ownership of the
submenu to the main menu or not? Or, is there some sort of reference
counting involved?

Which one of the following is true:

1) The caller of AppendMenu MUST NOT call DestroyMenu for the submenu
handle.

2) The caller of AppendMenu MUST call DestroyMenu for the submenu handle.

Thanks,
Antti



code,
line,
for
rights.

From my debugging result/bc, I believe we don't need to call detach() on
the submenu after it is appended. Then I checked our bug db but did not
find any bugs about this issue.
I will check the implementation of appendmenu and update you here if I
could find something surprising

Best regards,
Rhett Gong [MSFT]
Microsoft Online Partner Support

This posting is provided "AS IS" with no warranties, and confers no rights.
Please reply to newsgroups only. Thanks.

Hi Antti,
Sorry for my delay.

When a menu is destroyed (either explicitely via DestroyMenu or because the
window or menu it is attached to is destroyed) it will recursively destroy
all of its submenus. When you call menuSub.Detach, menuSub is no longer
attached to menuMain and so will not be destroyed when menuMain is
destroyed. This allows you to reuse menuSub, but also makes you
responsible for destroying menuSub.


Best regards,
Rhett Gong [MSFT]
Microsoft Online Partner Support

This posting is provided "AS IS" with no warranties, and confers no rights.
Please reply to newsgroups only. Thanks.

> When a menu is destroyed (either explicitely via DestroyMenu or because
the

I agree.


I don't agree. As far as I understand, CMenu::Detach just detaches the Win32
menu handle from the wrapper MFC object. It does not affect the relationship
between the "main menu" Win32 handle and "submenu" Win32 handle in any way.
Please correct me if I am wrong.

Note that the problem appears equally with the pure Win32 code below:

// Create the main menu.
HMENU hmenuMain = ::CreateMenu();
_ASSERTE( hmenuMain != NULL );

// Create the submenu.
HMENU hmenuSub = ::CreatePopupMenu();
_ASSERTE( hmenuSub != NULL );
VERIFY( ::AppendMenu( hmenuSub, MF_STRING, ID_APP_EXIT, "Exit" ) );

// Append submenu to main menu.
// Submenu becomes part of the main menu and will get destroyed
// when the main menu is destroyed.
VERIFY( ::AppendMenu( hmenuMain, MF_POPUP, ( UINT_PTR )hmenuSub,
"File" ) );

// Destroy the main menu. This should cause the submenu to get
// destroyed as well.
::DestroyMenu( hmenuMain );

The last line should destroy the submenu as well. The reason why I asked for
your help was because both BoundsChecker from Compuware and Memory Validator
from Software Verification claimed that after the above code has been
executed, the submenu still exists (= resource leak).

Since my original posting, I have contacted Software Verification and they
told that it was just a false leak report from Memory Validator, and they
quickly released an updated version of Memory Validator. Their support
really is fast.

I will also contact Compuware to ask if they have a similar bug in their
BoundsChecker product.

So, I believe that DestroyMenu is working recursively as the Win32 API
documentation says and it just happens that both of these debugging aids
that I was using reported the same false leak. But I must say I don't think
you really understood the point of my question and that your answers to the
question probably just raised more confusion.

Antti

Hello Antti,
I checked the code in CMenu::Detach, and you are right --- "it does not
affect the relationship between the "main menu" Win32 handle and "submenu"
Win32 handle in any way."
I am begining to thinnk this may be a bug in bc, but since I am not sure
how they detect this leak (from handles in process or Create/Destroy pair),
we need some time to check and confirm this.

I've described the problem to Peter, he will contact you in the newsgroup
during my oof time.

Thanks,
Rhett Gong [MSFT]
Microsoft Online Partner Support

This posting is provided "AS IS" with no warranties, and confers no rights.
Please reply to newsgroups only. Thanks.

Hi Antti,

Since Rhett is OOF these days, I will follow up with you on this issue.
If you still have any concern on this issue, please feel free to post here.
Thanks!

Best regards,

Perter Huang
Microsoft Online Partner Support

Get Secure! - www.microsoft.com/security
This posting is provided "AS IS" with no warranties, and confers no rights.

I received a confirmation from Compuware that this is a bug in
BoundsChecker. BoundsChecker mistakenly reports a resource leak about the
submenu even though it has been properly destroyed by calling DestroyMenu
for its parent menu.

Compuware says this problem will be fixed in version 8.0 of DevPartner
Studio.

The other tool vendor, Software Verification, also located a similar bug in
their Memory Validator product and quickly posted a fixed version of it.

Best regards,
Antti

Hi Antti,

Thank you for your sharing the information in the community.


Best regards,

Perter Huang
Microsoft Online Partner Support

Get Secure! - www.microsoft.com/security
This posting is provided "AS IS" with no warranties, and confers no rights.