Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

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

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

  • Comments 23

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/

 

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

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

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

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

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

  • the cbSize member of TPMPARAMS doesn't get set.

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

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

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

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

  • 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]

     

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

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

  • 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?

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

    Alright, still looking...

Page 1 of 2 (23 items) 12