What’s wrong with this code part 24 (From an MSDN article)?

I ran into this bug earlier today and realized that it’d make an awesome “What’s wrong with this code”.

I started pulling together a test app when I realized that this MSDN magazine article contains sample code that perfectly exhibits the bug:

 CRect rectangle;
VERIFY(m_splitButton.GetWindowRect(
    &rectangle));

TPMPARAMS params = { sizeof(TPMPARAMS) };
params.rcExclude = rectangle;

CMenuHandle menu = m_menu.GetSubMenu(0);

VERIFY(menu.TrackPopupMenuEx(TPM_LEFTBUTTON,
    rectangle.left, rectangle.bottom,
    m_hWnd, &params));

There’s not much to the code – it’s from the handler for the BCN_DROPDOWN notification message.  And it’s got a very nasty subtle bug in it.

So what’s the bug?

 

Edit: s/nasty/subtle/

Comments

• Anonymous
  October 06, 2008
  Well, to start with, if that's MFC then GetWindowRect returns void, so can't be used as an argument to VERIFY.  So the code will compile in Release mode but not Debug mode.  (If it's ATL code then it returns a BOOL so it ought to work ok.) I've never really liked relying on the internals of the structure to initialise specific members (in this case, initialising cbSize in TPMPARAMS), but it ought to work ok. CMenuHandle doesn't show up anywhere in my MFC/ATL reference.  Is this some other framework, then? I'm presuming that the problem is something to do with CMenuHandle.  If it thinks it owns the handle and tries to close it then it'll run into problems. Also, if there was some kind of problem with the menu resource and GetSubMenu returned NULL, then a moment later you're going to be either calling through a null pointer or passing that NULL on to an API that isn't expecting it. There might also be an issue with the menu location (the menu is told to keep out of the button's rect but also to show up directly adjacent to it, which might cause an issue if the button is near the screen edge), but I think the API is smart enough to sort that out so it shouldn't be a problem.

• Anonymous
  October 06, 2008
  Without looking at the code, I guess the bug is CMenuHandle's destructor incorrectly destroys the HMENU returned by GetSubMenu, so if the code gets called a second time, it'll either fail or (worse) crash.

• Anonymous
  October 06, 2008
  I can't see the bug, and running the code it doesn't seem to do anything wrong.  Does the bug in any way depend on using WTL, or the VERIFY() macros?  The way TPMPARAMS is initialised doesn't feel right either, but I can't put my finger on why.

• Anonymous
  October 06, 2008
  Oops, looks like I was wrong. WTL's CMenu's destructor destroys the HMENU, but CMenuHandle's doesn't. So I don't know.

• Anonymous
  October 06, 2008
  Since TPM_RETURNCMD is not used, the call to TrackPopupMenuEx() is not blocking. Hence the menu handle goes out of scope while the menu is being displayed. Doesn't llok very cool to me. Probably bad side effects, such as wrong command notifications when user clicks a menu item.

• Anonymous
  October 06, 2008
  the cbSize member of TPMPARAMS doesn't get set.

• Anonymous
  October 06, 2008
  The rectange goes out of scope but then it's used later by the popup menu. Probably usually works, or one of those where it only works in debug mode.

• Anonymous
  October 06, 2008
  It seems as if the menu is positioned right on the rectangle it is supposed to exclude.

• Anonymous
  October 06, 2008
  D'oh silly me. I didn't see that the y-coordinate was the bottom of the rectangle. Although I guess it could still appear off-screen because no alignment has been set.

• Anonymous
  October 07, 2008
  So far, none of the answers are right. Here's a hint: someone reported a bug in some of our UI code and it turns out that the exact same bug is in this code sample. That means that it's a real bug that had to be fixed for Windows.  And Windows runs in a lots of configurations and places.

• Anonymous
  October 07, 2008
  The comment has been removed

• Anonymous
  October 07, 2008
  Using TPM_LEFTBUTTON only allows people to select menu items with the left button.  I'm not sure what happens on systems where the user elects to swap the left and right buttons.

• Anonymous
  October 07, 2008
  The problem seems to be that the TPM_LEFTBUTTON flag is used. Left handed mouse users will have to use the wrong button to select the menu items. Unless the API does some translation. Testing now.

• Anonymous
  October 07, 2008
  I'd guess it's the use of TPM_LEFTBUTTON.  Does this not behave properly if the user has switched the meaning of the left and right mouse buttons?

• Anonymous
  October 07, 2008
  Actually, the TrackPopupMenuEx API does seem to be translating TPM_LEFTBUTTON to the right button when the buttons are switched. Alright, still looking...

• Anonymous
  October 07, 2008
  Assumes Left to Right reading?

• Anonymous
  October 07, 2008
  Well, if the split button is near the bottom of the screen, context menu should show up so that it fits on the screen. In this case suggesting it a rectangle.bottom might be a problem.

• Anonymous
  October 07, 2008
  At first I thought it might have something to do with screen coordinates vs. client or something similar, but it seems all the calls involved use only screen coordinates.  Maybe it is related to "weird" configurations with multiple monitors, negative coordinates, ...?

• Anonymous
  October 07, 2008
  Menu appears under the second window? I recently stumbled upon such behavior so I'll guess, without analyzing the code, that something similar could be happening here.

• Anonymous
  October 07, 2008
  Bah!  I think you exagerrated the seriousness of this one and didn't give us even the smidgen of a clue to the fact it was only a bug in RTL languages.   I enjoy the "what's wrong with these code" entries, but play fair!

• Anonymous
  October 07, 2008
  In my last post , I included a snippet from an MSDN article written by Kenny Kerr.  The snippet

• Anonymous
  October 07, 2008
  From what I can tell there is a positioning bug when you put the dialog at the far left of the screen. The menu is displayed 20+ pixels to the right of the button. However WTL does not exibit this behavior (at least the version I am using) because the implementation of TrackPopupMenuEx calls a function called _FixTrackMenuPopupX which fixes the problem. Chris

• Anonymous
  October 07, 2008
  I would think a check for left/right handedness of tablet PC users would be useful here, but I havn't tried it out personally. There is also the question of whether to use TPM_VERTICAL or not.