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;

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

CMenuHandle menu = m_menu.GetSubMenu(0);

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 (23)

  1. Anonymous says:

    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.

  2. Anonymous says:

    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.

  3. Anonymous says:

    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.

  4. Anonymous says:

    Oops, looks like I was wrong. WTL’s CMenu’s destructor destroys the HMENU, but CMenuHandle’s doesn’t. So I don’t know.

  5. Anonymous says:

    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.

  6. Anonymous says:

    the cbSize member of TPMPARAMS doesn’t get set.

  7. Anonymous says:

    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.

  8. Anonymous says:

    It seems as if the menu is positioned right on the rectangle it is supposed to exclude.

  9. Anonymous says:

    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.

  10. 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.

  11. Anonymous says:

    I guess you could be worried about support for RTL reading order. As I tried to trim the sample to the bare minimum I didn’t bother with this. You can use GetSystemMetrics to get the proper alignment, but I’m not sure this constitutes a “nasty bug” so you may be after something else.

    [Kenny, that’s not fair – I sent you an email about this post on Friday so you knew the answer already :).  But you’re right, it’s not a “nasty” bug, it’s a “subtle” bug – my bad. On the other hand, it’s the kind of bug that would stop Windows from shipping – Larry]


  12. Anonymous says:

    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.

  13. Anonymous says:

    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.

  14. Anonymous says:

    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?

  15. Anonymous says:

    Actually, the TrackPopupMenuEx API does seem to be translating TPM_LEFTBUTTON to the right button when the buttons are switched.

    Alright, still looking…

  16. Anonymous says:

    Assumes Left to Right reading?

  17. macbirdie says:

    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.

  18. Anonymous says:

    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, …?

  19. Anonymous says:

    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.

  20. Anonymous says:

    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!

  21. Anonymous says:

    In my last post , I included a snippet from an MSDN article written by Kenny Kerr.  The snippet

  22. Anonymous says:

    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.


  23. Anonymous says:

    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.

Skip to main content